Re: [Qemu-devel] [PATCH 1/3] nbd: Handle fixed new-style clients.

Re: [Qemu-devel] [PATCH 1/3] nbd: Handle fixed new-style clients.

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 10:39:40PM +0100, Hani Benhabiles wrote:
> Signed-off-by: Hani Benhabiles <hani@linux.com>
> ---
>  include/block/nbd.h |  6 ++++++
>  nbd.c               | 12 +++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)

No explanation or link to specification for this new flag field?  What's
different about a new-style client?

Stefan

Re: [Qemu-devel] [PATCH 1/3] nbd: Handle fixed new-style clients.

[Hani Benhabiles]

On Mon, Jun 02, 2014 at 02:32:06PM +0200, Stefan Hajnoczi wrote:
> On Sat, May 31, 2014 at 10:39:40PM +0100, Hani Benhabiles wrote:
> > Signed-off-by: Hani Benhabiles <hani@linux.com>
> > ---
> >  include/block/nbd.h |  6 ++++++
> >  nbd.c               | 12 +++++++-----
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> No explanation or link to specification for this new flag field?  What's
> different about a new-style client?

With this flag is set, the server tells the client that it can send another
option if the server got a request with an option it doesn't understand (instead
of the server closing the connection.) Thus, the while(1) loop in 2/3.

The kernel in Documentation/blockdev/nbd.txt points to the NBD project for
documentation. The proto documentation is in [1]. Shouldn't Qemu also do the
same ?

[1] https://github.com/yoe/nbd/blob/master/doc/proto.txt

Re: [Qemu-devel] [PATCH 3/3] nbd: Shutdown socket before closing.

[Paolo Bonzini]

Il 31/05/2014 23:39, Hani Benhabiles ha scritto:
> This forces finishing data sending to client before closing the socket like in
> exports listing or replying with NBD_REP_ERR_UNSUP cases.
>
> Signed-off-by: Hani Benhabiles <hani@linux.com>
> ---
>  blockdev-nbd.c | 1 +
>  qemu-nbd.c     | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index b60b66d..e609f66 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -28,6 +28,7 @@ static void nbd_accept(void *opaque)
>
>      int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
>      if (fd >= 0 && !nbd_client_new(NULL, fd, nbd_client_put)) {
> +        shutdown(fd, SHUT_RDWR);
>          close(fd);
>      }
>  }
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index ba60436..bf42861 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -372,6 +372,7 @@ static void nbd_accept(void *opaque)
>      if (nbd_client_new(exp, fd, nbd_client_closed)) {
>          nb_fds++;
>      } else {
> +        shutdown(fd, SHUT_RDWR);
>          close(fd);
>      }
>  }
>

IIUC, what this does is ensure that the other side gets a FIN before it 
gets a RST.  Is this correct?

Paolo

Re: [Qemu-devel] [PATCH 2/3] nbd: Add exports listing support.

[Paolo Bonzini]

