[PULL,23/28] nbd: always query export list in fixed new style protocol

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>
---
 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(-)

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
> 
>

[adding nbd-list]

On 05/17/2016 03:53 AM, Richard W.M. Jones wrote:
> 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.
>>

> 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).

Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
default name) as its only list entry?

And at some point, nbdkit should probably implement NBD_OPT_GO (which is
a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
qemu to implement that).

In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
try NBD_OPT_LIST.

> 
> 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.

Qemu is not choosing an export name, so much as proving whether any
OTHER errors can be detected (such as export name not present, or TLS
required).  It _should_ be gracefully ignoring NBD_OPT_LIST errors if
such errors don't definitively prove that the export will not succeed,
but I guess this is not happening.  I'll see if I can tweak the qemu
logic to be more forgiving of nbdkit's current behavior.

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

You're right that we may also want to tweak the NBD protocol to make
this interoperability point obvious.

> 
> 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.

Eric, Richard,

On 17 May 2016, at 16:09, Eric Blake <eblake@redhat.com> wrote:

> [adding nbd-list]
> 
> On 05/17/2016 03:53 AM, Richard W.M. Jones wrote:
>> 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.
>>> 
> 
>> 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).

nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
compulsory, even if you support it by returning a nameless export
(or default). Moreover support of export names is compulsory
(even if you have a single fixed one called "" or "default").

This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
supports 'newstyle' negotiation, then we know qemu won't connect
to it as modern qemu only supports servers that support 'fixed newstyle'
I believe.

> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
> default name) as its only list entry?

Or "default".

> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
> qemu to implement that).
> 
> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
> try NBD_OPT_LIST.

Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.

>> 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.
> 
> Qemu is not choosing an export name, so much as proving whether any
> OTHER errors can be detected (such as export name not present, or TLS
> required).  It _should_ be gracefully ignoring NBD_OPT_LIST errors if
> such errors don't definitively prove that the export will not succeed,
> but I guess this is not happening.  I'll see if I can tweak the qemu
> logic to be more forgiving of nbdkit's current behavior.

Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
specification. If a server doesn't implement mandatory parts of
the specification, then bad things will happen.

My interpretation of NBD_OPT_LIST failing would be 'this server
doesn't have anything it can export'.

>> Naturally the protocol document
>> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
>> this case.
> 
> You're right that we may also want to tweak the NBD protocol to make
> this interoperability point obvious.

I can't actually see the issue here. It explains what needs to be
implemented by the server, and that includes NBD_OPT_LIST. Very
happy to add some clarity, but I'm not sure where it's needed.

--
Alex Bligh

On 05/17/2016 09:22 AM, Alex Bligh wrote:

>>> 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).
> 
> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
> compulsory, even if you support it by returning a nameless export
> (or default). Moreover support of export names is compulsory
> (even if you have a single fixed one called "" or "default").

Where does the protocol state that? I don't see any mandatory NBD_OPT in
the protocol, except for NBD_OPT_EXPORT_NAME (as I read it, a server may
reply with NBD_REP_ERR_UNSUP for every other option).  I'm fine if we
WANT to make NBD_OPT_LIST mandatory (and either document under
NBD_OPT_LIST that NBD_REP_ERR_UNSUP cannot be used, or document under
NBD_REP_ERR_UNSUP that it cannot be used with NBD_OPT_LIST), but that
would make the current nbdkit non-conforming; so it might be nicer to
make a change to the protocol document that instead permits current
nbdkit behavior and puts the burden on clients to interoperate when
NBD_OPT_LIST is not supported.

> 
> This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
> supports 'newstyle' negotiation, then we know qemu won't connect
> to it as modern qemu only supports servers that support 'fixed newstyle'
> I believe.

Not quite true. Qemu as a client is perfectly fine connecting with a
plain 'newstyle' server (and not just 'fixed newstyle'); but will limit
itself to JUST NBD_OPT_EXPORT_NAME in that case.  So nbdkit must be
exposing 'fixed newstyle' for qemu to be attempting NBD_OPT_LIST.

