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

Re: [libvirt] [PATCH 2/9] Introduce new XMLs to specify disk source using libvirt storage



On 2013年02月01日 01:17, John Ferlan wrote:
On 01/30/2013 01:11 PM, Osier Yang wrote:
With this patch, one can specify the disk source using libvirt
storage like:

   <disk type='volume' device='disk'>
     <driver name='qemu' type='raw' cache='none'/>
     <source pool='default' volume='fc18.img'/>
     <target dev='vdb' bus='virtio'/>
   </disk>

"seclables" and "startupPolicy" are not supported for this new
disk type ("volume"). They will be supported in later patches.

docs/formatdomain.html.in:
   * Add documents for new XMLs
docs/schemas/domaincommon.rng:
   * Add rng for new XMLs;
src/conf/domain_conf.h:
   * New struct for 'volume' type disk source (virDomainDiskSourcePoolDef)
   * Add VIR_DOMAIN_DISK_TYPE_VOLUME for enum virDomainDiskType
src/conf/domain_conf.c:
   * New helper virDomainDiskSourcePoolDefParse to parse the 'volume'
     type disk source.
   * New helper virDomainDiskSourcePoolDefFree to free the source def
     if 'volume' type disk.
   * New helper virDomainDiskSourceDefFormat to format the disk source.
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml:
tests/qemuxml2xmltest.c:
   * New test

---
The data struct for disk source should be tidied up. It's a mess
now, but it will be later patches.
---
  docs/formatdomain.html.in                          |   17 +-
  docs/schemas/domaincommon.rng                      |   18 ++
  src/conf/domain_conf.c                             |  259 +++++++++++++-------
  src/conf/domain_conf.h                             |    9 +
  .../qemuxml2argv-disk-source-pool.xml              |   33 +++
  tests/qemuxml2xmltest.c                            |    1 +
  6 files changed, 247 insertions(+), 90 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7ad8aea..ac5657a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1367,6 +1367,11 @@
        &lt;blockio logical_block_size='512' physical_block_size='4096'/&gt;
        &lt;target dev='hda' bus='ide'/&gt;
      &lt;/disk&gt;
+&lt;disk type='volume' device='disk'&gt;
+&lt;driver name='qemu' type='raw'/&gt;
+&lt;source pool='blk-pool0' volume='blk-pool0-vol0'/&gt;
+&lt;target dev='hda' bus='ide'/&gt;
+&lt;/disk&gt;
    &lt;/devices&gt;
    ...</pre>

@@ -1374,7 +1379,7 @@
        <dt><code>disk</code></dt>
        <dd>The<code>disk</code>  element is the main container for describing
          disks. The<code>type</code>  attribute is either "file",
-        "block", "dir", or "network"
+        "block", "dir", "network", or "volume".
          and refers to the underlying source for the disk. The optional
          <code>device</code>  attribute indicates how the disk is to be exposed
          to the guest OS. Possible values for this attribute are
@@ -1444,9 +1449,15 @@
          volume/image will be used.  When the disk<code>type</code>  is
          "network", the<code>source</code>  may have zero or
          more<code>host</code>  sub-elements used to specify the hosts
-        to connect.
+        to connect.  If the disk<code>type</code>  is "volume", the underlying
+        disk source is represented by attributes<code>pool</code>  and
+<code>volume</code>. Attribute<code>pool</code>  specifies the
+        name of storage pool (managed by libvirt) where the disk source resides,
+        and attribute<code>volume</code>  specifies the name of storage volume
+        (managed by libvirt) used as the disk source.
          <span class="since">Since 0.0.3;<code>type='dir'</code>  since
-        0.7.5;<code>type='network'</code>  since 0.8.7</span><br/>
+        0.7.5;<code>type='network'</code>  since 0.8.7;
+<code>type='volume'</code>  since 1.0.3</span><br/>
          For a "file" disk type which represents a cdrom or floppy
          (the<code>device</code>  attribute), it is possible to define
          policy what to do with the disk if the source file is not accessible.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 87653ce..88612ae 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1081,6 +1081,24 @@
              <ref name="diskspec"/>
            </interleave>
          </group>
