[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 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) {"?

> +
> +    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.

> +            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.


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