> 
>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>> default name) as its only list entry?
> 
> Or "default".

As I read the protocol, I don't see "default" as a permissible name of
the default export, just "".

Also, we currently state that NBD_OPT_LIST has zero or more
NBD_REP_SERVER replies, which means that it is valid for the command to
succeed with NBD_REP_ACK _without_ advertising any exports at all
(rather annoying in that it tells you nothing about whether
NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).  Should we reword
that to require that if NBD_REP_ACK is sent, then at least one
NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
same time where "" is not mandatory if some other name is given)?

> 
>> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
>> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
>> qemu to implement that).
>>
>> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
>> try NBD_OPT_LIST.
> 
> Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.

Hopefully not much longer; we have multiple implementations converging
on interoperable use of it.

> 
>>> 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.
>>
>> Qemu is not choosing an export name, so much as proving whether any
>> OTHER errors can be detected (such as export name not present, or TLS
>> required).  It _should_ be gracefully ignoring NBD_OPT_LIST errors if
>> such errors don't definitively prove that the export will not succeed,
>> but I guess this is not happening.  I'll see if I can tweak the qemu
>> logic to be more forgiving of nbdkit's current behavior.
> 
> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
> specification. If a server doesn't implement mandatory parts of
> the specification, then bad things will happen.

Again, I don't see how the protocol justifies that claim. Maybe it
should, but I'd like to see your reasoning for stating that it is
mandatory that the server support it.

> 
> My interpretation of NBD_OPT_LIST failing would be 'this server
> doesn't have anything it can export'.

Indeed, and that's why qemu as a client is currently dropping the
connection with nbdkit.  But I would also make that interpretation for
NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
so maybe it is worth a note in the protocol how to detect servers that
are exporting exactly one volume and don't care what name you pass, then
tweaking either nbdkit, qemu, or both to comply to that added protocol
wording.

> 
>>> Naturally the protocol document
>>> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
>>> this case.
>>
>> You're right that we may also want to tweak the NBD protocol to make
>> this interoperability point obvious.
> 
> I can't actually see the issue here. It explains what needs to be
> implemented by the server, and that includes NBD_OPT_LIST. Very
> happy to add some clarity, but I'm not sure where it's needed.

I've hinted at it above - either an explicit statement that servers
cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
other exports, then the SHOULD include an NBD_REP_SERVER with name "";
or an explicit statement that if a server rejects NBD_OPT_LIST, then the
client SHOULD assume that any name will work for
NBD_OPT_EXPORT_NAME/NBD_OPT_GO.

On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
> compulsory, even if you support it by returning a nameless export
> (or default). Moreover support of export names is compulsory
> (even if you have a single fixed one called "" or "default").

The protocol doc doesn't mention "default" (assuming you mean that
literal string).  It says:

  A special, "empty", name (i.e., the length field is zero and no name
  is specified), is reserved for a "default" export, to be used in
  cases where explicitly specifying an export name makes no sense.

which seems to indicate only the empty string should be used for this
case.

Unfortunately qemu 2.5 (as a client) treated exportname == "" as
meaning that the old-style protocol should be used.  This is at least
fixed in qemu 2.6, so nbdkit could now answer NBD_OPT_LIST with a
single empty string, although we lose backwards compatibility with
qemu 2.5.

> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
> specification. If a server doesn't implement mandatory parts of
> the specification, then bad things will happen.

It implements it, it's just that there wasn't a way to implement
anything other than returning an error since we accept anything as an
export name.

Rich.

On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> so it might be nicer to
> make a change to the protocol document that instead permits current
> nbdkit behavior and puts the burden on clients to interoperate when
> NBD_OPT_LIST is not supported.

The purpose of nbdkit is to be a server for qemu, to be a replacement
for qemu-nbd in cases where proprietary code cannot be combined with
qemu for copyright/licensing/legal reasons.  So we aim to make sure we
can interoperate with qemu.  No need to change any standards for
nbdkit!  Clarifying standards documents is OK though.

