The code to format disk source is long enough to have a helper. --- src/conf/domain_conf.c | 174 ++++++++++++++++++++++++++----------------------- 1 file changed, 92 insertions(+), 82 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cc26f21..8288959 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12716,6 +12716,7 @@ virDomainDiskGeometryDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } } + static void virDomainDiskBlockIoDefFormat(virBufferPtr buf, virDomainDiskDefPtr def) @@ -12737,6 +12738,95 @@ 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->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; + } + } + + return 0; +} static int virDomainDiskDefFormat(virBufferPtr buf, @@ -12753,10 +12843,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) { @@ -12855,86 +12943,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); -- 1.8.1.4

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. tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml: tests/qemuxml2xmltest.c: * New test --- docs/formatdomain.html.in | 15 +++- docs/schemas/domaincommon.rng | 18 +++++ src/conf/domain_conf.c | 89 ++++++++++++++++++++-- src/conf/domain_conf.h | 9 +++ .../qemuxml2argv-disk-source-pool.xml | 33 ++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cf382e8..bdc815f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1372,6 +1372,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> @@ -1452,10 +1457,16 @@ <code>iqn.1992-01.com.example/1</code>); the default LUN is zero. 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. + specify the hosts 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; <code>protocol='iscsi'</code> since 1.0.4</span><br/> + 0.8.7; <code>protocol='iscsi'</code> since 1.0.4; + <code>type='volume'</code> since 1.0.5;</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 454ebdb..46cccc4 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1090,6 +1090,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 8288959..8538d5f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -207,7 +207,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", @@ -1085,6 +1086,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; @@ -1094,6 +1107,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); @@ -3935,6 +3949,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) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'pool' and 'volume' must be specified together " + "for 'pool' type source")); + goto cleanup; + } + + 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 @@ -4030,7 +4084,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; @@ -4123,6 +4177,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"), @@ -4421,7 +4479,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 == NULL && hosts == NULL && !def->srcpool && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { virReportError(VIR_ERR_NO_SOURCE, @@ -4443,8 +4501,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; } @@ -12745,7 +12814,7 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, int n; const char *startupPolicy = virDomainStartupPolicyTypeToString(def->startupPolicy); - if (def->src || def->nhosts > 0 || + if (def->src || def->nhosts > 0 || def->srcpool || def->startupPolicy) { switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: @@ -12817,6 +12886,14 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, 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"), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index edddf25..988636e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -447,6 +447,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 }; @@ -606,6 +607,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; @@ -617,6 +625,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 ba9aa96..fa79960 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -252,6 +252,7 @@ mymain(void) DO_TEST("disk-scsi-lun-passthrough-sgio"); DO_TEST("disk-scsi-disk-vpd"); + DO_TEST("disk-source-pool"); DO_TEST("virtio-rng-random"); DO_TEST("virtio-rng-egd"); -- 1.8.1.4

On 06/04/13 06:24, John Ferlan wrote: The only things which could be missed is some tags could prevent volume type disk out of the door. Yeah, it should be done in driver, in later patches. Except network volume, all other types are supported, e.g. File, Dir, Block.

This adds a new helper qemuTranslateDiskSourcePool which uses the storage pool/vol APIs to tranlsate the disk source before building the drive string. Network volume is not supported yet. Disk chain for volume type disk may be supported later, but before I'm confident it doesn't break anything, it's just disabled now. --- src/qemu/qemu_command.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 4 ++- 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 693d30d..03c7195 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2681,6 +2681,49 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op return 0; } +static int +qemuTranslateDiskSourcePool(virConnectPtr conn, + virDomainDiskDefPtr def, + int *voltype) +{ + virStoragePoolPtr pool = NULL; + virStorageVolPtr vol = NULL; + virStorageVolInfo info; + int ret = -1; + + if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) + return 0; + + if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool))) + return -1; + + if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume))) + goto cleanup; + + if (virStorageVolGetInfo(vol, &info) < 0) + goto cleanup; + + switch (info.type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + break; + case VIR_STORAGE_VOL_NETWORK: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Using network volume as disk source is not supported")); + goto cleanup; + } + + *voltype = info.type; + ret = 0; +cleanup: + virStoragePoolFree(pool); + virStorageVolFree(vol); + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2693,6 +2736,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskGeometryTransTypeToString(disk->geometry.trans); int idx = virDiskNameToIndex(disk->dst); int busid = -1, unitid = -1; + int voltype = -1; if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2700,6 +2744,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; } + if (qemuTranslateDiskSourcePool(conn, disk, &voltype) < 0) + goto error; + switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { @@ -2834,6 +2881,38 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } break; } + } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { + switch (voltype) { + case VIR_STORAGE_VOL_DIR: + if (!disk->readonly) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot create virtual FAT disks in read-write mode")); + goto error; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + virBufferEscape(&opt, ',', ",", "file=fat:floppy:%s,", + disk->src); + else + virBufferEscape(&opt, ',', ",", "file=fat:%s,", disk->src); + break; + case VIR_STORAGE_VOL_BLOCK: + if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("tray status 'open' is invalid for " + "block type volume")); + goto error; + } + virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); + break; + case VIR_STORAGE_VOL_FILE: + virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); + break; + case VIR_STORAGE_VOL_NETWORK: + /* Let the compiler shutup, qemuTranslateDiskSourcePool already + * reported the unsupported error. + */ + break; + } } else { if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) && (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c79b05d..6762152 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1943,7 +1943,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = 0; - if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (!disk->src || + disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || + disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) goto cleanup; if (disk->backingChain) { -- 1.8.1.4

On 04/05/2013 05:24 PM, John Ferlan wrote: One huge benefit of pool designations is that you can specify a domain that uses volume "foo" within pool "bar", and migrate it between two machines where pool "bar" has two different locations for the two machines (maybe /mnt/nfs/bar on one machine and /longer/path/to/bar on the other), and the XML on the destination will automatically find the absolute path to foo relative to the pool bar. If you specify with disk type='file', you have to specify an absolute path in your XML, and on migration, if the destination doesn't have the same absolute path, you are forced to pass alternate xml that you manage yourself to touch up the path as part of the migration. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 06/04/13 07:24, John Ferlan wrote: Except Eric's feedback, see the discussion about "Migration with NPIV" here: http://www.redhat.com/archives/libvir-list/2012-November/msg00826.html 0 means the VIR_STORAGE_VOL_FILE. That's more polite, changed. :-)

On 04/04/2013 03:37 PM, Osier Yang wrote: Is there no way later on in this module to validate that the startupPolicy for a DISK_TYPE_VOLUME is for a file only? Seems a shame to wait for building the qemu command to tell someone the bad news. John

"seclabels" is only valid for 'file' or 'block' type storage volume. --- docs/formatdomain.html.in | 6 ++++-- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 20 ++++++++++++++------ .../qemuxml2argv-disk-source-pool.xml | 6 +++++- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ce185a9..44e7f28 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1439,11 +1439,13 @@ path to the file holding the disk. If the disk <code>type</code> is "block", then the <code>dev</code> attribute specifies the path to the host device to serve as - the disk. With both "file" and "block", one or more optional + the disk. With "file", "block", and "volume", one or more optional sub-elements <code>seclabel</code>, <a href="#seclabel">described below</a> (and <span class="since">since 0.9.9</span>), can be used to override the domain security labeling policy for just - that source file. If the disk <code>type</code> is "dir", then the + that source file. (NB, for "volume" type disk, <code>seclable</code> + is only valid when the specified storage volume is of 'file' or + 'block' type). If the disk <code>type</code> is "dir", then the <code>dir</code> attribute specifies the fully-qualified path to the directory to use as the disk. If the disk <code>type</code> is "network", then the <code>protocol</code> attribute specifies diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4e7e712..80749e1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1106,6 +1106,9 @@ <optional> <ref name="startupPolicy"/> </optional> + <optional> + <ref name='devSeclabel'/> + </optional> </element> </optional> <ref name="diskspec"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c1d2cbb..7f5e727 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12888,16 +12888,24 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, } break; case VIR_DOMAIN_DISK_TYPE_VOLUME: - /* Parsing guarantees the def->srcpool->volume cannot be NULL - * if def->srcpool->pool is not NULL. - */ + virBufferAddLit(buf, " <source"); + if (def->srcpool) - virBufferAsprintf(buf, " <source pool='%s' volume='%s'", + virBufferAsprintf(buf, " pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); if (def->startupPolicy) - virBufferEscapeString(buf, " startupPolicy='%s'/>\n", startupPolicy); - else + 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; default: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index a218e78..acf9753 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -15,7 +15,11 @@ <devices> <emulator>/usr/bin/qemu</emulator> <disk type='volume' device='cdrom'> - <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> <target dev='hda' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> -- 1.8.1.4

To support "shareable" for volume type disk, we have to translate the source before trying to add the shared disk entry. To archieve the goal, this moves the helper qemuTranslateDiskSourcePool into src/qemu/qemu_conf.c, and introduce a internal only member (voltype) for struct _virDomainDiskSourcePoolDef, to record the underlying volume type, for use when building the drive string. Later patch will support "shareable" volume type disk. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 56 +------------------------------------------------ src/qemu/qemu_command.h | 1 - src/qemu/qemu_conf.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_driver.c | 16 ++++++++++---- src/qemu/qemu_process.c | 7 +++++++ 7 files changed, 76 insertions(+), 60 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 988636e..13d6d89 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -611,6 +611,7 @@ typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef; struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ + int voltype; /* enum virStorageVolType, internal only */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdfe801..c29796d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2681,56 +2681,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op return 0; } -static int -qemuTranslateDiskSourcePool(virConnectPtr conn, - virDomainDiskDefPtr def, - int *voltype) -{ - virStoragePoolPtr pool = NULL; - virStorageVolPtr vol = NULL; - virStorageVolInfo info; - int ret = -1; - - if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) - return 0; - - if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool))) - return -1; - - if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume))) - goto cleanup; - - if (virStorageVolGetInfo(vol, &info) < 0) - goto cleanup; - - if (def->startupPolicy && - info.type != VIR_STORAGE_VOL_FILE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for 'file' type volume")); - goto cleanup; - } - - switch (info.type) { - case VIR_STORAGE_VOL_FILE: - case VIR_STORAGE_VOL_BLOCK: - case VIR_STORAGE_VOL_DIR: - if (!(def->src = virStorageVolGetPath(vol))) - goto cleanup; - break; - case VIR_STORAGE_VOL_NETWORK: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Using network volume as disk source is not supported")); - goto cleanup; - } - - *voltype = info.type; - ret = 0; -cleanup: - virStoragePoolFree(pool); - virStorageVolFree(vol); - return ret; -} - char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2743,7 +2693,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskGeometryTransTypeToString(disk->geometry.trans); int idx = virDiskNameToIndex(disk->dst); int busid = -1, unitid = -1; - int voltype = -1; if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2751,9 +2700,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; } - if (qemuTranslateDiskSourcePool(conn, disk, &voltype) < 0) - goto error; - switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { @@ -2889,7 +2835,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, break; } } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { - switch (voltype) { + switch (disk->srcpool->voltype) { case VIR_STORAGE_VOL_DIR: if (!disk->readonly) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 17687f4..20e3066 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -237,5 +237,4 @@ qemuParseKeywords(const char *str, char ***retvalues, int allowEmptyValue); - #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c2e2e10..8ae690f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1240,3 +1240,55 @@ int qemuDriverAllocateID(virQEMUDriverPtr driver) { return virAtomicIntInc(&driver->nextvmid); } + +int +qemuTranslateDiskSourcePool(virConnectPtr conn, + virDomainDiskDefPtr def) +{ + virStoragePoolPtr pool = NULL; + virStorageVolPtr vol = NULL; + virStorageVolInfo info; + int ret = -1; + + if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) + return 0; + + if (!def->srcpool) + return 0; + + if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool))) + return -1; + + if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume))) + goto cleanup; + + if (virStorageVolGetInfo(vol, &info) < 0) + goto cleanup; + + if (def->startupPolicy && + info.type != VIR_STORAGE_VOL_FILE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'startupPolicy' is only valid for 'file' type volume")); + goto cleanup; + } + + switch (info.type) { + case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_BLOCK: + case VIR_STORAGE_VOL_DIR: + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + break; + case VIR_STORAGE_VOL_NETWORK: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Using network volume as disk source is not supported")); + goto cleanup; + } + + def->srcpool->voltype = info.type; + ret = 0; +cleanup: + virStoragePoolFree(pool); + virStorageVolFree(vol); + return ret; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c5ddaad..9f58454 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -303,4 +303,7 @@ void qemuSharedDiskEntryFree(void *payload, const void *name) int qemuDriverAllocateID(virQEMUDriverPtr driver); virDomainXMLConfPtr virQEMUDriverCreateXMLConf(void); +int qemuTranslateDiskSourcePool(virConnectPtr conn, + virDomainDiskDefPtr def); + #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 552a81b..4e8c666 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5748,6 +5748,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } + if (qemuTranslateDiskSourcePool(conn, disk) < 0) + goto end; + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) goto end; @@ -6016,7 +6019,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, } static int -qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, +qemuDomainChangeDiskMediaLive(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev, virQEMUDriverPtr driver, bool force) @@ -6029,6 +6033,9 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, virCapsPtr caps = NULL; int ret = -1; + if (qemuTranslateDiskSourcePool(conn, disk) < 0) + goto end; + if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) goto end; @@ -6107,7 +6114,8 @@ end: } static int -qemuDomainUpdateDeviceLive(virDomainObjPtr vm, +qemuDomainUpdateDeviceLive(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev, virDomainPtr dom, bool force) @@ -6117,7 +6125,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainChangeDiskMediaLive(vm, dev, driver, force); + ret = qemuDomainChangeDiskMediaLive(conn, vm, dev, driver, force); break; case VIR_DOMAIN_DEVICE_GRAPHICS: ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics); @@ -6529,7 +6537,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom); break; case QEMU_DEVICE_UPDATE: - ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force); + ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8c4bfb7..3ac0c70 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3008,6 +3008,8 @@ qemuProcessReconnect(void *opaque) * qemu_driver->sharedDisks. */ for (i = 0; i < obj->def->ndisks; i++) { + if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) + goto error; if (qemuAddSharedDisk(driver, obj->def->disks[i], obj->def->name) < 0) goto error; @@ -3556,6 +3558,11 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) + goto cleanup; + } + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, -- 1.8.1.4

On 06/04/13 08:34, John Ferlan wrote: Yeah, I expect there are still places missed, but as I replied for 0/8. Let's find them out step by step. Osier

On 04/04/2013 03:38 PM, Osier Yang wrote: ACK, although my eyes hurt thinking about all those nots and ors to with an and... John

On 05/04/13 03:38, Osier Yang wrote: With the following missed diff squashed in:

On 06/04/13 08:44, John Ferlan wrote: Sounds good, I will consider to create it in later patches.

On 04/05/2013 01:07 PM, Osier Yang wrote: ACK - seems reasonable. John