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

[Eric Blake]

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>