Rich.

On 05/17/2016 09:52 AM, Eric Blake wrote:
>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>> default name) as its only list entry?
>>
>> Or "default".
> 
> As I read the protocol, I don't see "default" as a permissible name of
> the default export, just "".
> 
> Also, we currently state that NBD_OPT_LIST has zero or more
> NBD_REP_SERVER replies, which means that it is valid for the command to
> succeed with NBD_REP_ACK _without_ advertising any exports at all
> (rather annoying in that it tells you nothing about whether
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).  Should we reword
> that to require that if NBD_REP_ACK is sent, then at least one
> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> same time where "" is not mandatory if some other name is given)?

Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
entries and NBD_REP_ACK (for success), and NOT an error code; so it is
implying that there are NO known export names but that the command was
recognized.

>>
>> My interpretation of NBD_OPT_LIST failing would be 'this server
>> doesn't have anything it can export'.
> 
> Indeed, and that's why qemu as a client is currently dropping the
> connection with nbdkit.

Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
replies.

>  But I would also make that interpretation for
> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -

which is the nbdkit behavior

> so maybe it is worth a note in the protocol how to detect servers that
> are exporting exactly one volume and don't care what name you pass, then
> tweaking either nbdkit, qemu, or both to comply to that added protocol
> wording.

On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
>> so it might be nicer to
>> make a change to the protocol document that instead permits current
>> nbdkit behavior and puts the burden on clients to interoperate when
>> NBD_OPT_LIST is not supported.
> 
> The purpose of nbdkit is to be a server for qemu, to be a replacement
> for qemu-nbd in cases where proprietary code cannot be combined with
> qemu for copyright/licensing/legal reasons.  So we aim to make sure we
> can interoperate with qemu.  No need to change any standards for
> nbdkit!  Clarifying standards documents is OK though.

I also noticed that nbdkit forcefully rejects a client that sends more
than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
on the part of the client.  Maybe the protocol should document a
(higher) limit on minimum number of options a client can expect to be
serviced before the server dropping the connection because the client is
wasting the server's time.

On 17 May 2016, at 16:52, Eric Blake <eblake@redhat.com> wrote:

> On 05/17/2016 09:22 AM, Alex Bligh wrote:
> 
>>>> 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).
>> 
>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>> compulsory, even if you support it by returning a nameless export
>> (or default). Moreover support of export names is compulsory
>> (even if you have a single fixed one called "" or "default").
> 
> Where does the protocol state that? I don't see any mandatory NBD_OPT in
> the protocol, except for NBD_OPT_EXPORT_NAME (as I read it, a server may
> reply with NBD_REP_ERR_UNSUP for every other option).  I'm fine if we
> WANT to make NBD_OPT_LIST mandatory (and either document under
> NBD_OPT_LIST that NBD_REP_ERR_UNSUP cannot be used, or document under
> NBD_REP_ERR_UNSUP that it cannot be used with NBD_OPT_LIST), but that
> would make the current nbdkit non-conforming; so it might be nicer to
> make a change to the protocol document that instead permits current
> nbdkit behavior and puts the burden on clients to interoperate when
> NBD_OPT_LIST is not supported.

Under "Option Types" it says:

    NBD_OPT_LIST (3) "Return zero or more NBD_REP_SERVER replies, one
    for each export, followed by NBD_REP_ACK or an error (such as
    NBD_REP_ERR_SHUTDOWN). The server MAY omit entries from this list if
    TLS has not been negotiated, the server is operating in SELECTIVETLS
    mode, and the entry concerned is a TLS-only export."

The language is imperative (if admittedly not RFC2119).

Also

    The server will reply to any option apart from NBD_OPT_EXPORT_NAME
    with reply packets in the following format:

again not in RFC2119 words, but says 'will' not 'MAY'.

I think the burden here is on the server to implement the protocol
as described.

I now accept that the wording should be cleared up :-)

