Re: [PATCH for-7.1 6/8] nbd: move s->state under requests_lock

[Paolo Bonzini]

On 4/13/22 18:23, Eric Blake wrote:
> > The function nbd_client_connecting_wait() was used mostly to check if
> > a request had to be reissued (outside requests_lock), but also
> > under requests_lock in nbd_client_connecting_wait().  The two uses have to
> "Function A was used mostly..., but also under requests_lock in
> function A."  Reading the rest of the patch, I think...[1]
>
> > be separated; for the former we rename it to nbd_client_will_reconnect()
> > and make it take s->requests_lock; for the latter the access can simply
> > be inlined.  The new name is clearer, and ensures that a missing
> > conversion is caught by the compiler.
>
> I take it your experiments with C++ coroutines helped find this;)

No, they never went that far. :)  Rather, these atomics have always 
bugged me, and after Emanuele pointed me to the enter_all without lock, 
I noticed that they can be fixed with the same hammer.

> > +    QEMU_LOCK_GUARD(&s->requests_lock);
> > +    return s->state == NBD_CLIENT_CONNECTING_WAIT;
> >  }
>
> [2]...while here, you only needed two lines, using QEMU_LOCK_GUARD.
> Both styles work, but it seems like we should be consistent, and I
> would favor the shorter style when all that is being guarded is a
> single line.

QEMU_LOCK_GUARD() is a declaration in some sense (well, it is also a 
declaration when you expand the macro) and QEMU in general doesn't do 
declaration-after-statement.

Also, QEMU_LOCK_GUARD() emphasizes that the whole function is guarded, 
while WITH_QEMU_LOCK_GUARD() has the opposite effect on the reader.

> although the suggestion in [3] to split out the function motion to a
> separate patch may result in the v2 series looking different enough
> that you may want to leave off my R-b to ensure I still review things
> carefully.

Will do.

Paolo