[libvirt] [PATCH 2/9] Introduce new XMLs to specify disk source using libvirt storage
Osier Yang
jyang at redhat.com
Thu Jan 31 17:25:29 UTC 2013
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 @@
>> <blockio logical_block_size='512' physical_block_size='4096'/>
>> <target dev='hda' bus='ide'/>
>> </disk>
>> +<disk type='volume' device='disk'>
>> +<driver name='qemu' type='raw'/>
>> +<source pool='blk-pool0' volume='blk-pool0-vol0'/>
>> +<target dev='hda' bus='ide'/>
>> +</disk>
>> </devices>
>> ...</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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list