[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Feb 25, 2013 at 06:44:22PM +0100, 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 <pbonzini redhat com>
> ---
>  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)
> +        goto no_memory;
> +
> +    h->port = strdup(port);
> +    if (!h->port)
> +        goto no_memory;
> +
> +    VIR_FREE(disk->src);
> +    disk->nhosts = 1;
> +    disk->hosts = h;
> +    return 0;
> +
> +no_memory:
> +    virReportOOMError();
> +error:
> +    virDomainDiskHostDefFree(h);
> +    VIR_FREE(h);
> +    return -1;
> +}
> +

I would have had the 'parse' method further down near the other
parse function which calls it, but no big deal.

> +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");
> +        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");

This test case addition could be pushed indepedently of the rest
of this patch.

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]