[Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics

[Richard W.M. Jones]

On Wed, Jan 31, 2018 at 09:26:37PM -0600, Eric Blake wrote:
> Previously, we let a plugin set an error in either thread-local
> storage (nbdkit_set_error()) or errno, then connections.c would
> decode which error to use.  But with filters in the mix, it is
> very difficult for a filter to know what error was set by the
> plugin (particularly since nbdkit_set_error() has no public
> counterpart for reading the thread-local storage).  What's more,
> if a filter does any non-trivial processing after calling into
> next_ops, it is very probable that errno might be corrupted,
> which would affect the error returned by a plugin that relied
> on errno but not the error stored in thread-local storage.
> 
> Better is to change the backend interface to just pass the
> direct error value, by moving the decoding of thread-local vs.
> errno into plugins.c.  With the change in decoding location,
> the backend interface no longer needs an .errno_is_preserved()
> callback.
> 
> For maximum convenience, this change lets a filter return an
> error either by passing through the underlying plugin return
> (a positive error) or by setting -1 and storing something in
> errno.  However, I did have to tweak some of the existing
> filters to actually handle and/or return the right error; which
> means this IS a subtle change to the filter interface (and
> worse, one that cannot be detected by the compiler because
> the API signatures did not change).  However, more ABI changes
> are planned with adding FUA support, so as long as it is all
> done as part of the same release, we are okay.
> 
> Also note that only nbdkit-plugin.h declares nbdkit_set_error();
> we can actually keep it this way (filters do not need to call
> that function).
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  docs/nbdkit-filter.pod        | 83 +++++++++++++++++++++++++++++++++++++------
>  src/internal.h                |  1 -
>  src/connections.c             | 45 ++++++-----------------
>  src/filters.c                 | 81 +++++++++++++++++++++++++----------------
>  src/plugins.c                 | 66 +++++++++++++++++++---------------
>  filters/cache/cache.c         | 49 +++++++++++++++----------
>  filters/cow/cow.c             | 28 +++++++++------
>  filters/partition/partition.c |  2 +-
>  8 files changed, 221 insertions(+), 134 deletions(-)
> 
> diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
> index eb72dae..4ddf25d 100644
> --- a/docs/nbdkit-filter.pod
> +++ b/docs/nbdkit-filter.pod
> @@ -163,10 +163,14 @@ short-circuited.
> 
>  The filter’s other methods like C<.prepare>, C<.get_size>, C<.pread>
>  etc ― always called in the context of a connection ― are passed a
> -pointer to C<struct nbdkit_next_ops> which contains a subset of the
> -plugin methods that can be called during a connection.  It is possible
> -for a filter to issue (for example) extra read calls in response to a
> -single C<.pwrite> call.
> +pointer to C<struct nbdkit_next_ops> which contains a comparable set
> +of accessors to plugin methods that can be called during a connection.
> +It is possible for a filter to issue (for example) extra read calls in
> +response to a single C<.pwrite> call.  Note that the semantics of the
> +functions in C<struct nbdkit_next_ops> are slightly different from
> +what a plugin implements: for example, while a plugin's C<.pread>
> +returns -1 on error, C<next_ops->pread> returns a positive errno

I believe you should write this: C<next_ops-E<gt>pread>
Similarly in the rest of the document.


Looking a the patch as a whole, if I'm understanding it correctly, the
functions like backend.pread will now always return 0 or positive
errno?  Or can they return -1 too?

Would this patch have been simpler if we'd just added the
nbdkit_get_errno() function to the public API (which is what I thought
we were going to do).  In that case the filters could check for the
errno by doing:

  if (next_ops->pread (...) == -1) {
    /* If I need to know the errno, then ... */
    int err = nbdkit_get_errno ();
    ...
  }

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top