9:14 a.m.
On 02/01/2018 08:50 AM, Richard W.M. Jones wrote:
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).
>
> 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.
Oh, indeed, so that it doesn't interfere with detecting the ending >.
(Can you tell that I haven't done much pod documentation before, and
that I'm just copy-and-pasting concepts?)
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?
The patch asserts that backend.pread always returns 0 or positive. Look
at connections.c; handle_request() already has to return 0 or positive,
because recv_request_send_reply() then takes that return value and
shoves it over the wire to the NBD client. The plugins or filters can
still return -1 instead of a positive error, but plugins.c/filters.c
then converts that to the desired positive value for the next layer in
the stack.
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 ();
...
I originally tried that, but no, it was not simpler. Consider this
potential filter implementation (which rather resembles my log filter in
patch 2/3):
int r = next_ops->pread ();
printf ("Logging something");
return r;
Oops - the call to printf() may have changed the value of errno.
Returning -1 means that if next_ops->pread() called nbdkit_set_error(),
we use THAT error, but if next_ops->pread() relied on returning -1 with
errno preserved, we've clobbered the error that we wanted to report to
the end user.
Making the filter user call nbdkit_get_error(), in order to then either
re-call nbdkit_set_error() or set errno, is a lot more boilerplate,
compared to having the filter just return the existing positive value
unchanged.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org