[Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
Richard W.M. Jones
rjones at redhat.com
Thu Feb 1 14:50:51 UTC 2018
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
More information about the Libguestfs
mailing list