+<group>
+<attribute name="type">
+<value>volume</value>
+</attribute>
+<interleave>
+<optional>
+<element name="source">
+<attribute name="pool">
+<ref name="genericName"/>
+</attribute>
+<attribute name="volume">
+<ref name="volName"/>
+</attribute>
+</element>
+</optional>
+<ref name="diskspec"/>
+</interleave>
+</group>
          <ref name="diskspec"/>
        </choice>
      </element>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8dbfb96..5e65406 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -182,7 +182,8 @@ VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST,
                "block",
                "file",
                "dir",
-              "network")
+              "network",
+              "volume")

  VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST,
                "disk",
@@ -985,6 +986,18 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def)
      VIR_FREE(def);
  }

+static void
+virDomainDiskSourcePoolDefFree(virDomainDiskSourcePoolDefPtr def)
+{
+    if (!def)
+        return;
+
+    VIR_FREE(def->pool);
+    VIR_FREE(def->volume);
+
+    VIR_FREE(def);
+}
+
  void virDomainDiskDefFree(virDomainDiskDefPtr def)
  {
      unsigned int i;
@@ -994,6 +1007,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)

      VIR_FREE(def->serial);
      VIR_FREE(def->src);
+    virDomainDiskSourcePoolDefFree(def->srcpool);
      VIR_FREE(def->dst);
      VIR_FREE(def->driverName);
      virStorageFileFreeMetadata(def->backingChain);
@@ -3573,6 +3587,46 @@ cleanup:
      goto cleanup;
  }

+static int
+virDomainDiskSourcePoolDefParse(xmlNodePtr node,
+                                virDomainDiskDefPtr def)
+{
+    char *pool = NULL;
+    char *volume = NULL;
+    int ret = -1;
+
+    pool = virXMLPropString(node, "pool");
+    volume = virXMLPropString(node, "volume");
+
+    /* CD-ROM and Floppy allows no source */
+    if (!pool&&  !volume)
+        return 0;
+
+    if ((!pool&&  volume) || (pool&&  !volume)) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("'pool' and 'volume' must be specified together "
+                         "for 'pool' type source"));
+        goto cleanup;
+    }

Couldn't this be simplified to "if (!pool || !volume) {"?

Oh, thanks, it's left unchanged when I moved "if (!pool && !volume)"
above.


+
+    if (VIR_ALLOC(def->srcpool)<  0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    def->srcpool->pool = pool;
+    pool = NULL;
+    def->srcpool->volume = volume;
+    volume = NULL;
+
+    ret = 0;
+
+cleanup:
+    VIR_FREE(pool);
+    VIR_FREE(volume);
+    return ret;
+}
+
  #define VENDOR_LEN  8
  #define PRODUCT_LEN 16

