On Thu, Feb 03, 2022 at 02:10:26PM +0200, Nir Soffer wrote: > > Some of the high-level commands (L<nbd_pread_structured(3)>, > > -L<nbd_block_status(3)>) involve the use of a callback function invoked by > > -the state machine at appropriate points in the server's reply before > > -the overall command is complete. These callback functions, along with > > -all of the completion callbacks, include a parameter C<error> > > -containing the value of any error detected so far; if the callback
Pre-existing, but... > > -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, > > -assigning C<0> into C<error> does not have an effect. > > +L<nbd_block_status(3)>) involve the use of a callback function invoked > > +by the state machine at appropriate points in the server's reply > > +before the overall command is complete. These callback functions, > > +along with all of the completion callbacks, include a parameter > > +C<error> containing the value of any error detected so far. If a > > Error is a pointer to the value ...yes, I'll improve that. > > > +callback function fails and wants to change the resulting error of the > > +overall command visible later in the API sequence, it should assign > > +back into C<error> and return C<-1>. Assignments into C<error> are > > +ignored for any other return value; similarly, assigning C<0> into > > +C<error> does not have an effect. > > But maybe the text about assigning into it is good enough to make this clear. > > > + > > +Note that a mid-command callback might never be reached, such as if > > +libnbd detects that the command was invalid to send (see > > +L<nbd_set_strict_mode(3)>) or if the server reports a failure. It is > > +safe for a mid-command callback to ignore non-zero C<error>: all the > > +other parameters to the mid-command callback will still be valid > > +(corresponding to the current portion of the server's reply), and the > > +overall command will still fail (at the completion callback or > > +L<nbd_aio_command_completed(3)> for an asynchronous command, or as the > > +result of the overall synchronous command). Returing C<-1> from a > > +mid-command callback does not prevent that callback from being reached > > +again, if the server sends more mid-command replies that warrant > > +another use of that callback. A mid-command callback may be reached > > +more times than expected if the server is non-compliant. > > + > > +On the other hand, if a completion callback is supplied (only possible > > +with asynchronous commands), it will always be reached exactly once, > > +and the completion callback must not ignore the value of C<error>. In > > +particular, the content of a buffer passed to L<nbd_aio_pread(3)> or > > +L<nbd_aio_pread_structured(3)> is unspecified if C<error> is set on > > +entry to the completion callback. It is recommended that if your code > > +uses automatic command retirement, then the completion function should > > +return C<1> on all control paths, even when handling errors (note that > > +with automatic retirement, assigning into C<error> is pointless as > > +there is no later API to see that value). > > I think we miss a warning here, that if you don't use automatic retirement, > you *must* call nbd_aio_command_completed() to retire the command, > otherwise commands will leak until you close the handle. That's somewhat already there, not shown in the patch, but several lines above: | When the completion callback returns C<1>, the command is | automatically retired (there is no need to call | L<nbd_aio_command_completed(3)>); for any other return value, the command | still needs to be retired. but I'll try to improve it in v2. > > The text should make it clear that the caller need to chose how to use > the library, either use callbacks or use completion checks. > > Can we make this simpler by preventing usage of completion checks > if you define a completion callback? This will eliminate the auto retire > concept - if you define a callback, commands always retires. Sadly, we've made a promise of C API stability, so no, we can't change the semantics. There are existing callers that use the completion callback but intentionally return 0 for further processing at a more convenient point in time (in callbacks, you can't call nbd_* functions, but when you manually clean up with nbd_aio_command_completed, you can) - see fuse/operations.c. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs