| Message ID | 1282611879-28954-1-git-send-email-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
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 <laurent@vivier.eu> > --- > 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
Le mardi 24 août 2010 à 10:59 +0200, Kevin Wolf a écrit : > 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 <laurent@vivier.eu> > > --- > > 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? I agree, but have a look at curl_open(). > > > + 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) OK. > 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. I think sizeof() is resolved at compile time whereas strlen() it is at runtime. For me, it is better, it like to have a constant. > > > + 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) OK > > > > > - 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. It is never used. But as it is in the file, I take it. But I'll modify the function to return flags as it is done in nbd-client (the reference implementation). > > > > > 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? because it is done like that in nbd-client.c... but I agree. > > > - > > - 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. OK > I don't have a new enough ndbserver to test if it actually works (and I You can test against an old nbdserver to check it is always working ;-) (or qemu-nbd). > also haven't checked the spec for the new parts), so I'll have to trust > your tests there. There is no spec (what is a spec ;-) ?), only a reference implementation in nbd-client.c and nbd-server.c from http://nbd.sf.net. Thank you for the review. Patch follows nearly... > > Kevin Laurent
Am 25.08.2010 00:21, schrieb Laurent Vivier: > Le mardi 24 août 2010 à 10:59 +0200, Kevin Wolf a écrit : >> 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 <laurent@vivier.eu> >>> --- >>> 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? > > I agree, but have a look at curl_open(). Seems I should have reviewed the curl patch when it came in. :-) >> 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. > > I think sizeof() is resolved at compile time whereas strlen() it is at > runtime. For me, it is better, it like to have a constant. gcc is smart enough to make strlen of a literal string a constant, too. Besides, opening a connection isn't really performance critical. I guess it's a matter of taste, though. So if you like it better as it is, just leave it. >> I don't have a new enough ndbserver to test if it actually works (and I > > You can test against an old nbdserver to check it is always working ;-) > (or qemu-nbd). Sure, I can test for regressions in the old thing. >> also haven't checked the spec for the new parts), so I'll have to trust >> your tests there. > > There is no spec (what is a spec ;-) ?), only a reference implementation > in nbd-client.c and nbd-server.c from http://nbd.sf.net. Oh right, I forgot, the world outside qemu is just as bad. :-) Kevin
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=" + 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; + 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; - 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; 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; } - - 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 +"exportname" option: +@example +qemu -cdrom nbd:localhost:exportname=debian-500-ppc-netinst +qemu -cdrom nbd:localhost:exportname=openSUSE-11.1-ppc-netinst +@end example + @node pcsys_network @section Network emulation diff --git a/qemu-nbd.c b/qemu-nbd.c index 4e607cf..c28ac2f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -398,7 +398,7 @@ int main(int argc, char **argv) goto out; } - ret = nbd_receive_negotiate(sock, &size, &blocksize); + ret = nbd_receive_negotiate(sock, NULL, &size, &blocksize); if (ret == -1) { ret = 1; goto out;
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 <laurent@vivier.eu> --- 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(-)