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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
