Re: [Libguestfs] [PATCH libnbd v3 1/2] lib: Implement closure lifetimes.

On 7/25/19 8:07 AM, Richard W.M. Jones wrote: Always good to see testsuite coverage of new features :) I guess I'll see below if I can offer more ideas for what the test can do. Missing a change to the auto-retire text and error handling text (since we have places where we now ignore the return value when calling with only FREE); it may be worth squashing in: diff --git i/docs/libnbd.pod w/docs/libnbd.pod index 4d31c64..1fcbfd7 100644 --- i/docs/libnbd.pod +++ w/docs/libnbd.pod @@ -501,9 +501,10 @@ complete. The completion callback will be invoked with C<cookie> set to the same value returned by the original API such as C<nbd_aio_pread_callback> (in rare cases, it is possible that the completion callback may fire before the original API has returned). -If the completion callback returns C<1>, the command is automatically -retired (there is no need to call C<nbd_aio_command_completed>); for -any other return value, the command still needs to be retired. +When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the +completion callback returns C<1>, the command is automatically retired +(there is no need to call C<nbd_aio_command_completed>); for any other +return value, the command still needs to be retired. =head2 Callbacks with C<int *error> parameter @@ -515,8 +516,9 @@ all of the completion callbacks, include a parameter C<error> containing the value of any error detected so far; if the callback function fails, it should assign back into C<error> and return C<-1> to change the resulting error of the overall command. Assignments -into C<error> are ignored for any other return value; similarly, +into C<error> are ignored for any other return value or when +C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly, assigning C<0> into C<error> does not have an effect. =head1 SEE ALSO Was this patch posted with whitespace changes ignored? Otherwise, it looks like you missed reindenting things. Indentation. and again; I'll quit pointing it out for args=[]. Another place where indentation looks odd, unless this email was generated with whitespace changes ignored. (Cleaning up whitespace in a separate patch to let THIS patch focus on the semantic changes is also acceptable) Same here. We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that can be a followup. Hmm - no change to the FINISH state, which means you are relying on command retirement to free chunk/extent instead. As long as that happens, we should be okay, though. Moving the side effect out of the switch() condition is a reasonable move; my fault for putting it there in the first place. Looks like a nice helper function. (Side note: using 'git config diff.orderfile some/file' can be a way to rearrange patches to be easier to review logically: I think placing lib/internal.h and generator/generator first, then generator/* and lib/* prior to tests/* interop/* examples/* would have have made this review easier to read - maybe I should propose a scripts/git.orderfile similar to nbdkit). Looks odd that this was not spelled 'cmd->type == NBD_CMD_BLOCK_STATUS'. Perhaps we could even have: switch (cmd->type) { case NBD_CMD_READ: if (cmd->cb.fn.read) ... break; case NBD_CMD_BLOCK_STATUS: if (cmd->cb.fn.extent) ... break; default: assert (!cmd->cb.fn.read); } But as written, the function operates correctly. So up to you if you want to tweak it. I recommend either setting h->debug_fn = NULL here, or deferring the FREE callback to after the h->sock->ops->close (h->sock) below. Otherwise, a future edit to lib/sockets.c to add in a debug statement there will cause a use-after-free at a distance. Is it worth assert(!debug_fn_free), to prove that we never have use-after-free? Same here. and again If you add asserts against use-after-free above, you'd also need to reset debug_fn_free back to 0 here. with knock-on affects to what you assert here. Is there any way to reliably test whether debug_fn_valid was incremented? (To some extent, it depends on whether nbd_set_debug was used, and if we know for sure that we triggered an action that results in a debug statement). Testing for >0 is sufficient, testing for a specific value would be fragile as we may add or remove debug calls in future refactorings. Side effects in an assert. Nasty. (Maybe you can get away with it if we explicitly #undef NDEBUG at the top of this file, so the test still works if someone does ./configure CFLAGS=-DNDEBUG, but splitting the side effects from the test validation seems wise) More side effects in an assert :( We can use --filter=delay delay-read=15 to ensure the server is not very fast, to reduce the risk of the race (but only for this part of the test, not the earlier part where we do wait for completion - which means two separate parameter lists for nbd_connect_command() calls...) side effects :( Worth asserting that read_cb_valid and completion_cb_valid are 0 (if we are confident in our ability to strand the command unretired due to use of a filter delay)? Most of my comments can be addressed by followups or pertain to the new test; I'm okay if you want to push this without posting v4. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org