[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 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<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)

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


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