Eric Blake
2018-Feb-01 23:27 UTC
Re: [Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
On 01/31/2018 09:26 PM, Eric Blake wrote:> 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.For filters, this makes sense (all filters are in-tree, so we can audit that the return values follow conventions). But for plugins, we're better off being conservative:> +++ b/src/connections.c> @@ -942,49 +931,36 @@ handle_request (struct connection *conn, > uint32_t f = 0; > bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA); > > - /* The plugin should call nbdkit_set_error() to request a particular > - error, otherwise we fallback to errno or EIO. */ > + /* Clear the error, so that we know if the plugin calls > + nbdkit_set_error() or relied on errno. */ > threadlocal_set_error (0); > > switch (cmd) { > case NBD_CMD_READ: > - if (backend->pread (backend, conn, buf, count, offset, 0) == -1) > - return get_error (conn); > - break; > + return backend->pread (backend, conn, buf, count, offset, 0);...> - default: > - abort (); > + return backend->zero (backend, conn, count, offset, f); > } > - > - return 0; > + /* Unreachable */ > + abort (); > }Prior to the patch, only an explicit -1 value return from the plugin would trigger returning an error code to the client; all other values (whether -2, 0, or positive, even though the documentation only mentioned 0 as valid) would treat things as success on the reply to the client.> +++ b/src/plugins.c> static int > plugin_pread (struct backend *b, struct connection *conn, > void *buf, uint32_t count, uint64_t offset, uint32_t flags) > { > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > + int r; > > assert (connection_get_handle (conn, 0)); > assert (p->plugin.pread != NULL); > @@ -370,25 +375,29 @@ plugin_pread (struct backend *b, struct connection *conn, > > debug ("pread count=%" PRIu32 " offset=%" PRIu64, count, offset); > > - return p->plugin.pread (connection_get_handle (conn, 0), buf, count, offset); > + r = p->plugin.pread (connection_get_handle (conn, 0), buf, count, offset); > + if (r < 0) > + r = get_error (p); > + return r;But in the new code, I'm blindly returning the plugin's value (even if positive), which may break an out-of-tree plugin that used to return positive on success in spite of the documentation. I'll fix this for v3 to be more careful. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2018-Feb-06 07:05 UTC
Re: [Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
I had a think about this. How about (for the internal backend interface only) returning an extra ‘int *err’ parameter? int (*pread) (struct backend *, struct connection *conn, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err); This has some advantages: - It's explicit. - It means we preserve the convention that -1 = error used everywhere else in nbdkit. - Better for APIs that actually return something like get_size. - Can be both read and written. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Eric Blake
2018-Feb-06 14:42 UTC
Re: [Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
On 02/06/2018 01:05 AM, Richard W.M. Jones wrote:> > I had a think about this. How about (for the internal backend > interface only) returning an extra ‘int *err’ parameter?For backend only, or for backend and filters? The biggest reason for the change is so that filters can inspect/modify the error to push over the wire to the client. If for the filters, would it be for both the callbacks and the next_ops functions, or just the next_ops functions?> > int (*pread) (struct backend *, struct connection *conn, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err); > > This has some advantages: > > - It's explicit. > > - It means we preserve the convention that -1 = error > used everywhere else in nbdkit. > > - Better for APIs that actually return something like get_size. > > - Can be both read and written.Seems like it might be reasonable enough, even though it's a bit more verbose; I can respin my series along those lines for comparison. Note that get_size() doesn't send an error over the wire to the client, so there, accessing errno in the filter doesn't buy much, and the filter really has no need to modify errno if failing its own .get_size callback. It's only the transmission functions (.pread, .pwrite, .flush, .trim, .zero) where getting at the error value matters. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Maybe Matching Threads
- Re: [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
- Re: [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
- Re: [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
- [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
- [PATCH nbdkit filters-v2 5/5] INCOMPLETE filters: Add nbdkit-partition-filter.