Re: [Qemu-devel] [PULL 23/28] nbd: always query export list in fixed new style protocol

["Richard W.M. Jones" <rjones@redhat.com>]

On Tue, Feb 16, 2016 at 05:34:41PM +0100, Paolo Bonzini wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> With the new style protocol, the NBD client will currenetly
> send NBD_OPT_EXPORT_NAME as the first (and indeed only)
> option it wants. The problem is that the NBD protocol spec
> does not allow for returning an error message with the
> NBD_OPT_EXPORT_NAME option. So if the server mandates use
> of TLS, the client will simply see an immediate connection
> close after issuing NBD_OPT_EXPORT_NAME which is not user
> friendly.
> 
> To improve this situation, if we have the fixed new style
> protocol, we can sent NBD_OPT_LIST as the first option
> to query the list of server exports. We can check for our
> named export in this list and raise an error if it is not
> found, instead of going ahead and sending NBD_OPT_EXPORT_NAME
> with a name that we know will be rejected.
> 
> This improves the error reporting both in the case that the
> server required TLS, and in the case that the client requested
> export name does not exist on the server.
> 
> If the server does not support NBD_OPT_LIST, we just ignore
> that and carry on with NBD_OPT_EXPORT_NAME as before.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Message-Id: <1455129674-17255-12-git-send-email-berrange@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I just bisected qemu 2.6 to find out where it breaks interop with
nbdkit, and it is this commit.

nbdkit implements the newstyle protocol, but doesn't implement export
names (yet, maybe in future).  It processes the NBD_OPT_EXPORT_NAME
option, but ignores the export name and carries on regardless.

nbdkit's implemention of NBD_OPT_LIST returns an error, because there
is no such thing as a list of export names supported (in effect nbdkit
allows any export name).

Therefore I don't believe the assumption made here -- that you can
list export names and choose them on the client side -- is a valid
one.

Naturally the protocol document
(https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
this case.

To test qemu against nbdkit you can do this in the nbdkit sources:

  make
  make check TESTS=test-newstyle \
    LIBGUESTFS_HV=/path/to/qemu/x86_64-softmmu/qemu-system-x86_64 \
    LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1

Rich.

>  nbd/client.c               | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>  nbd/server.c               |   2 +
>  tests/qemu-iotests/140.out |   2 +-
>  tests/qemu-iotests/143.out |   2 +-
>  4 files changed, 196 insertions(+), 5 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 88f2ada..be5f08d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -71,6 +71,177 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
>  
>  */
>  
> +
> +static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
> +{
> +    if (!(type & (1 << 31))) {
> +        return 0;
> +    }
> +
> +    switch (type) {
> +    case NBD_REP_ERR_UNSUP:
> +        error_setg(errp, "Unsupported option type %x", opt);
> +        break;
> +
> +    case NBD_REP_ERR_INVALID:
> +        error_setg(errp, "Invalid data length for option %x", opt);
> +        break;
> +
> +    default:
> +        error_setg(errp, "Unknown error code when asking for option %x", opt);
> +        break;
> +    }
> +
> +    return -1;
> +}
> +
> +static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
> +{
> +    uint64_t magic;
> +    uint32_t opt;
> +    uint32_t type;
> +    uint32_t len;
> +    uint32_t namelen;
> +
> +    *name = NULL;
> +    if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
> +        error_setg(errp, "failed to read list option magic");
> +        return -1;
> +    }
> +    magic = be64_to_cpu(magic);
> +    if (magic != NBD_REP_MAGIC) {
> +        error_setg(errp, "Unexpected option list magic");
> +        return -1;
> +    }
> +    if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
> +        error_setg(errp, "failed to read list option");
> +        return -1;
> +    }
> +    opt = be32_to_cpu(opt);
> +    if (opt != NBD_OPT_LIST) {
> +        error_setg(errp, "Unexpected option type %x expected %x",
> +                   opt, NBD_OPT_LIST);
> +        return -1;
> +    }
> +
> +    if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
> +        error_setg(errp, "failed to read list option type");
> +        return -1;
> +    }
> +    type = be32_to_cpu(type);
> +    if (type == NBD_REP_ERR_UNSUP) {
> +        return 0;
> +    }
> +    if (nbd_handle_reply_err(opt, type, errp) < 0) {
> +        return -1;
> +    }
> +
> +    if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) {
> +        error_setg(errp, "failed to read option length");
> +        return -1;
> +    }
> +    len = be32_to_cpu(len);
> +
> +    if (type == NBD_REP_ACK) {
> +        if (len != 0) {
> +            error_setg(errp, "length too long for option end");
> +            return -1;
> +        }
> +    } else if (type == NBD_REP_SERVER) {
> +        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> +            error_setg(errp, "failed to read option name length");
> +            return -1;
> +        }
> +        namelen = be32_to_cpu(namelen);
> +        if (len != (namelen + sizeof(namelen))) {
> +            error_setg(errp, "incorrect option mame length");
> +            return -1;
> +        }
> +        if (namelen > 255) {
> +            error_setg(errp, "export name length too long %d", namelen);
> +            return -1;
> +        }
> +
> +        *name = g_new0(char, namelen + 1);
> +        if (read_sync(ioc, *name, namelen) != namelen) {
> +            error_setg(errp, "failed to read export name");
> +            g_free(*name);
> +            *name = NULL;
> +            return -1;
> +        }
> +        (*name)[namelen] = '\0';
> +    } else {
> +        error_setg(errp, "Unexpected reply type %x expected %x",
> +                   type, NBD_REP_SERVER);
> +        return -1;
> +    }
> +    return 1;
> +}
> +
> +
> +static int nbd_receive_query_exports(QIOChannel *ioc,
> +                                     const char *wantname,
> +                                     Error **errp)
> +{
> +    uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
> +    uint32_t opt = cpu_to_be32(NBD_OPT_LIST);
> +    uint32_t length = 0;
> +    bool foundExport = false;
> +
> +    TRACE("Querying export list");
> +    if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
> +        error_setg(errp, "Failed to send list option magic");
> +        return -1;
> +    }
> +
> +    if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
> +        error_setg(errp, "Failed to send list option number");
> +        return -1;
> +    }
> +
> +    if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
> +        error_setg(errp, "Failed to send list option length");
> +        return -1;
> +    }
> +
> +    TRACE("Reading available export names");
> +    while (1) {
> +        char *name = NULL;
> +        int ret = nbd_receive_list(ioc, &name, errp);
> +
> +        if (ret < 0) {
> +            g_free(name);
> +            name = NULL;
> +            return -1;
> +        }
> +        if (ret == 0) {
> +            /* Server doesn't support export listing, so
> +             * we will just assume an export with our
> +             * wanted name exists */
> +            foundExport = true;
> +            break;
> +        }
> +        if (name == NULL) {
> +            TRACE("End of export name list");
> +            break;
> +        }
> +        if (g_str_equal(name, wantname)) {
> +            foundExport = true;
> +            TRACE("Found desired export name '%s'", name);
> +        } else {
> +            TRACE("Ignored export name '%s'", name);
> +        }
> +        g_free(name);
> +    }
> +
> +    if (!foundExport) {
> +        error_setg(errp, "No export with name '%s' available", wantname);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
>                            off_t *size, Error **errp)
>  {
> @@ -121,28 +292,44 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
>          uint32_t namesize;
>          uint16_t globalflags;
>          uint16_t exportflags;
> +        bool fixedNewStyle = false;
>  
>          if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
>              sizeof(globalflags)) {
>              error_setg(errp, "Failed to read server flags");
>              goto fail;
>          }
> -        *flags = be16_to_cpu(globalflags) << 16;
> +        globalflags = be16_to_cpu(globalflags);
> +        *flags = globalflags << 16;
> +        TRACE("Global flags are %x", globalflags);
>          if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
> +            fixedNewStyle = true;
>              TRACE("Server supports fixed new style");
>              clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
>          }
>          /* client requested flags */
> +        clientflags = cpu_to_be32(clientflags);
>          if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
>              sizeof(clientflags)) {
>              error_setg(errp, "Failed to send clientflags field");
>              goto fail;
>          }
> -        /* write the export name */
>          if (!name) {
>              error_setg(errp, "Server requires an export name");
>              goto fail;
>          }
> +        if (fixedNewStyle) {
> +            /* Check our desired export is present in the
> +             * server export list. Since NBD_OPT_EXPORT_NAME
> +             * cannot return an error message, running this
> +             * query gives us good error reporting if the
> +             * server required TLS
> +             */
> +            if (nbd_receive_query_exports(ioc, name, errp) < 0) {
> +                goto fail;
> +            }
> +        }
> +        /* write the export name */
>          magic = cpu_to_be64(magic);
>          if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
>              error_setg(errp, "Failed to send export name magic");
> @@ -176,7 +363,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
>              error_setg(errp, "Failed to read export flags");
>              goto fail;
>          }
> -        *flags |= be16_to_cpu(exportflags);
> +        exportflags = be16_to_cpu(exportflags);
> +        *flags |= exportflags;
> +        TRACE("Export flags are %x", exportflags);
>      } else if (magic == NBD_CLIENT_MAGIC) {
>          if (name) {
>              error_setg(errp, "Server does not support export names");
> diff --git a/nbd/server.c b/nbd/server.c
> index 074a1e6..3d2fb10 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -294,6 +294,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
>      }
>      name[length] = '\0';
>  
> +    TRACE("Client requested export '%s'", name);
> +
>      client->exp = nbd_export_find(name);
>      if (!client->exp) {
>          LOG("export not found");
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> index fdedeb3..72f1b4c 100644
> --- a/tests/qemu-iotests/140.out
> +++ b/tests/qemu-iotests/140.out
> @@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
>  {"return": {}}
> -can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
>  no file open, try 'help open'
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
> index dad2024..d24ad20 100644
> --- a/tests/qemu-iotests/143.out
> +++ b/tests/qemu-iotests/143.out
> @@ -1,7 +1,7 @@
>  QA output created by 143
>  {"return": {}}
>  {"return": {}}
> -can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length
> +can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: No export with name 'no_such_export' available
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
>  *** done
> -- 
> 2.5.0
> 
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html