From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=38816 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oo1s0-0001NG-VT for qemu-devel@nongnu.org; Tue, 24 Aug 2010 18:21:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oo1rz-0005ds-9D for qemu-devel@nongnu.org; Tue, 24 Aug 2010 18:21:28 -0400 Received: from moutng.kundenserver.de ([212.227.126.187]:61862) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oo1ry-0005dE-P4 for qemu-devel@nongnu.org; Tue, 24 Aug 2010 18:21:27 -0400 Subject: Re: [Qemu-devel] [PATCH] Introduce NBD named exports. From: Laurent Vivier In-Reply-To: <4C738A01.8090605@redhat.com> References: <1282611879-28954-1-git-send-email-laurent@vivier.eu> <4C738A01.8090605@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Wed, 25 Aug 2010 00:21:20 +0200 Message-ID: <1282688480.22220.14.camel@Quad> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel Le mardi 24 ao=C3=BBt 2010 =C3=A0 10:59 +0200, Kevin Wolf a =C3=A9crit : > 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. > >=20 > > For instance, if on the host "isoserver", in /etc/nbd-server/config, yo= u have: > >=20 > > [generic] > > [debian-500-ppc-netinst] > > exportname =3D /ISO/debian-500-powerpc-netinst.iso > > [Fedora-10-ppc-netinst] > > exportname =3D /ISO/Fedora-10-ppc-netinst.iso > >=20 > > You can connect to it, using: > >=20 > > qemu -cdrom nbd:isoserver:exportname=3Ddebian-500-ppc-netinst > > qemu -cdrom nbd:isoserver:exportname=3DFedora-10-ppc-netinst > >=20 > > NOTE: you need at least nbd-server 2.9.18 > > Signed-off-by: Laurent Vivier > > --- > > 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(-) > >=20 > > 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 fl= ags) > > { > > BDRVNBDState *s =3D bs->opaque; > > + > > + #define EN_OPTSTR ":exportname=3D" >=20 > 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(). >=20 > > + char *file; > > + char *name; > > const char *host; > > const char *unixpath; > > int sock; > > off_t size; > > size_t blocksize; > > int ret; > > + int err =3D -EINVAL; > > + > > + file =3D qemu_strdup(filename); > > =20 > > - if (!strstart(filename, "nbd:", &host)) > > - return -EINVAL; > > + name =3D strstr(file, EN_OPTSTR); > > + if (name) { > > + if (name[sizeof(EN_OPTSTR) - 1] =3D=3D 0) > > + goto out; >=20 > 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. >=20 > > + name[0] =3D 0; > > + name +=3D sizeof(EN_OPTSTR) - 1; > > + } > > + > > + if (!strstart(file, "nbd:", &host)) > > + goto out; > > =20 > > if (strstart(host, "unix:", &unixpath)) { > > =20 > > if (unixpath[0] !=3D '/') > > - return -EINVAL; > > + goto out; > > =20 > > sock =3D unix_socket_outgoing(unixpath); > > =20 > > } else { > > - uint16_t port; > > + uint16_t port =3D NBD_DEFAULT_PORT; > > char *p, *r; > > char hostname[128]; > > =20 > > pstrcpy(hostname, 128, host); > > =20 > > p =3D strchr(hostname, ':'); > > - if (p =3D=3D NULL) > > - return -EINVAL; > > + if (p !=3D NULL) { > > + *p =3D '\0'; > > + p++; > > =20 > > - *p =3D '\0'; > > - p++; > > + port =3D strtol(p, &r, 0); > > + if (r =3D=3D p) > > + goto out; > > + } else if (name =3D=3D NULL) > > + goto out; >=20 > Wrong indentation level (and missing braces again) OK >=20 > > =20 > > - port =3D strtol(p, &r, 0); > > - if (r =3D=3D p) > > - return -EINVAL; > > sock =3D tcp_socket_outgoing(hostname, port); > > } > > =20 > > - if (sock =3D=3D -1) > > - return -errno; > > + if (sock =3D=3D -1) { > > + err =3D -errno; > > + goto out; > > + } > > =20 > > - ret =3D nbd_receive_negotiate(sock, &size, &blocksize); > > - if (ret =3D=3D -1) > > - return -errno; > > + ret =3D nbd_receive_negotiate(sock, name, &size, &blocksize); > > + if (ret =3D=3D -1) { > > + err =3D -errno; > > + goto out; > > + } > > =20 > > s->sock =3D sock; > > s->size =3D size; > > s->blocksize =3D blocksize; > > + err =3D 0; > > =20 > > - return 0; > > +out: > > + qemu_free(file); > > + return err; > > } > > =20 > > 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) > > =20 > > +#define NBD_OPT_EXPORT_NAME (1 << 0) > > + > > /* That's all folks */ > > =20 > > #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; > > } > > =20 > > -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] =3D "\0\0\0\0\0\0\0\0\0"; > > + uint64_t magic, s; > > + uint16_t tmp; > > + uint32_t flags; >=20 > 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). >=20 > > =20 > > TRACE("Receiving negotation."); > > =20 > > - if (read_sync(csock, buf, sizeof(buf)) !=3D sizeof(buf)) { > > + if (read_sync(csock, buf, 8) !=3D 8) { > > LOG("read failed"); > > errno =3D EINVAL; > > return -1; > > } >=20 > 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] =3D '\0' after the read? because it is done like that in nbd-client.c... but I agree. >=20 > > - > > - magic =3D be64_to_cpup((uint64_t*)(buf + 8)); > > - *size =3D be64_to_cpup((uint64_t*)(buf + 16)); > > - *blocksize =3D 1024; > > + if (strlen(buf) =3D=3D 0) { > > + LOG("server connection closed"); > > + errno =3D EINVAL; > > + return -1; > > + } > > =20 > > 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, s= ize_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); > > =20 > > if (memcmp(buf, "NBDMAGIC", 8) !=3D 0) { > > LOG("Invalid magic received"); > > @@ -331,10 +335,99 @@ int nbd_receive_negotiate(int csock, off_t *size,= size_t *blocksize) > > return -1; > > } > > =20 > > - TRACE("Checking magic"); > > + if (read_sync(csock, &magic, sizeof(magic)) !=3D sizeof(magic)) { > > + LOG("read failed"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + magic =3D be64_to_cpu(magic); > > + TRACE("Magic is 0x%" PRIx64, magic); > > =20 > > - if (magic !=3D 0x00420281861253LL) { > > - LOG("Bad magic received"); > > + if (name) { > > + uint32_t reserved =3D 0; > > + uint32_t opt; > > + uint32_t namesize; > > + > > + TRACE("Checking magic (opts_magic)"); > > + if (magic !=3D 0x49484156454F5054LL) { > > + LOG("Bad magic received"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + if (read_sync(csock, &tmp, sizeof(tmp)) !=3D sizeof(tmp)) { > > + LOG("flags read failed"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + flags =3D be16_to_cpu(tmp) << 16; > > + /* reserved for future use */ > > + if (write_sync(csock, &reserved, sizeof(reserved)) !=3D > > + sizeof(reserved)) { > > + LOG("write failed (reserved)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + /* write the export name */ > > + magic =3D cpu_to_be64(magic); > > + if (write_sync(csock, &magic, sizeof(magic)) !=3D sizeof(magic)) { > > + LOG("write failed (magic)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + opt =3D cpu_to_be32(NBD_OPT_EXPORT_NAME); > > + if (write_sync(csock, &opt, sizeof(opt)) !=3D sizeof(opt)) { > > + LOG("write failed (opt)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + namesize =3D cpu_to_be32(strlen(name)); > > + if (write_sync(csock, &namesize, sizeof(namesize)) !=3D > > + sizeof(namesize)) { > > + LOG("write failed (namesize)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + if (write_sync(csock, (char*)name, strlen(name)) !=3D strlen(name)) = { > > + LOG("write failed (name)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + } else { > > + TRACE("Checking magic (cli_magic)"); > > + > > + if (magic !=3D 0x00420281861253LL) { > > + LOG("Bad magic received"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + } > > + > > + if (read_sync(csock, &s, sizeof(s)) !=3D sizeof(s)) { > > + LOG("read failed"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + *size =3D be64_to_cpu(s); > > + *blocksize =3D 1024; > > + TRACE("Size is %" PRIu64, *size); > > + > > + if (!name) { > > + if (read_sync(csock, &flags, sizeof(flags)) !=3D sizeof(flags)) { > > + LOG("read failed (flags)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + flags =3D be64_to_cpu(flags); > > + } else { > > + if (read_sync(csock, &tmp, sizeof(tmp)) !=3D sizeof(tmp)) { > > + LOG("read failed (tmp)"); > > + errno =3D EINVAL; > > + return -1; > > + } > > + flags |=3D be32_to_cpu(tmp); > > + } > > + if (read_sync(csock, &buf, 124) !=3D 124) { > > + LOG("read failed (buf)"); > > errno =3D 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 =3D 2 > > }; > > =20 > > +#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); > > =20 > > 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 > > =20 > > +If the nbd-server uses named exports (since NBD 2.9.18) , you must use= the >=20 > 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... >=20 > Kevin Laurent --=20 --------------------- laurent@vivier.eu ---------------------- "Tout ce qui est impossible reste =C3=A0 accomplir" Jules Verne "Things are only impossible until they're not" Jean-Luc Picard