@@ -3666,7 +3720,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
      cur = node->children;
      while (cur != NULL) {
          if (cur->type == XML_ELEMENT_NODE) {
-            if (!source&&  !hosts&&
+            if (!source&&  !hosts&&  !def->srcpool&&
                  xmlStrEqual(cur->name, BAD_CAST "source")) {

                  sourceNode = cur;
@@ -3760,6 +3814,10 @@ virDomainDiskDefParseXML(virCapsPtr caps,
                          child = child->next;
                      }
                      break;
+                case VIR_DOMAIN_DISK_TYPE_VOLUME:
+                    if (virDomainDiskSourcePoolDefParse(cur, def)<  0)
+                        goto error;
+                    break;
                  default:
                      virReportError(VIR_ERR_INTERNAL_ERROR,
                                     _("unexpected disk type %s"),
@@ -4050,7 +4108,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,

      /* Only CDROM and Floppy devices are allowed missing source path
       * to indicate no media present */
-    if (source == NULL&&  hosts == NULL&&
+    if (!source&&  !hosts&&  !def->srcpool&&
          def->device != VIR_DOMAIN_DISK_DEVICE_CDROM&&
          def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
          virReportError(VIR_ERR_NO_SOURCE,
@@ -4072,8 +4130,19 @@ virDomainDiskDefParseXML(virCapsPtr caps,
      }

      if (target == NULL) {
-        virReportError(VIR_ERR_NO_TARGET,
-                       source ? "%s" : NULL, source);
+        if (def->srcpool) {
+            char *tmp;
+            if (virAsprintf(&tmp, "pool = '%s', volume = '%s'",
+                def->srcpool->pool, def->srcpool->volume)<  0) {
+                virReportOOMError();
+                goto error;
+            }
+
+            virReportError(VIR_ERR_NO_TARGET, "%s", tmp);
+            VIR_FREE(tmp);
+        } else {
+            virReportError(VIR_ERR_NO_TARGET, source ? "%s" : NULL, source);
+        }
          goto error;
      }

@@ -12168,6 +12237,102 @@ static void virDomainDiskBlockIoDefFormat(virBufferPtr buf,
      }
  }

+static int
+virDomainDiskSourceDefFormat(virBufferPtr buf,
+                             virDomainDiskDefPtr def)
+{
+    int n;
+    const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy);
+
+    if (def->src || def->nhosts>  0 || def->srcpool ||
+        def->startupPolicy) {
+        switch (def->type) {
+        case VIR_DOMAIN_DISK_TYPE_FILE:

What happened to virBufferAddLit() here?

+            if (def->src)
+                virBufferEscapeString(buf, "<source file='%s'", def->src);

If (!def->src), then startupPolicy and nseclabels won't have "<source " - whether that's realistic
I'm not sure (still learning).  If def->src cannot be NULL like other case labels, then no need
to check. It seems from other code I read that it must be set.

You are correct, it's old bug, which is fixed in later patch of this
series.


+            if (def->startupPolicy)
+                virBufferEscapeString(buf, " startupPolicy='%s'",
+                                      startupPolicy);
+            if (def->nseclabels) {
+                virBufferAddLit(buf, ">\n");
+                virBufferAdjustIndent(buf, 8);
+                for (n = 0; n<  def->nseclabels; n++)
+                    virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]);
+                virBufferAdjustIndent(buf, -8);
+                virBufferAddLit(buf, "</source>\n");
+            } else {
+                virBufferAddLit(buf, "/>\n");
+            }
+            break;
+        case VIR_DOMAIN_DISK_TYPE_BLOCK:
+            virBufferEscapeString(buf, "<source dev='%s'",
+                                  def->src);
+            if (def->nseclabels) {
+                virBufferAddLit(buf, ">\n");
+                virBufferAdjustIndent(buf, 8);
+                for (n = 0; n<  def->nseclabels; n++)
+                    virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]);
+                virBufferAdjustIndent(buf, -8);
+                virBufferAddLit(buf, "</source>\n");
+            } else {
+                virBufferAddLit(buf, "/>\n");
+            }
+            break;
+        case VIR_DOMAIN_DISK_TYPE_DIR:
+            virBufferEscapeString(buf, "<source dir='%s'/>\n",
+                                  def->src);
+            break;
+        case VIR_DOMAIN_DISK_TYPE_NETWORK:
+            virBufferAsprintf(buf, "<source protocol='%s'",
+                              virDomainDiskProtocolTypeToString(def->protocol));
+            if (def->src) {
+                virBufferEscapeString(buf, " name='%s'", def->src);
+            }
+            if (def->nhosts == 0) {
+                virBufferAddLit(buf, "/>\n");
+            } else {
+                int i;
+
+                virBufferAddLit(buf, ">\n");
+                for (i = 0; i<  def->nhosts; i++) {
+                    virBufferAddLit(buf, "<host");
+                    if (def->hosts[i].name) {
+                        virBufferEscapeString(buf, " name='%s'", def->hosts[i].name);
+                    }
+                    if (def->hosts[i].port) {
+                        virBufferEscapeString(buf, " port='%s'",
+                                              def->hosts[i].port);
+                    }
+                    if (def->hosts[i].transport) {
+                        virBufferAsprintf(buf, " transport='%s'",
+                                          virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport));
+                    }
+                    if (def->hosts[i].socket) {
+                        virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket);
+                    }
+                    virBufferAddLit(buf, "/>\n");
+                }
+                virBufferAddLit(buf, "</source>\n");
+            }
+            break;
+        case VIR_DOMAIN_DISK_TYPE_VOLUME:
+            /* Parsing guarantees the def->srcpool->volume cannot be NULL
+             * if def->srcpool->pool is not NULL.
+             */
+            if (def->srcpool->pool)
+                virBufferAsprintf(buf, "<source pool='%s' volume='%s'/>\n",
+                                  def->srcpool->pool, def->srcpool->volume);
+            break;
+        default:
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("unexpected disk type %s"),
+                           virDomainDiskTypeToString(def->type));
+            return -1;
+        }
+    }
+
+    return 0;
+}

  static int
  virDomainDiskDefFormat(virBufferPtr buf,
@@ -12184,10 +12349,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
      const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd);
      const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx);
      const char *copy_on_read = virDomainVirtioEventIdxTypeToString(def->copy_on_read);
