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. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list