Il 31/05/2014 23:39, Hani Benhabiles ha scritto:
> This is added by handling the NBD_OPT_LIST and NBD_OPT_ABORT options.
>
> NBD_REP_ERR_UNSUP is also sent for unknown NBD option values.
>
> Signed-off-by: Hani Benhabiles <hani@linux.com>
> ---
>  include/block/nbd.h |   5 ++
>  nbd.c               | 197 +++++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 162 insertions(+), 40 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 95d52ab..fd7e057 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -51,6 +51,11 @@ struct nbd_reply {
>  /* New-style client flags. */
>  #define NBD_FLAG_C_FIXED_NEWSTYLE   (1 << 0)    /* Fixed newstyle protocol. */
>
> +/* Reply types. */
> +#define NBD_REP_ACK             (1)             /* Data sending finished. */
> +#define NBD_REP_SERVER          (2)             /* Export description. */
> +#define NBD_REP_ERR_UNSUP       ((1 << 31) | 1) /* Unknown option. */
> +
>  #define NBD_CMD_MASK_COMMAND	0x0000ffff
>  #define NBD_CMD_FLAG_FUA	(1 << 16)
>
> diff --git a/nbd.c b/nbd.c
> index aa2e552..012e5da 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -64,6 +64,7 @@
>  #define NBD_REPLY_MAGIC         0x67446698
>  #define NBD_OPTS_MAGIC          0x49484156454F5054LL
>  #define NBD_CLIENT_MAGIC        0x0000420281861253LL
> +#define NBD_REP_MAGIC           0x3e889045565a9LL
>
>  #define NBD_SET_SOCK            _IO(0xab, 0)
>  #define NBD_SET_BLKSIZE         _IO(0xab, 1)
> @@ -77,7 +78,9 @@
>  #define NBD_SET_TIMEOUT         _IO(0xab, 9)
>  #define NBD_SET_FLAGS           _IO(0xab, 10)
>
> -#define NBD_OPT_EXPORT_NAME     (1 << 0)
> +#define NBD_OPT_EXPORT_NAME     (1)
> +#define NBD_OPT_ABORT           (2)
> +#define NBD_OPT_LIST            (3)
>
>  /* Definitions for opaque data types */
>
> @@ -215,54 +218,98 @@ static ssize_t write_sync(int fd, void *buffer, size_t size)
>
>  */
>
> -static int nbd_receive_options(NBDClient *client)
> +static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
>  {
> -    int csock = client->sock;
> -    char name[256];
> -    uint32_t tmp, length;
>      uint64_t magic;
> -    int rc;
> -
> -    /* Client sends:
> -        [ 0 ..   3]   client flags
> -        [ 4 ..  11]   NBD_OPTS_MAGIC
> -        [12 ..  15]   NBD_OPT_EXPORT_NAME
> -        [16 ..  19]   length
> -        [20 ..  xx]   export name (length bytes)
> -     */
> +    uint32_t len;
>
> -    rc = -EINVAL;
> -    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> -        LOG("read failed");
> -        goto fail;
> +    magic = cpu_to_be64(NBD_REP_MAGIC);
> +    if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> +        LOG("write failed (rep magic)");
> +        return -EINVAL;
>      }
> -    TRACE("Checking client flags");
> -    tmp = be32_to_cpu(tmp);
> -    if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
> -        LOG("Bad client flags received");
> -        goto fail;
> +    opt = cpu_to_be32(opt);
> +    if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
> +        LOG("write failed (rep opt)");
> +        return -EINVAL;
>      }
> -
> -    if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> -        LOG("read failed");
> -        goto fail;
> +    type = cpu_to_be32(type);
> +    if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
> +        LOG("write failed (rep type)");
> +        return -EINVAL;
>      }
> -    TRACE("Checking opts magic");
> -    if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
> -        LOG("Bad magic received");
> -        goto fail;
> +    len = cpu_to_be32(0);
> +    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> +        LOG("write failed (rep data length)");
> +        return -EINVAL;
>      }
> +    return 0;
> +}
>
> -    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> -        LOG("read failed");
> -        goto fail;
> +static int nbd_send_rep_list(int csock, NBDExport *exp)
> +{
> +    uint64_t magic, name_len;
> +    uint32_t opt, type, len;
> +
> +    name_len = strlen(exp->name);
> +    magic = cpu_to_be64(NBD_REP_MAGIC);
> +    if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> +        LOG("write failed (magic)");
> +        return -EINVAL;
>      }
> -    TRACE("Checking option");
> -    if (tmp != be32_to_cpu(NBD_OPT_EXPORT_NAME)) {
> -        LOG("Bad option received");
> -        goto fail;
> +    opt = cpu_to_be32(NBD_OPT_LIST);
> +    if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
> +        LOG("write failed (opt)");
> +        return -EINVAL;
> +    }
> +    type = cpu_to_be32(NBD_REP_SERVER);
> +    if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
> +        LOG("write failed (reply type)");
> +        return -EINVAL;
> +    }
> +    len = cpu_to_be32(name_len + sizeof(len));
> +    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> +        LOG("write failed (length)");
> +        return -EINVAL;
> +    }
> +    len = cpu_to_be32(name_len);
> +    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> +        LOG("write failed (length)");
> +        return -EINVAL;
> +    }
> +    if (write_sync(csock, exp->name, name_len) != name_len) {
> +        LOG("write failed (buffer)");
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +static int nbd_send_list(NBDClient *client)
> +{
> +    int csock;
> +    NBDExport *exp;
> +
> +    csock = client->sock;
> +    /* For each export, send a NBD_REP_SERVER reply. */
> +    QTAILQ_FOREACH(exp, &exports, next) {
> +        if (nbd_send_rep_list(csock, exp)) {
> +            return -EINVAL;
> +        }
>      }
> +    /* Finish with a NBD_REP_ACK. */
> +    return nbd_send_rep(csock, NBD_REP_ACK, NBD_OPT_LIST);
> +}
> +
> +static int nbd_handle_export_name(NBDClient *client)
> +{
> +    int rc = -EINVAL, csock = client->sock;
> +    char name[256];
> +    uint32_t length;
>
> +    /* Client sends:
> +        [16 ..  19]   length
> +        [20 ..  xx]   export name (length bytes)
> +     */
>      if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
>          LOG("read failed");
>          goto fail;
> @@ -287,8 +334,76 @@ static int nbd_receive_options(NBDClient *client)
>
>      QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
>      nbd_export_get(client->exp);
> +    rc = 0;
> +fail:
> +    return rc;
> +}
> +
> +static int nbd_receive_options(NBDClient *client)
> +{
> +    int rc = -EINVAL;
>
> -    TRACE("Option negotiation succeeded.");
> +    while (1) {
> +        int csock = client->sock;
> +        uint32_t tmp;
> +        uint64_t magic;
> +
> +        /* Client sends:
> +            [ 0 ..   3]   client flags
> +            [ 4 ..  11]   NBD_OPTS_MAGIC
> +            [12 ..  15]   NBD option
> +            ...           Rest of request
> +        */
> +
> +        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> +            LOG("read failed");
> +            goto fail;
> +        }
> +        TRACE("Checking client flags");
> +        tmp = be32_to_cpu(tmp);
> +        if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
> +            LOG("Bad client flags received");
> +            goto fail;
> +        }
> +
> +        if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> +            LOG("read failed");
> +            goto fail;
> +        }
> +        TRACE("Checking opts magic");
> +        if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
> +            LOG("Bad magic received");
> +            goto fail;
> +        }
> +
> +        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> +            LOG("read failed");
> +            goto fail;
> +        }
> +
> +        TRACE("Checking option");
> +        switch (be32_to_cpu(tmp)) {
> +        case NBD_OPT_LIST:
> +            if (nbd_send_list(client) < 0) {
> +                return -EINVAL;
> +            }
> +            break;

You can just do "return nbd_send_list(client);" here.  You probably 
should also rename the function to nbd_handle_list.

> +        case NBD_OPT_ABORT:
> +            return 1;
> +
> +        case NBD_OPT_EXPORT_NAME:
> +            return nbd_handle_export_name(client);

Part of this patch belongs in patch 1.  This one should only add 
nbd_send_rep_list, nbd_handle_list and the NBD_OPT_LIST case.

Paolo

> +        default:
> +            tmp = be32_to_cpu(tmp);
> +            LOG("Unsupported option 0x%x", tmp);
> +            if (nbd_send_rep(client->sock, NBD_REP_ERR_UNSUP, tmp)) {
> +                return -EINVAL;
> +            }
> +            return 1;
> +        }
> +    }
>      rc = 0;
>  fail:
>      return rc;
> @@ -351,6 +466,8 @@ static int nbd_send_negotiate(NBDClient *client)
>          if (rc < 0) {
>              LOG("option negotiation failed");
>              goto fail;
> +        } else if (rc > 0) {
> +            goto fail;
>          }
>
>          assert ((client->exp->nbdflags & ~65535) == 0);
> @@ -1175,7 +1292,7 @@ NBDClient *nbd_client_new(NBDExport *exp, int csock,
>      client->refcount = 1;
>      client->exp = exp;
>      client->sock = csock;
> -    if (nbd_send_negotiate(client) < 0) {
> +    if (nbd_send_negotiate(client)) {
>          g_free(client);
>          return NULL;
>      }
>

Re: [Qemu-devel] [PATCH 1/3] nbd: Handle fixed new-style clients.

[Stefan Hajnoczi]

On Mon, Jun 02, 2014 at 11:09:02PM +0100, Hani Benhabiles wrote:
> On Mon, Jun 02, 2014 at 02:32:06PM +0200, Stefan Hajnoczi wrote:
> > On Sat, May 31, 2014 at 10:39:40PM +0100, Hani Benhabiles wrote:
> > > Signed-off-by: Hani Benhabiles <hani@linux.com>
> > > ---
> > >  include/block/nbd.h |  6 ++++++
> > >  nbd.c               | 12 +++++++-----
> > >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > No explanation or link to specification for this new flag field?  What's
> > different about a new-style client?
> 
> With this flag is set, the server tells the client that it can send another
> option if the server got a request with an option it doesn't understand (instead
> of the server closing the connection.) Thus, the while(1) loop in 2/3.

Great, please include this explanation in the commit description.

> The kernel in Documentation/blockdev/nbd.txt points to the NBD project for
> documentation. The proto documentation is in [1]. Shouldn't Qemu also do the
> same ?
> 
> [1] https://github.com/yoe/nbd/blob/master/doc/proto.txt

Please add the link as a comment to the top of nbd.c.

Stefan

Re: [Qemu-devel] [PATCH 3/3] nbd: Shutdown socket before closing.

[Hani Benhabiles]

On Tue, Jun 03, 2014 at 01:33:41PM +0200, Paolo Bonzini wrote:
> Il 31/05/2014 23:39, Hani Benhabiles ha scritto:
> >This forces finishing data sending to client before closing the socket like in
> >exports listing or replying with NBD_REP_ERR_UNSUP cases.
> >
> >Signed-off-by: Hani Benhabiles <hani@linux.com>
> >---
> > blockdev-nbd.c | 1 +
> > qemu-nbd.c     | 1 +
> > 2 files changed, 2 insertions(+)
> >
> >diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> >index b60b66d..e609f66 100644
> >--- a/blockdev-nbd.c
> >+++ b/blockdev-nbd.c
> >@@ -28,6 +28,7 @@ static void nbd_accept(void *opaque)
> >
> >     int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> >     if (fd >= 0 && !nbd_client_new(NULL, fd, nbd_client_put)) {
> >+        shutdown(fd, SHUT_RDWR);
> >         close(fd);
> >     }
> > }
> >diff --git a/qemu-nbd.c b/qemu-nbd.c
> >index ba60436..bf42861 100644
> >--- a/qemu-nbd.c
> >+++ b/qemu-nbd.c
> >@@ -372,6 +372,7 @@ static void nbd_accept(void *opaque)
> >     if (nbd_client_new(exp, fd, nbd_client_closed)) {
> >         nb_fds++;
> >     } else {
> >+        shutdown(fd, SHUT_RDWR);
> >         close(fd);
> >     }
> > }
> >
> 
> IIUC, what this does is ensure that the other side gets a FIN before it gets
> a RST.  Is this correct?