>> This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
>> supports 'newstyle' negotiation, then we know qemu won't connect
>> to it as modern qemu only supports servers that support 'fixed newstyle'
>> I believe.
> 
> Not quite true. Qemu as a client is perfectly fine connecting with a
> plain 'newstyle' server (and not just 'fixed newstyle'); but will limit
> itself to JUST NBD_OPT_EXPORT_NAME in that case.  So nbdkit must be
> exposing 'fixed newstyle' for qemu to be attempting NBD_OPT_LIST.

OK. I think the problem is that if nbdkit supports 'fixed newstyle'
it should really support all of the options. Alternatively it can
support just 'newstyle' and then nothing will send anything bar
NBD_OPT_EXPORT_NAME.

>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>> default name) as its only list entry?
>> 
>> Or "default".
> 
> As I read the protocol, I don't see "default" as a permissible name of
> the default export, just "".

Sorry, I meant that if Richard implements NBD_OPT_LIST he could either
return an export name of "" or "default". Or for that matter "nbdkit"
or "hello world". Just "default" might display better than "". As he's
ignoring whatever comes back in NBD_OPT_EXPORT_NAME it doesn't really
matter what he returns in NBD_OPT_LIST.

> Also, we currently state that NBD_OPT_LIST has zero or more
> NBD_REP_SERVER replies, which means that it is valid for the command to
> succeed with NBD_REP_ACK _without_ advertising any exports at all

Yes. This is a valid way of saying "I have no exports that work".

> (rather annoying in that it tells you nothing about whether
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).

If there are no exports then NBD_OPT_GO can't be expected to work.

>  Should we reword
> that to require that if NBD_REP_ACK is sent, then at least one
> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> same time where "" is not mandatory if some other name is given)?

I don't think so. A server might legitimately serve an empty list to
one IP and a non-empty list to another IP depending on configuration.

>>> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
>>> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
>>> qemu to implement that).
>>> 
>>> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
>>> try NBD_OPT_LIST.
>> 
>> Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.
> 
> Hopefully not much longer; we have multiple implementations converging
> on interoperable use of it.

Indeed.

>> My interpretation of NBD_OPT_LIST failing would be 'this server
>> doesn't have anything it can export'.
> 
> Indeed, and that's why qemu as a client is currently dropping the
> connection with nbdkit.  But I would also make that interpretation for
> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
> so maybe it is worth a note in the protocol how to detect servers that
> are exporting exactly one volume and don't care what name you pass, then
> tweaking either nbdkit, qemu, or both to comply to that added protocol
> wording.

I think the correct thing for nbdkit to do would be to return a single
entry with an arbitrary name.

I can't see much harm in a client being 'nice' if it gets an UNSUP
error, but other errors (e.g. TLSREQD) have to be respected as errors.

> I've hinted at it above - either an explicit statement that servers
> cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
> other exports, then the SHOULD include an NBD_REP_SERVER with name "";
> or an explicit statement that if a server rejects NBD_OPT_LIST, then the
> client SHOULD assume that any name will work for
> NBD_OPT_EXPORT_NAME/NBD_OPT_GO.

I think it's simpler than that. Servers claiming to implement fixed
newstyle in my view need to implement all the options unless the options
are marked as optional. I admit (now) those could be clarified.

--
Alex Bligh

On 17 May 2016, at 16:54, Richard W.M. Jones <rjones@redhat.com> wrote:

> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>> compulsory, even if you support it by returning a nameless export
>> (or default). Moreover support of export names is compulsory
>> (even if you have a single fixed one called "" or "default").
> 
> The protocol doc doesn't mention "default" (assuming you mean that
> literal string).  It says:

As per my message to Eric, I meant use an arbitrary piece of
text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
'default' is not special). It doesn't matter because you ignore
what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
things more readable for clients that did a list.

>> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
>> specification. If a server doesn't implement mandatory parts of
>> the specification, then bad things will happen.
> 
> It implements it, it's just that there wasn't a way to implement
> anything other than returning an error since we accept anything as an
> export name.

