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
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)