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 <laur...@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
--
--------------------- laur...@vivier.eu ----------------------
"Tout ce qui est impossible reste à accomplir" Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard
