On 02/03/22 21:25, Eric Blake wrote: > Recent patches have demonstrated confusion on the order in which > callbacks are reached, when it is safe or dangerous to ignore *error, > and what a completion callback should do when auto-retirement is in > use. Add wording to make it more obvious that: > > - callbacks are reached in the following order: mid-command callback > (0, 1, or many times, if supplied), completion callback (exactly > once, if supplied), mid-command free (exactly once, if supplied), > completion free (exactly once, if supplied) > - returning -1 from a mid-command callback does does not prevent > future callbacks > - ignoring *error in a mid-command callback is safe > - completion callbacks are reached unconditionally, and must NOT ignore > *error > - if the user chooses to use auto-retirement instead of manual calls to > nbd_aio_command_completed, the completion callback should return 1 even > on error cases to avoid complicating command cleanup > - the contents of buf after nbd_pread and friends is undefined on > error (at present, if the user did not pre-initialize the buffer, > there are some code paths in libnbd that leave it uninitialized) > --- > docs/libnbd.pod | 69 +++++++++++++++++++++++++++++++++++++----------- > generator/API.ml | 24 ++++++++++++----- > 2 files changed, 71 insertions(+), 22 deletions(-) > > diff --git a/docs/libnbd.pod b/docs/libnbd.pod > index eb8038b0..15bdf0a8 100644 > --- a/docs/libnbd.pod > +++ b/docs/libnbd.pod > @@ -829,8 +829,12 @@ This can be used to free associated C<user_data>. For > example: > NBD_NULL_COMPLETION, > 0); > > -will call L<free(3)> on C<my_data> after the last time that the > -S<C<chunk.callback = my_fn>> function is called. > +will call L<free(3)> once on C<my_data> after the point where it is > +known that the S<C<chunk.callback = my_fn>> function can no longer be > +called, regardless of how many times C<my_fn> was actually called. If > +both a mid-command and completion callback are supplied, the functions > +will be reached in this order: mid-function callbacks, completion > +callback, mid-function free, and finally completion free. > > The free function is only accessible in the C API as it is not needed > in garbage collected programming languages. > @@ -858,27 +862,60 @@ same nbd object, as it would cause deadlock. > > =head2 Completion callbacks > > -All of the low-level commands have a completion callback variant that > -registers a callback function used right before the command is marked > -complete. > +All of the asychronous commands have an optional completion callback > +function that is used right before the command is marked complete, > +after any mid-command callbacks have finished, and before any free > +functions. > > 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. > +L<nbd_aio_command_completed(3)>); for any other return value, the > +command still needs to be manually retired (otherwise, the command > +will tie up resources until L<nbd_close(3)> is eventually reached). > > =head2 Callbacks with C<int *error> parameter > > 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 > -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> which is a pointer containing the value of any error detected > +so far. If a 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> in the > +C API. Assignments into C<error> are ignored for any other return > +value; similarly, assigning C<0> into C<error> does not have an > +effect. In other language bindings, reporting callback errors is > +generally done by raising an exception rather than by return value. > + > +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 that > +concludes the command. 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 pointed to by > +C<error>. In particular, the content of a buffer passed to > +L<nbd_aio_pread(3)> or L<nbd_aio_pread_structured(3)> is undefined > +if C<*error> is non-zero on entry to the completion callback. It is > +recommended that if you choose to use automatic command retirement > +(where the completion callback returns C<1> to avoid needing to call > +L<nbd_aio_command_completed(3)> later), your 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). > > =head1 COMPILING YOUR PROGRAM > > diff --git a/generator/API.ml b/generator/API.ml > index cf2e7543..012016bc 100644 > --- a/generator/API.ml > +++ b/generator/API.ml > @@ -1,6 +1,6 @@ > (* hey emacs, this is OCaml code: -*- tuareg -*- *) > (* nbd client library in userspace: the API > - * Copyright (C) 2013-2021 Red Hat Inc. > + * Copyright (C) 2013-2022 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -1829,7 +1829,9 @@ "pread", { > L<nbd_get_block_size(3)>. > > The C<flags> parameter must be C<0> for now (it exists for future NBD > -protocol extensions)." > +protocol extensions). > + > +Note that if this command fails, the contents of C<buf> are undefined." > ^ strict_call_description; > see_also = [Link "aio_pread"; Link "pread_structured"; > Link "get_block_size"; Link "set_strict_mode"]; > @@ -1914,7 +1916,9 @@ "pread_structured", { > C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with > more than one fragment (if that is supported - some servers cannot do > this, see L<nbd_can_df(3)>). Libnbd does not validate that the server > -actually obeys the flag." > +actually obeys the flag. > + > +Note that if this command fails, the contents of C<buf> are undefined." > ^ strict_call_description; > see_also = [Link "can_df"; Link "pread"; > Link "aio_pread_structured"; Link "get_block_size"; > @@ -2155,7 +2159,8 @@ "block_status", { > for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains constants > beginning with C<LIBNBD_STATE_> that may help decipher the values. > On entry to the callback, the C<error> parameter contains the errno > -value of any previously detected error. > +value of any previously detected error, but even if an earlier error > +was detected, the current C<metacontext> and C<entries> are valid. > > It is possible for the extent function to be called > more times than you expect (if the server is buggy), > @@ -2454,7 +2459,10 @@ "aio_pread", { > as described in L<libnbd(3)/Completion callbacks>. > > Note that you must ensure C<buf> is valid until the command has > -completed. Other parameters behave as documented in L<nbd_pread(3)>." > +completed. Furthermore, the contents of C<buf> are undefined if the > +C<error> parameter to C<completion_callback> is set, or if > +L<nbd_aio_command_completed(3)> reports failure. Other parameters behave > +as documented in L<nbd_pread(3)>." > ^ strict_call_description; > example = Some "examples/aio-connect-read.c"; > see_also = [SectionLink "Issuing asynchronous commands"; > @@ -2478,7 +2486,11 @@ "aio_pread_structured", { > Or supply the optional C<completion_callback> which will be invoked > as described in L<libnbd(3)/Completion callbacks>. > > -Other parameters behave as documented in L<nbd_pread_structured(3)>." > +Note that you must ensure C<buf> is valid until the command has > +completed. Furthermore, the contents of C<buf> are undefined if the > +C<error> parameter to C<completion_callback> is set, or if > +L<nbd_aio_command_completed(3)> reports failure. Other parameters behave > +as documented in L<nbd_pread_structured(3)>." > ^ strict_call_description; > see_also = [SectionLink "Issuing asynchronous commands"; > Link "aio_pread"; Link "pread_structured"; >
Great! Reviewed-by: Laszlo Ersek <ler...@redhat.com> _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs