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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180201/ac4e5321/attachment.sig>