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