[Libguestfs] [libnbd PATCH v2 3/5] docs: Clarify how callbacks should handle errors

Thu Feb 3 22:59:07 UTC 2022

()On Thu, Feb 3, 2022 at 10:28 PM Eric Blake <eblake at redhat.com> 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";
> --
> 2.34.1

Looks good.

Nir