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