Yes. Without shutdown(), this could be reproduced (unreliably) on multiple
tries. This is done in nbd_client_close() too, for the same reasons AFAICT.

Re: [Qemu-devel] [PATCH 2/3] nbd: Add exports listing support.

[Hani Benhabiles]

On Tue, Jun 03, 2014 at 01:36:35PM +0200, Paolo Bonzini wrote:
> Il 31/05/2014 23:39, Hani Benhabiles ha scritto:
> >This is added by handling the NBD_OPT_LIST and NBD_OPT_ABORT options.
> >
> >NBD_REP_ERR_UNSUP is also sent for unknown NBD option values.
> >
> >Signed-off-by: Hani Benhabiles <hani@linux.com>
> >---
> > include/block/nbd.h |   5 ++
> > nbd.c               | 197 +++++++++++++++++++++++++++++++++++++++++-----------
> > 2 files changed, 162 insertions(+), 40 deletions(-)
> >
> >diff --git a/include/block/nbd.h b/include/block/nbd.h
> >index 95d52ab..fd7e057 100644
> >--- a/include/block/nbd.h
> >+++ b/include/block/nbd.h
> >@@ -51,6 +51,11 @@ struct nbd_reply {
> > /* New-style client flags. */
> > #define NBD_FLAG_C_FIXED_NEWSTYLE   (1 << 0)    /* Fixed newstyle protocol. */
> >
> >+/* Reply types. */
> >+#define NBD_REP_ACK             (1)             /* Data sending finished. */
> >+#define NBD_REP_SERVER          (2)             /* Export description. */
> >+#define NBD_REP_ERR_UNSUP       ((1 << 31) | 1) /* Unknown option. */
> >+
> > #define NBD_CMD_MASK_COMMAND	0x0000ffff
> > #define NBD_CMD_FLAG_FUA	(1 << 16)
> >
> >diff --git a/nbd.c b/nbd.c
> >index aa2e552..012e5da 100644
> >--- a/nbd.c
> >+++ b/nbd.c
> >@@ -64,6 +64,7 @@
> > #define NBD_REPLY_MAGIC         0x67446698
> > #define NBD_OPTS_MAGIC          0x49484156454F5054LL
> > #define NBD_CLIENT_MAGIC        0x0000420281861253LL
> >+#define NBD_REP_MAGIC           0x3e889045565a9LL
> >
> > #define NBD_SET_SOCK            _IO(0xab, 0)
> > #define NBD_SET_BLKSIZE         _IO(0xab, 1)
> >@@ -77,7 +78,9 @@
> > #define NBD_SET_TIMEOUT         _IO(0xab, 9)
> > #define NBD_SET_FLAGS           _IO(0xab, 10)
> >
> >-#define NBD_OPT_EXPORT_NAME     (1 << 0)
> >+#define NBD_OPT_EXPORT_NAME     (1)
> >+#define NBD_OPT_ABORT           (2)
> >+#define NBD_OPT_LIST            (3)
> >
> > /* Definitions for opaque data types */
> >
> >@@ -215,54 +218,98 @@ static ssize_t write_sync(int fd, void *buffer, size_t size)
> >
> > */
> >
> >-static int nbd_receive_options(NBDClient *client)
> >+static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
> > {
> >-    int csock = client->sock;
> >-    char name[256];
> >-    uint32_t tmp, length;
> >     uint64_t magic;
> >-    int rc;
> >-
> >-    /* Client sends:
> >-        [ 0 ..   3]   client flags
> >-        [ 4 ..  11]   NBD_OPTS_MAGIC
> >-        [12 ..  15]   NBD_OPT_EXPORT_NAME
> >-        [16 ..  19]   length
> >-        [20 ..  xx]   export name (length bytes)
> >-     */
> >+    uint32_t len;
> >
> >-    rc = -EINVAL;
> >-    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> >-        LOG("read failed");
> >-        goto fail;
> >+    magic = cpu_to_be64(NBD_REP_MAGIC);
> >+    if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> >+        LOG("write failed (rep magic)");
> >+        return -EINVAL;
> >     }
> >-    TRACE("Checking client flags");
> >-    tmp = be32_to_cpu(tmp);
> >-    if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
> >-        LOG("Bad client flags received");
> >-        goto fail;
> >+    opt = cpu_to_be32(opt);
> >+    if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
> >+        LOG("write failed (rep opt)");
> >+        return -EINVAL;
> >     }
> >-
> >-    if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> >-        LOG("read failed");
> >-        goto fail;
> >+    type = cpu_to_be32(type);
> >+    if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
> >+        LOG("write failed (rep type)");
> >+        return -EINVAL;
> >     }
> >-    TRACE("Checking opts magic");
> >-    if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
> >-        LOG("Bad magic received");
> >-        goto fail;
> >+    len = cpu_to_be32(0);
> >+    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> >+        LOG("write failed (rep data length)");
> >+        return -EINVAL;
> >     }
> >+    return 0;
> >+}
> >
> >-    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> >-        LOG("read failed");
> >-        goto fail;
> >+static int nbd_send_rep_list(int csock, NBDExport *exp)
> >+{
> >+    uint64_t magic, name_len;
> >+    uint32_t opt, type, len;
> >+
> >+    name_len = strlen(exp->name);
> >+    magic = cpu_to_be64(NBD_REP_MAGIC);
> >+    if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> >+        LOG("write failed (magic)");
> >+        return -EINVAL;
> >     }
> >-    TRACE("Checking option");
> >-    if (tmp != be32_to_cpu(NBD_OPT_EXPORT_NAME)) {
> >-        LOG("Bad option received");
> >-        goto fail;
> >+    opt = cpu_to_be32(NBD_OPT_LIST);
> >+    if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
> >+        LOG("write failed (opt)");
> >+        return -EINVAL;
> >+    }
> >+    type = cpu_to_be32(NBD_REP_SERVER);
> >+    if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
> >+        LOG("write failed (reply type)");
> >+        return -EINVAL;
> >+    }
> >+    len = cpu_to_be32(name_len + sizeof(len));
> >+    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> >+        LOG("write failed (length)");
> >+        return -EINVAL;
> >+    }
> >+    len = cpu_to_be32(name_len);
> >+    if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
> >+        LOG("write failed (length)");
> >+        return -EINVAL;
> >+    }
> >+    if (write_sync(csock, exp->name, name_len) != name_len) {
> >+        LOG("write failed (buffer)");
> >+        return -EINVAL;
> >+    }
> >+    return 0;
> >+}
> >+
> >+static int nbd_send_list(NBDClient *client)
> >+{
> >+    int csock;
> >+    NBDExport *exp;
> >+
> >+    csock = client->sock;
> >+    /* For each export, send a NBD_REP_SERVER reply. */
> >+    QTAILQ_FOREACH(exp, &exports, next) {
> >+        if (nbd_send_rep_list(csock, exp)) {
> >+            return -EINVAL;
> >+        }
> >     }
> >+    /* Finish with a NBD_REP_ACK. */
> >+    return nbd_send_rep(csock, NBD_REP_ACK, NBD_OPT_LIST);
> >+}
> >+
> >+static int nbd_handle_export_name(NBDClient *client)
> >+{
> >+    int rc = -EINVAL, csock = client->sock;
> >+    char name[256];
> >+    uint32_t length;
> >
> >+    /* Client sends:
> >+        [16 ..  19]   length
> >+        [20 ..  xx]   export name (length bytes)
> >+     */
> >     if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
> >         LOG("read failed");
> >         goto fail;
> >@@ -287,8 +334,76 @@ static int nbd_receive_options(NBDClient *client)
> >
> >     QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
> >     nbd_export_get(client->exp);
> >+    rc = 0;
> >+fail:
> >+    return rc;
> >+}
> >+
> >+static int nbd_receive_options(NBDClient *client)
> >+{
> >+    int rc = -EINVAL;
> >
> >-    TRACE("Option negotiation succeeded.");
> >+    while (1) {
> >+        int csock = client->sock;
> >+        uint32_t tmp;
> >+        uint64_t magic;
> >+
> >+        /* Client sends:
> >+            [ 0 ..   3]   client flags
> >+            [ 4 ..  11]   NBD_OPTS_MAGIC
> >+            [12 ..  15]   NBD option
> >+            ...           Rest of request
> >+        */
> >+
> >+        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> >+            LOG("read failed");
> >+            goto fail;
> >+        }
> >+        TRACE("Checking client flags");
> >+        tmp = be32_to_cpu(tmp);
> >+        if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
> >+            LOG("Bad client flags received");
> >+            goto fail;
> >+        }
> >+
> >+        if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> >+            LOG("read failed");
> >+            goto fail;
> >+        }
> >+        TRACE("Checking opts magic");
> >+        if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
> >+            LOG("Bad magic received");
> >+            goto fail;
> >+        }
> >+
> >+        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> >+            LOG("read failed");
> >+            goto fail;
> >+        }
> >+
> >+        TRACE("Checking option");
> >+        switch (be32_to_cpu(tmp)) {
> >+        case NBD_OPT_LIST:
> >+            if (nbd_send_list(client) < 0) {
> >+                return -EINVAL;
> >+            }
> >+            break;
> 
> You can just do "return nbd_send_list(client);" here.  You probably should
> also rename the function to nbd_handle_list.
> 

Ack.

> >+        case NBD_OPT_ABORT:
> >+            return 1;
> >+
> >+        case NBD_OPT_EXPORT_NAME:
> >+            return nbd_handle_export_name(client);
> 
> Part of this patch belongs in patch 1.  This one should only add
> nbd_send_rep_list, nbd_handle_list and the NBD_OPT_LIST case.
> 

Alright, I will resend, making the changes you and Stefan suggested.

Thanks.

Re: [Qemu-devel] [PATCH 3/3] nbd: Shutdown socket before closing.

[Paolo Bonzini]

Il 05/06/2014 00:33, Hani Benhabiles ha scritto:
> > IIUC, what this does is ensure that the other side gets a FIN before it gets
> > a RST.  Is this correct?
>
> Yes. Without shutdown(), this could be reproduced (unreliably) on multiple
> tries. This is done in nbd_client_close() too, for the same reasons AFAICT.

Actually, nbd_client_close() is different because it's an abortive close 
of the socket.  nbd_client_close() doesn't care about FIN vs. RST, it 
does the shutdown to force all the requests to fail (with either an 
error for writes, or a short read if they're receiving).  This will 
cause a flurry of nbd_client_put() calls soon after nbd_clint_close() 
returns, until the last reference is dropped and the socket is closed.

I'll apply the patch.

Paolo

Re: [Qemu-devel] [PATCH 3/3] nbd: Shutdown socket before closing.

[Hani Benhabiles]

On Thu, Jun 05, 2014 at 03:55:40AM +0200, Paolo Bonzini wrote:
> Il 05/06/2014 00:33, Hani Benhabiles ha scritto:
> >> IIUC, what this does is ensure that the other side gets a FIN before it gets
> >> a RST.  Is this correct?
> >
> >Yes. Without shutdown(), this could be reproduced (unreliably) on multiple
> >tries. This is done in nbd_client_close() too, for the same reasons AFAICT.
> 
> Actually, nbd_client_close() is different because it's an abortive close of
> the socket.  nbd_client_close() doesn't care about FIN vs. RST, it does the
> shutdown to force all the requests to fail (with either an error for writes,
> or a short read if they're receiving).  This will cause a flurry of
> nbd_client_put() calls soon after nbd_clint_close() returns, until the last
> reference is dropped and the socket is closed.
> 

I see, thanks for the explanation.

> I'll apply the patch.

Will you apply it directly or should I resend it in v3 ?

Re: [Qemu-devel] [PATCH 3/3] nbd: Shutdown socket before closing.

[Paolo Bonzini]

Il 05/06/2014 11:58, Hani Benhabiles ha scritto:
> On Thu, Jun 05, 2014 at 03:55:40AM +0200, Paolo Bonzini wrote:
>> Il 05/06/2014 00:33, Hani Benhabiles ha scritto:
>>>> IIUC, what this does is ensure that the other side gets a FIN before it gets
>>>> a RST.  Is this correct?
>>>
>>> Yes. Without shutdown(), this could be reproduced (unreliably) on multiple
>>> tries. This is done in nbd_client_close() too, for the same reasons AFAICT.
>>
>> Actually, nbd_client_close() is different because it's an abortive close of
>> the socket.  nbd_client_close() doesn't care about FIN vs. RST, it does the
>> shutdown to force all the requests to fail (with either an error for writes,
>> or a short read if they're receiving).  This will cause a flurry of
>> nbd_client_put() calls soon after nbd_clint_close() returns, until the last
>> reference is dropped and the socket is closed.
>>
>
> I see, thanks for the explanation.
>
>> I'll apply the patch.
>
> Will you apply it directly or should I resend it in v3 ?
>

This is independent, so I can apply it first.

Paolo