Re: [Qemu-devel] [PATCH] Introduce NBD named exports.

[Kevin Wolf]

Am 24.08.2010 03:04, schrieb Laurent Vivier:
> This patch allows to connect Qemu using NBD protocol to an nbd-server
> using named exports.
> 
> For instance, if on the host "isoserver", in /etc/nbd-server/config, you have:
> 
> [generic]
> [debian-500-ppc-netinst]
>         exportname = /ISO/debian-500-powerpc-netinst.iso
> [Fedora-10-ppc-netinst]
>         exportname = /ISO/Fedora-10-ppc-netinst.iso
> 
> You can connect to it, using:
> 
>     qemu -cdrom nbd:isoserver:exportname=debian-500-ppc-netinst
>     qemu -cdrom nbd:isoserver:exportname=Fedora-10-ppc-netinst
> 
> NOTE: you need at least nbd-server 2.9.18
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  block/nbd.c   |   57 +++++++++++++++++++--------
>  nbd.c         |  119 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  nbd.h         |    5 ++-
>  qemu-doc.texi |    7 +++
>  qemu-nbd.c    |    2 +-
>  5 files changed, 158 insertions(+), 32 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index a1ec123..ee22b47 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -42,55 +42,78 @@ typedef struct BDRVNBDState {
>  static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
>  {
>      BDRVNBDState *s = bs->opaque;
> +
> +    #define EN_OPTSTR ":exportname="

Hm, this makes EN_OPTSTR look like a local variable, but it's actually a
macro which is valid after the end of the function. So maybe move it out?

> +    char *file;
> +    char *name;
>      const char *host;
>      const char *unixpath;
>      int sock;
>      off_t size;
>      size_t blocksize;
>      int ret;
> +    int err = -EINVAL;
> +
> +    file = qemu_strdup(filename);
>  
> -    if (!strstart(filename, "nbd:", &host))
> -        return -EINVAL;
> +    name = strstr(file, EN_OPTSTR);
> +    if (name) {
> +        if (name[sizeof(EN_OPTSTR) - 1] == 0)
> +            goto out;

Please add braces here and throughout the patch (see CODING_STYLE)

Maybe using strlen(EN_OPTSTR) would be clearer? At first I missed the
fact that sizeof includes the null byte, but maybe it's just me.

> +        name[0] = 0;
> +        name += sizeof(EN_OPTSTR) - 1;
> +    }
> +
> +    if (!strstart(file, "nbd:", &host))
> +        goto out;
>  
>      if (strstart(host, "unix:", &unixpath)) {
>  
>          if (unixpath[0] != '/')
> -            return -EINVAL;
> +            goto out;
>  
>          sock = unix_socket_outgoing(unixpath);
>  
>      } else {
> -        uint16_t port;
> +        uint16_t port = NBD_DEFAULT_PORT;
>          char *p, *r;
>          char hostname[128];
>  
>          pstrcpy(hostname, 128, host);
>  
>          p = strchr(hostname, ':');
> -        if (p == NULL)
> -            return -EINVAL;
> +        if (p != NULL) {
> +            *p = '\0';
> +            p++;
>  
> -        *p = '\0';
> -        p++;
> +            port = strtol(p, &r, 0);
> +            if (r == p)
> +                goto out;
> +        } else if (name == NULL)
> +                goto out;

Wrong indentation level (and missing braces again)

>  
> -        port = strtol(p, &r, 0);
> -        if (r == p)
> -            return -EINVAL;
>          sock = tcp_socket_outgoing(hostname, port);
>      }
>  
> -    if (sock == -1)
> -        return -errno;
> +    if (sock == -1) {
> +        err = -errno;
> +        goto out;
> +    }
>  
> -    ret = nbd_receive_negotiate(sock, &size, &blocksize);
> -    if (ret == -1)
> -        return -errno;
> +    ret = nbd_receive_negotiate(sock, name, &size, &blocksize);
> +    if (ret == -1) {
> +        err = -errno;
> +        goto out;
> +    }
>  
>      s->sock = sock;
>      s->size = size;
>      s->blocksize = blocksize;
> +    err = 0;
>  
> -    return 0;
> +out:
> +    qemu_free(file);
> +    return err;
>  }
>  
>  static int nbd_read(BlockDriverState *bs, int64_t sector_num,
> diff --git a/nbd.c b/nbd.c
> index a9f295f..755613a 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -62,6 +62,8 @@
>  #define NBD_SET_SIZE_BLOCKS  _IO(0xab, 7)
>  #define NBD_DISCONNECT          _IO(0xab, 8)
>  
> +#define NBD_OPT_EXPORT_NAME  (1 << 0)
> +
>  /* That's all folks */
>  
>  #define read_sync(fd, buffer, size) nbd_wr_sync(fd, buffer, size, true)
> @@ -296,22 +298,26 @@ int nbd_negotiate(int csock, off_t size)
>       return 0;
>  }
>  
> -int nbd_receive_negotiate(int csock, off_t *size, size_t *blocksize)
> +int nbd_receive_negotiate(int csock, const char *name,
> +                          off_t *size, size_t *blocksize)
>  {
> -     char buf[8 + 8 + 8 + 128];
> -     uint64_t magic;
> +     char buf[256] = "\0\0\0\0\0\0\0\0\0";
> +     uint64_t magic, s;
> +     uint16_t tmp;
> +     uint32_t flags;

Hm, where are the flags actually used? There are a few places where they
are assigned, but I can't see any use.

>  
>       TRACE("Receiving negotation.");
>  
> -     if (read_sync(csock, buf, sizeof(buf)) != sizeof(buf)) {
> +     if (read_sync(csock, buf, 8) != 8) {
>               LOG("read failed");
>               errno = EINVAL;
>               return -1;
>       }

I think this makes it easy to miss that you rely on getting null
termination by the initialisation of buf. Why not do the obvious thing
and just do a buf[8] = '\0' after the read?

> -
> -     magic = be64_to_cpup((uint64_t*)(buf + 8));
> -     *size = be64_to_cpup((uint64_t*)(buf + 16));
> -     *blocksize = 1024;
> +     if (strlen(buf) == 0) {
> +             LOG("server connection closed");
> +             errno = EINVAL;
> +             return -1;
> +     }
>  
>       TRACE("Magic is %c%c%c%c%c%c%c%c",
>             qemu_isprint(buf[0]) ? buf[0] : '.',
> @@ -322,8 +328,6 @@ int nbd_receive_negotiate(int csock, off_t *size, size_t 
> *blocksize)
>             qemu_isprint(buf[5]) ? buf[5] : '.',
>             qemu_isprint(buf[6]) ? buf[6] : '.',
>             qemu_isprint(buf[7]) ? buf[7] : '.');
> -     TRACE("Magic is 0x%" PRIx64, magic);
> -     TRACE("Size is %" PRIu64, *size);
>  
>       if (memcmp(buf, "NBDMAGIC", 8) != 0) {
>               LOG("Invalid magic received");
> @@ -331,10 +335,99 @@ int nbd_receive_negotiate(int csock, off_t *size, 
> size_t *blocksize)
>               return -1;
>       }
>  
> -     TRACE("Checking magic");
> +     if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> +             LOG("read failed");
> +             errno = EINVAL;
> +             return -1;
> +     }
> +     magic = be64_to_cpu(magic);
> +     TRACE("Magic is 0x%" PRIx64, magic);
>  
> -     if (magic != 0x00420281861253LL) {
> -             LOG("Bad magic received");
> +     if (name) {
> +             uint32_t reserved = 0;
> +             uint32_t opt;
> +             uint32_t namesize;
> +
> +             TRACE("Checking magic (opts_magic)");
> +             if (magic != 0x49484156454F5054LL) {
> +                     LOG("Bad magic received");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> +                     LOG("flags read failed");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             flags = be16_to_cpu(tmp) << 16;
> +             /* reserved for future use */
> +             if (write_sync(csock, &reserved, sizeof(reserved)) !=
> +                 sizeof(reserved)) {
> +                     LOG("write failed (reserved)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             /* write the export name */
> +             magic = cpu_to_be64(magic);
> +             if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
> +                     LOG("write failed (magic)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             opt = cpu_to_be32(NBD_OPT_EXPORT_NAME);
> +             if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
> +                     LOG("write failed (opt)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             namesize = cpu_to_be32(strlen(name));
> +             if (write_sync(csock, &namesize, sizeof(namesize)) !=
> +                 sizeof(namesize)) {
> +                     LOG("write failed (namesize)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             if (write_sync(csock, (char*)name, strlen(name)) != 
> strlen(name)) {
> +                     LOG("write failed (name)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +     } else {
> +             TRACE("Checking magic (cli_magic)");
> +
> +             if (magic != 0x00420281861253LL) {
> +                     LOG("Bad magic received");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +     }
> +
> +     if (read_sync(csock, &s, sizeof(s)) != sizeof(s)) {
> +             LOG("read failed");
> +             errno = EINVAL;
> +             return -1;
> +     }
> +     *size = be64_to_cpu(s);
> +     *blocksize = 1024;
> +     TRACE("Size is %" PRIu64, *size);
> +
> +     if (!name) {
> +             if (read_sync(csock, &flags, sizeof(flags)) != sizeof(flags)) {
> +                     LOG("read failed (flags)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             flags = be64_to_cpu(flags);
> +     } else {
> +             if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
> +                     LOG("read failed (tmp)");
> +                     errno = EINVAL;
> +                     return -1;
> +             }
> +             flags |= be32_to_cpu(tmp);
> +     }
> +     if (read_sync(csock, &buf, 124) != 124) {
> +             LOG("read failed (buf)");
>               errno = EINVAL;
>               return -1;
>       }
> diff --git a/nbd.h b/nbd.h
> index 5a1fbdf..17be0fa 100644
> --- a/nbd.h
> +++ b/nbd.h
> @@ -42,6 +42,8 @@ enum {
>      NBD_CMD_DISC = 2
>  };
>  
> +#define NBD_DEFAULT_PORT     10809
> +
>  size_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
>  int tcp_socket_outgoing(const char *address, uint16_t port);
>  int tcp_socket_incoming(const char *address, uint16_t port);
> @@ -49,7 +51,8 @@ int unix_socket_outgoing(const char *path);
>  int unix_socket_incoming(const char *path);
>  
>  int nbd_negotiate(int csock, off_t size);
> -int nbd_receive_negotiate(int csock, off_t *size, size_t *blocksize);
> +int nbd_receive_negotiate(int csock, const char *name,
> +                          off_t *size, size_t *blocksize);
>  int nbd_init(int fd, int csock, off_t size, size_t blocksize);
>  int nbd_send_request(int csock, struct nbd_request *request);
>  int nbd_receive_reply(int csock, struct nbd_reply *reply);
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 55a966f..ca5bf53 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -620,6 +620,13 @@ qemu linux1.img -hdb nbd:unix:/tmp/my_socket
>  qemu linux2.img -hdb nbd:unix:/tmp/my_socket
>  @end example
>  
> +If the nbd-server uses named exports (since NBD 2.9.18) , you must use the

Please remove the space before the comma.

I don't have a new enough ndbserver to test if it actually works (and I
also haven't checked the spec for the new parts), so I'll have to trust
your tests there.

Kevin