In which case you should just return an export name. Any
export name will work. The fact you don't have names for your
exports doesn't mean (in my book) you can error the list
request. You have one export. You don't know what it's name is,
but you should list it.

On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
> On 05/17/2016 09:52 AM, Eric Blake wrote:
> >>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
> >>> default name) as its only list entry?
> >>
> >> Or "default".
> > 
> > As I read the protocol, I don't see "default" as a permissible name of
> > the default export, just "".
> > 
> > Also, we currently state that NBD_OPT_LIST has zero or more
> > NBD_REP_SERVER replies, which means that it is valid for the command to
> > succeed with NBD_REP_ACK _without_ advertising any exports at all
> > (rather annoying in that it tells you nothing about whether
> > NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).  Should we reword
> > that to require that if NBD_REP_ACK is sent, then at least one
> > NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
> > same time where "" is not mandatory if some other name is given)?
> 
> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
> implying that there are NO known export names but that the command was
> recognized.

Bleah you are right.  I misread the code I wrote :-(

We can add a empty string list entry there easily enough.

Rich.

> >>
> >> My interpretation of NBD_OPT_LIST failing would be 'this server
> >> doesn't have anything it can export'.
> > 
> > Indeed, and that's why qemu as a client is currently dropping the
> > connection with nbdkit.
> 
> Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
> replies.
> 
> >  But I would also make that interpretation for
> > NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
> 
> which is the nbdkit behavior
> 
> > so maybe it is worth a note in the protocol how to detect servers that
> > are exporting exactly one volume and don't care what name you pass, then
> > tweaking either nbdkit, qemu, or both to comply to that added protocol
> > wording.
> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> > On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> >> so it might be nicer to
> >> make a change to the protocol document that instead permits current
> >> nbdkit behavior and puts the burden on clients to interoperate when
> >> NBD_OPT_LIST is not supported.
> > 
> > The purpose of nbdkit is to be a server for qemu, to be a replacement
> > for qemu-nbd in cases where proprietary code cannot be combined with
> > qemu for copyright/licensing/legal reasons.  So we aim to make sure we
> > can interoperate with qemu.  No need to change any standards for
> > nbdkit!  Clarifying standards documents is OK though.
> 
> I also noticed that nbdkit forcefully rejects a client that sends more
> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
> on the part of the client.  Maybe the protocol should document a
> (higher) limit on minimum number of options a client can expect to be
> serviced before the server dropping the connection because the client is
> wasting the server's time.

This, as you say, is a brake on clients that try to waste time by
sending infinite numbers of options.  Is there any danger that 32 is
too small?

Rich.

On 05/17/2016 10:22 AM, Alex Bligh wrote:

[replying as I read, so I may seem to change my mind as I go...]

>>
>> As I read the protocol, I don't see "default" as a permissible name of
>> the default export, just "".
> 
> Sorry, I meant that if Richard implements NBD_OPT_LIST he could either
> return an export name of "" or "default". Or for that matter "nbdkit"
> or "hello world". Just "default" might display better than "". As he's
> ignoring whatever comes back in NBD_OPT_EXPORT_NAME it doesn't really
> matter what he returns in NBD_OPT_LIST.

Except that qemu client DOES check that the returned list includes the
name that qemu plans on sending to NBD_OPT_EXPORT_NAME (that is, if the
client wants to connect to "foo", it expects "foo" in the list returned
by NBD_OPT_LIST).

Option 1: I think what I would prefer is a doc patch to the NBD protocol
that explicitly states that returning 0 names followed by NBD_REP_ACK
(current nbdkit behavior) implies that the server doesn't honor names at
all, and will let ANY name work for NBD_OPT_EXPORT_NAME/NBD_OPT_GO; then
patch qemu client to treat 0 names the same as NBD_REP_UNSUP (the server
doesn't honor names, so assume any name will work).

> 
>> Also, we currently state that NBD_OPT_LIST has zero or more
>> NBD_REP_SERVER replies, which means that it is valid for the command to
>> succeed with NBD_REP_ACK _without_ advertising any exports at all
> 
> Yes. This is a valid way of saying "I have no exports that work".

Except that nbdkit is using it as a way of saying "ALL export names
work" - and that's the special case I'm leaning towards documenting, and
fixing qemu client to support.

> 
>> (rather annoying in that it tells you nothing about whether
>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).
> 
> If there are no exports then NBD_OPT_GO can't be expected to work.

Option 2: An alternative solution would be to allow nbdkit to fail
NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
It is the fact that it is returning NBD_REP_ACK with 0 names that is
giving qemu grief.

> 
>>  Should we reword
>> that to require that if NBD_REP_ACK is sent, then at least one
>> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
>> same time where "" is not mandatory if some other name is given)?
> 
> I don't think so. A server might legitimately serve an empty list to
> one IP and a non-empty list to another IP depending on configuration.

Okay, in such a case, returning 0 names to mean NBD_OPT_GO will never
succeed makes sense.  So I'd argue that NBD_OPT_LIST should NOT succeed
in the case where names are ignored, and that nbdkit could be fixed by
merely ALWAYS returning NBD_REP_ERR_UNSUP to NBD_OPT_LIST.

>>> My interpretation of NBD_OPT_LIST failing would be 'this server
>>> doesn't have anything it can export'.
>>
>> Indeed, and that's why qemu as a client is currently dropping the
>> connection with nbdkit.  But I would also make that interpretation for
>> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
>> so maybe it is worth a note in the protocol how to detect servers that
>> are exporting exactly one volume and don't care what name you pass, then
>> tweaking either nbdkit, qemu, or both to comply to that added protocol
>> wording.
> 
> I think the correct thing for nbdkit to do would be to return a single
> entry with an arbitrary name.

Still not ideal, because qemu 2.6 won't connect if the arbitrary name
doesn't match qemu's expectations.  But qemu 2.6 WILL connect if nbdkit
rejects the command instead of succeeding the command with 0 exports (my
Option 2 above).

> 
> I can't see much harm in a client being 'nice' if it gets an UNSUP
> error, but other errors (e.g. TLSREQD) have to be respected as errors.

That's what qemu 2.6 does: UNSUP errors are special cased to continue
negotiation, all other errors are treated as "can't possibly succeed" so
it terminates immediately.  The interoperability problem is that
returning 0 names (or even 1 name but where the arbitrary name doesn't
match qemu's expectation) is ALSO treated as grounds for "my export is
not available, give up".  So the more I type, the more I'm leaning
towards Option 2: nbdkit should reject NBD_OPT_LIST with
NBD_REP_ERR_UNSUP, rather than succeeding it.

> 
>> I've hinted at it above - either an explicit statement that servers
>> cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
>> other exports, then the SHOULD include an NBD_REP_SERVER with name "";
>> or an explicit statement that if a server rejects NBD_OPT_LIST, then the
>> client SHOULD assume that any name will work for
>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO.
> 
> I think it's simpler than that. Servers claiming to implement fixed
> newstyle in my view need to implement all the options unless the options
> are marked as optional. I admit (now) those could be clarified.

But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
_are_ optional.  In fact, I'm arguing that per Option 2, we WANT
NBD_OPT_LIST to be optional for servers that don't care about names.

On 05/17/2016 10:41 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
>>> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
>>>> so it might be nicer to
>>>> make a change to the protocol document that instead permits current
>>>> nbdkit behavior and puts the burden on clients to interoperate when
>>>> NBD_OPT_LIST is not supported.
>>>
>>> The purpose of nbdkit is to be a server for qemu, to be a replacement
>>> for qemu-nbd in cases where proprietary code cannot be combined with
>>> qemu for copyright/licensing/legal reasons.  So we aim to make sure we
>>> can interoperate with qemu.  No need to change any standards for
>>> nbdkit!  Clarifying standards documents is OK though.
>>
>> I also noticed that nbdkit forcefully rejects a client that sends more
>> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
>> on the part of the client.  Maybe the protocol should document a
>> (higher) limit on minimum number of options a client can expect to be
>> serviced before the server dropping the connection because the client is
>> wasting the server's time.
> 
> This, as you say, is a brake on clients that try to waste time by
> sending infinite numbers of options.  Is there any danger that 32 is
> too small?

Yes. Consider a client that connects to a server that lists more than 32
exports in NBD_OPT_LIST, then the client calls (the still-experimental)
NBD_OPT_INFO on each of those exports, to learn further details about
each export, before finally using NBD_OPT_GO to pick the one the user
likes best.

That's why I wonder if we need to document a minimum cutoff at which
clients should assume will always be serviced, and which servers should
not treat as an attack, and whether it should be larger than 32.

On 05/17/2016 10:39 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:52 AM, Eric Blake wrote:
>>>>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>>>>> default name) as its only list entry?
>>>>
>>>> Or "default".
>>>
>>> As I read the protocol, I don't see "default" as a permissible name of
>>> the default export, just "".
>>>
>>> Also, we currently state that NBD_OPT_LIST has zero or more
>>> NBD_REP_SERVER replies, which means that it is valid for the command to
>>> succeed with NBD_REP_ACK _without_ advertising any exports at all
>>> (rather annoying in that it tells you nothing about whether
>>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).  Should we reword
>>> that to require that if NBD_REP_ACK is sent, then at least one
>>> NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
>>> same time where "" is not mandatory if some other name is given)?
>>
>> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
>> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
>> implying that there are NO known export names but that the command was
>> recognized.
> 
> Bleah you are right.  I misread the code I wrote :-(
> 
> We can add a empty string list entry there easily enough.

Will adding the empty string break qemu 2.5 connections?  qemu 2.6  is
picky in that if the name it is requesting is not in the advertised
list, it gives up; but if there is NO advertised list, it assumes the
name will just work.  Since nbdkit has the latter behavior, I'm thinking
that nbdkit returning NBD_REP_ERR_UNSUP instead of NBD_REP_ACK is a
smarter course of action.

On 05/17/2016 10:26 AM, Alex Bligh wrote:
> 
> On 17 May 2016, at 16:54, Richard W.M. Jones <rjones@redhat.com> wrote:
> 
>> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>>> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
>>> compulsory, even if you support it by returning a nameless export
>>> (or default). Moreover support of export names is compulsory
>>> (even if you have a single fixed one called "" or "default").
>>
>> The protocol doc doesn't mention "default" (assuming you mean that
>> literal string).  It says:
> 
> As per my message to Eric, I meant use an arbitrary piece of
> text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
> 'default' is not special). It doesn't matter because you ignore
> what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
> things more readable for clients that did a list.
> 
>>> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
>>> specification. If a server doesn't implement mandatory parts of
>>> the specification, then bad things will happen.
>>
>> It implements it, it's just that there wasn't a way to implement
>> anything other than returning an error since we accept anything as an
>> export name.
> 
> In which case you should just return an export name. Any
> export name will work. The fact you don't have names for your
> exports doesn't mean (in my book) you can error the list
> request. You have one export. You don't know what it's name is,
> but you should list it.