-    const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy);
      const char *sgio = virDomainDiskSGIOTypeToString(def->sgio);

-    int n;
      char uuidstr[VIR_UUID_STRING_BUFLEN];

      if (!type) {
@@ -12283,86 +12446,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
          virBufferAddLit(buf, "</auth>\n");
      }

-    if (def->src || def->nhosts>  0 ||
-        def->startupPolicy) {
-        switch (def->type) {
-        case VIR_DOMAIN_DISK_TYPE_FILE:
-            virBufferAddLit(buf,"<source");
-            if (def->src)
-                virBufferEscapeString(buf, " file='%s'", def->src);
-            if (def->startupPolicy)
-                virBufferEscapeString(buf, " startupPolicy='%s'",
-                                      startupPolicy);
-            if (def->nseclabels) {
-                virBufferAddLit(buf, ">\n");
-                virBufferAdjustIndent(buf, 8);
-                for (n = 0; n<  def->nseclabels; n++)
-                    virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]);
-                virBufferAdjustIndent(buf, -8);
-                virBufferAddLit(buf, "</source>\n");
-            } else {
-                virBufferAddLit(buf, "/>\n");
-            }
-            break;
-        case VIR_DOMAIN_DISK_TYPE_BLOCK:
-            virBufferEscapeString(buf, "<source dev='%s'",
-                                  def->src);
-            if (def->nseclabels) {
-                virBufferAddLit(buf, ">\n");
-                virBufferAdjustIndent(buf, 8);
-                for (n = 0; n<  def->nseclabels; n++)
-                    virSecurityDeviceLabelDefFormat(buf, def->seclabels[n]);
-                virBufferAdjustIndent(buf, -8);
-                virBufferAddLit(buf, "</source>\n");
-            } else {
-                virBufferAddLit(buf, "/>\n");
-            }
-            break;
-        case VIR_DOMAIN_DISK_TYPE_DIR:
-            virBufferEscapeString(buf, "<source dir='%s'/>\n",
-                                  def->src);
-            break;
-        case VIR_DOMAIN_DISK_TYPE_NETWORK:
-            virBufferAsprintf(buf, "<source protocol='%s'",
-                              virDomainDiskProtocolTypeToString(def->protocol));
-            if (def->src) {
-                virBufferEscapeString(buf, " name='%s'", def->src);
-            }
-            if (def->nhosts == 0) {
-                virBufferAddLit(buf, "/>\n");
-            } else {
-                int i;
-
-                virBufferAddLit(buf, ">\n");
-                for (i = 0; i<  def->nhosts; i++) {
-                    virBufferAddLit(buf, "<host");
-                    if (def->hosts[i].name) {
-                        virBufferEscapeString(buf, " name='%s'", def->hosts[i].name);
-                    }
-                    if (def->hosts[i].port) {
-                        virBufferEscapeString(buf, " port='%s'",
-                                              def->hosts[i].port);
-                    }
-                    if (def->hosts[i].transport) {
-                        virBufferAsprintf(buf, " transport='%s'",
-                                          virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport));
-                    }
-                    if (def->hosts[i].socket) {
-                        virBufferEscapeString(buf, " socket='%s'", def->hosts[i].socket);
-                    }
-                    virBufferAddLit(buf, "/>\n");
-                }
-                virBufferAddLit(buf, "</source>\n");
-            }
-            break;
-        default:
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unexpected disk type %s"),
-                           virDomainDiskTypeToString(def->type));
-            return -1;
-        }
-    }
-
+    if (virDomainDiskSourceDefFormat(buf, def)<  0)
+        return -1;
      virDomainDiskGeometryDefFormat(buf, def);
      virDomainDiskBlockIoDefFormat(buf, def);

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9a9e0b1..9919db3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -427,6 +427,7 @@ enum virDomainDiskType {
      VIR_DOMAIN_DISK_TYPE_FILE,
      VIR_DOMAIN_DISK_TYPE_DIR,
      VIR_DOMAIN_DISK_TYPE_NETWORK,
+    VIR_DOMAIN_DISK_TYPE_VOLUME,

      VIR_DOMAIN_DISK_TYPE_LAST
  };
