[Libguestfs] [libnbd PATCH] docs: Clarify how callbacks should handle errors

[Eric Blake]

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