Re: [libvirt] [PATCH 03/13] qemu: rewrite NBD command-line builder and parser

On 2013年02月26日 01:44, Paolo Bonzini wrote: > Move the code to an external function, and structure it to prepare > the addition of new features in the next few patches. > > Signed-off-by: Paolo Bonzini&lt;pbonzini(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 128 > ++++++++++++++++++++++++++++-------------------- > tests/qemuxml2xmltest.c | 1 + > 2 files changed, 76 insertions(+), 53 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index a3c5a4e..beb7cfe 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -2128,6 +2128,45 @@ error: > } > > static int > +qemuParseNBDString(virDomainDiskDefPtr disk) > +{ > + virDomainDiskHostDefPtr h = NULL; > + char *host, *port; > + > + if (VIR_ALLOC(h)< 0) > + goto no_memory; > + > + host = disk->src + strlen("nbd:"); > + port = strchr(host, ':'); > + if (!port) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot parse nbd filename '%s'"), disk->src); > + goto error; > + } > + > + *port++ = '\0'; > + h->name = strdup(host); > + if (!h->name) Trivial, but we prefer: if (!(h->name = strdup(host))) > + goto no_memory; > + > + h->port = strdup(port); > + if (!h->port) Likewise.

> + goto no_memory; > + > + VIR_FREE(disk->src); You lost setting defaults when refactoring: - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;

> + disk->nhosts = 1; > + disk->hosts = h; > + return 0; > + > +no_memory: > + virReportOOMError(); > +error: > + virDomainDiskHostDefFree(h); > + VIR_FREE(h); > + return -1; > +} > + > +static int > qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) > { > int ret = -1; > @@ -2188,6 +2227,36 @@ no_memory: > goto cleanup; > } > > +static int > +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) > +{ > + const char *transp; > + > + if (disk->nhosts != 1) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("nbd accepts only one host")); > + return -1; > + } > + > + virBufferAddLit(opt, "file=nbd:"); > + > + switch (disk->hosts->transport) { > + case VIR_DOMAIN_DISK_PROTO_TRANS_TCP: > + if (disk->hosts->name) > + virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); > + virBufferEscape(opt, ',', ",", ":%s", > + disk->hosts->port ? disk->hosts->port : > "10809"); What's the magic (10809)? It's not in the old code, I guess it's the default port, but should we have a macro for it instead?

> + break; > + default: > + transp = > virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("nbd does not support transport '%s'"), > transp); > + break; > + } > + > + return 0; > +} > + > char * > qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > virDomainDiskDefPtr disk, > @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn > ATTRIBUTE_UNUSED, > } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { > switch (disk->protocol) { > case VIR_DOMAIN_DISK_PROTOCOL_NBD: > - if (disk->nhosts != 1) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("NBD accepts only one host")); > + if (qemuBuildNBDString(disk,&opt)< 0) > goto error; > - } > - virBufferAsprintf(&opt, "file=nbd:%s:%s,", > - disk->hosts->name, disk->hosts->port); > + virBufferAddChar(&opt, ','); > break; > case VIR_DOMAIN_DISK_PROTOCOL_RBD: > virBufferAddLit(&opt, "file="); > @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, > if (STRPREFIX(def->src, "/dev/")) > def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; > else if (STRPREFIX(def->src, "nbd:")) { > - char *host, *port; > - > def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; > def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; > - host = def->src + strlen("nbd:"); > - port = strchr(host, ':'); > - if (!port) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("cannot parse nbd filename > '%s'"), > - def->src); > - goto error; > - } > - *port++ = '\0'; > - if (VIR_ALLOC(def->hosts)< 0) { > - virReportOOMError(); > - goto error; > - } > - def->nhosts = 1; > - def->hosts->name = strdup(host); > - if (!def->hosts->name) { > - virReportOOMError(); > - goto error; > - } > - def->hosts->port = strdup(port); > - if (!def->hosts->port) { > - virReportOOMError(); > - goto error; > - } > - def->hosts->transport = > VIR_DOMAIN_DISK_PROTO_TRANS_TCP; > - def->hosts->socket = NULL; > > - VIR_FREE(def->src); > - def->src = NULL; > + if (qemuParseNBDString(def)< 0) > + goto error; > } else if (STRPREFIX(def->src, "rbd:")) { > char *p = def->src; > > @@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr > qemuCaps, > else if (STRPREFIX(val, "nbd:")) { > disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; > disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; > - val += strlen("nbd:"); > } else if (STRPREFIX(val, "rbd:")) { > disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; > disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; > @@ -8639,26 +8675,12 @@ virDomainDefPtr > qemuParseCommandLine(virCapsPtr qemuCaps, > goto no_memory; > > if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { > - char *host, *port; > + char *port; > > switch (disk->protocol) { > case VIR_DOMAIN_DISK_PROTOCOL_NBD: > - host = disk->src; > - port = strchr(host, ':'); > - if (!port) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("cannot parse nbd filename > '%s'"), disk->src); > + if (qemuParseNBDString(disk)< 0) > goto error; > - } > - *port++ = '\0'; > - if (VIR_ALLOC(disk->hosts)< 0) > - goto no_memory; > - disk->nhosts = 1; > - disk->hosts->name = host; > - disk->hosts->port = strdup(port); > - if (!disk->hosts->port) > - goto no_memory; > - disk->src = NULL; > break; > case VIR_DOMAIN_DISK_PROTOCOL_RBD: > /* old-style CEPH_ARGS env variable is parsed > later */ > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c > index 3f36896..2fa93a9 100644 > --- a/tests/qemuxml2xmltest.c > +++ b/tests/qemuxml2xmltest.c > @@ -166,6 +166,7 @@ mymain(void) > DO_TEST("disk-drive-cache-v1-wt"); > DO_TEST("disk-drive-cache-v1-wb"); > DO_TEST("disk-drive-cache-v1-none"); > + DO_TEST("disk-drive-network-nbd"); > DO_TEST("disk-scsi-device"); > DO_TEST("disk-scsi-vscsi"); > DO_TEST("disk-scsi-virtio-scsi");