@@ -585,6 +586,13 @@ struct _virDomainBlockIoTuneInfo {
  };
  typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;

+typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef;
+struct _virDomainDiskSourcePoolDef {
+    char *pool; /* pool name */
+    char *volume; /* volume name */
+};
+typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
+
  /* Stores the virtual disk configuration */
  struct _virDomainDiskDef {
      int type;
@@ -596,6 +604,7 @@ struct _virDomainDiskDef {
      int protocol;
      size_t nhosts;
      virDomainDiskHostDefPtr hosts;
+    virDomainDiskSourcePoolDefPtr srcpool;
      struct {
          char *username;
          int secretType; /* enum virDomainDiskSecretType */
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
new file mode 100644
index 0000000..876eebe
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
@@ -0,0 +1,33 @@
+<domain type='qemu'>
+<name>QEMUGuest1</name>
+<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+<memory unit='KiB'>219136</memory>
+<currentMemory unit='KiB'>219136</currentMemory>
+<vcpu placement='static'>1</vcpu>
+<os>
+<type arch='i686' machine='pc'>hvm</type>
+<boot dev='hd'/>
+</os>
+<clock offset='utc'/>
+<on_poweroff>destroy</on_poweroff>
+<on_reboot>restart</on_reboot>
+<on_crash>destroy</on_crash>
+<devices>
+<emulator>/usr/bin/qemu</emulator>
+<disk type='volume' device='cdrom'>
+<source pool='blk-pool0' volume='blk-pool0-vol0'/>
+<target dev='hda' bus='ide'/>
+<readonly/>
+<address type='drive' controller='0' bus='0' target='0' unit='1'/>
+</disk>
+<disk type='file' device='disk'>
+<source file='/tmp/idedisk.img'/>
+<target dev='hdc' bus='ide'/>
+<address type='drive' controller='0' bus='0' target='0' unit='2'/>
+</disk>
+<controller type='usb' index='0'/>
+<controller type='ide' index='0'/>
+<controller type='ide' index='1'/>
+<memballoon model='virtio'/>
+</devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 160e958..7e174dd 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -241,6 +241,7 @@ mymain(void)
      DO_TEST("disk-scsi-lun-passthrough-sgio");

      DO_TEST("disk-scsi-disk-vpd");
+    DO_TEST("disk-source-pool");

      /* These tests generate different XML */
      DO_TEST_DIFFERENT("balloon-device-auto");


ACK - although my knowledge of the xml format/parse is still getting up to speed.

--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list


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