Except that the qemu 2.6 client is specifically checking that the name
it is looking for is present in the list, and gives up trying if the
name was not found. And there is no way for the server to know a priori
which name the client is trying to match.  An arbitrary name is less
likely to be found; the empty name "" is the most likely arbitrary name,
but even then, there is no guarantee that the qemu client is using the
"" default name, and since NBD_OPT_EXPORT_NAME doesn't care, I think
it's nicer to state that NBD_OPT_LIST should ONLY be supported if names
matter to NBD_OPT_EXPORT_NAME (and should return NBD_REP_ERR_UNSUP when
names don't matter).

On 17 May 2016, at 17:50, Eric Blake <eblake@redhat.com> wrote:
> 
> Option 1: I think what I would prefer is a doc patch to the NBD protocol
> that explicitly states that returning 0 names followed by NBD_REP_ACK
> (current nbdkit behavior) implies that the server doesn't honor names at
> all, and will let ANY name work for NBD_OPT_EXPORT_NAME/NBD_OPT_GO; then
> patch qemu client to treat 0 names the same as NBD_REP_UNSUP (the server
> doesn't honor names, so assume any name will work).
...
> But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
> _are_ optional.  In fact, I'm arguing that per Option 2, we WANT
> NBD_OPT_LIST to be optional for servers that don't care about names.

Option 3: (which I would prefer) fix the wording indicating what
options are mandatory (I think they all should be but that's another
discussion really), and in the meantime simply have nbdkit return
the empty string as an export name in NBD_OPT_LIST as you
originally suggested, which is what qemu will connect to by default,
and which is what other clients connect to by default.

I don't want to be an arse about this, but I think the standard is
meant to be be normative, IE influence behaviour, rather than just
document existing behaviour. In this instance I think nbdkit
is actually wrong (returning zero entries cannot be right as
that explicitly says nothing is exported, whereas an error is
forgiveable perhaps), and the author is OK to fix it, so I think
we should fix the software to confirm to the standard, not the
standard to conform to the software.

--
Alex Bligh

On 17 May 2016, at 17:56, Eric Blake <eblake@redhat.com> wrote:

> That's why I wonder if we need to document a minimum cutoff at which
> clients should assume will always be serviced, and which servers should
> not treat as an attack, and whether it should be larger than 32.

I don't think we need a specific number, but I think a one sentence
statement that servers are in general permitted to disconnect clients
which are behaving in a denial of service attack type manner would be
useful.

Given nbdkit only ever has one export, it seems eminently reasonable
for it to use a lower number.

--
Alex Bligh

Just to close this thread out, I have implemented the exportname in
qemu.  NBD_OPT_LIST returns the exportname.  nbdkit now interoperates
with qemu 2.5 and 2.6.

Rich.

On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> Option 2: An alternative solution would be to allow nbdkit to fail
> NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
> should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
> It is the fact that it is returning NBD_REP_ACK with 0 names that is
> giving qemu grief.

I think this makes most sense. If you don't look at export names, you
effectively don't support them, and you can't be expected to send a list
of "supported" names, because *everything* is supported (or, put a
different way, nothing is).

I note that nbdkit has been patched to now send the empty name, which
is also fine as a way to fix interoperability in this particular case --
but for the general case, I think if we want to define ways for a server
to explicitly say that it doesn't support export names, returning
ERR_UNSUP to OPT_LIST seems cleaner.

(That does mean I'll have to fix nbd-client, since it currently assumes
that fixed newstyle implies support for OPT_LIST -- but that should be
an easy enough patch)

> But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
> _are_ optional.  In fact, I'm arguing that per Option 2, we WANT
> NBD_OPT_LIST to be optional for servers that don't care about names.

Sortof. Like I said, nbd-client assumes that servers which support fixed
newstyle will also support OPT_LIST -- but that's mostly an artefact of
the fact that OPT_LIST was used as a test case for fixed newstyle, and
isn't necessarily a good enough reason to make that a required part of
the protocol.

On Sat, May 21, 2016 at 11:53:12PM +0200, Wouter Verhelst wrote:
> On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> > Option 2: An alternative solution would be to allow nbdkit to fail
> > NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
> > should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
> > It is the fact that it is returning NBD_REP_ACK with 0 names that is
> > giving qemu grief.
> 
> I think this makes most sense. If you don't look at export names, you
> effectively don't support them, and you can't be expected to send a list
> of "supported" names, because *everything* is supported (or, put a
> different way, nothing is).
> 
> I note that nbdkit has been patched to now send the empty name, which
> is also fine as a way to fix interoperability in this particular case --

nbdkit now fully supports export names via the '-e' option.
This discussion is moot.

Rich.