[Libguestfs] [nbdkit PATCH v2 1/3] backend: Rework internal/filter error return semantics
Eric Blake
eblake at redhat.com
Thu Feb 1 23:27:05 UTC 2018
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
-------------- 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/6a106cbd/attachment.sig>
More information about the Libguestfs
mailing list