[libvirt] [PATCH 0/8 v2] Glue domain and storage
This is the 4th part to implement NPIV migration support [1]. Part 1 and part 2 are pushed. Part 3 (v3: pending for review): https://www.redhat.com/archives/libvir-list/2013-March/msg01440.html ====== This introduces new XMLs to specify the disk source with storage like <disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool="$pool_name" volume='$vol_name'/> <seclabel relabel='no'/> </source> <target dev='vdb' bus='virtio'/> </disk> And before domain starting, and disk attaching/updating, the source represented by storage is translated into the real underlying storage volume. Future work: * Support network volume * Support disk backing chain? v1 - v2: * Invoke storage APIs to translate the source directly in qemu driver * 1/8 in v1 is pushed. * 2/8 in v1 is splitted (the code refactoring now is a standalone patch) * Support shared disk for volume type disk * Support sgio setting for volume type disk * No network volume support in v2 RFC - v1: * The XMLs are more simpler - only using pool name and volume name to specify disk source. * Support network pool (rbd, and sheepdog) * Support startupPolicy for volume type disk * Support seclabels for volume type disk * Fix bugs on disk source formating Osier Yang (8): conf: New helper virDomainDiskSourceDefFormat to format the disk source Introduce new XMLs to specify disk source using libvirt storage qemu: Translate the pool disk source when building drive string Support startupPolicy for 'volume' disk Support seclabels for volume type disk qemu: Translate the pool disk source earlier qemu: Support shareable volume type disk qemu: Support sgio setting for volume type disk docs/formatdomain.html.in | 28 ++- docs/schemas/domaincommon.rng | 24 ++ src/conf/domain_conf.c | 274 ++++++++++++++------- src/conf/domain_conf.h | 10 + src/qemu/qemu_command.c | 32 +++ src/qemu/qemu_command.h | 1 - src/qemu/qemu_conf.c | 68 ++++- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 16 +- src/qemu/qemu_process.c | 14 +- .../qemuxml2argv-disk-source-pool.xml | 37 +++ tests/qemuxml2xmltest.c | 1 + 13 files changed, 406 insertions(+), 106 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml -- 1.8.1.4
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
On 04/04/2013 03:37 PM, Osier Yang wrote:
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(-)
ACK John
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 @@ <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> @@ -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
Il 04/04/2013 21:37, Osier Yang ha scritto:
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.
Depending on the pool type, this should be treated as file or block. You do this correctly in patch 8, but I think it is not complete. For example, will your patches allow SCSI passthrough of a volume inside an iSCSI pool? Perhaps libvirt should always work in terms of enum virStorageVolType, initializing it from either the pool or the "type" attribute. Paolo
On 05/04/13 03:15, Paolo Bonzini wrote:
Il 04/04/2013 21:37, Osier Yang ha scritto:
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. Depending on the pool type, this should be treated as file or block. You do this correctly in patch 8, but I think it is not complete. For example, will your patches allow SCSI passthrough of a volume inside an iSCSI pool?
No, it will be another patch set. On top of Han cheng's patches. I'm waiting for those patches go in first, or I will rebase/update it if Han doesn't have enough time to do it, and do the work after that.
Perhaps libvirt should always work in terms of enum virStorageVolType, initializing it from either the pool or the "type" attribute.
Paolo
Il 04/04/2013 21:40, Osier Yang ha scritto:
Depending on the pool type, this should be treated as file or block. You do this correctly in patch 8, but I think it is not complete. For example, will your patches allow SCSI passthrough of a volume inside an iSCSI pool?
No, it will be another patch set. On top of Han cheng's patches.
scsi-generic support for iSCSI pool would need his patches, but scsi-block shouldn't. My impression is that you already have "half baked" support for this feature in patches 7/8, and you should either complete it, or leave it out of this series. Paolo
I'm waiting for those patches go in first, or I will rebase/update it if Han doesn't have enough time to do it, and do the work after that.
On 05/04/13 14:53, Paolo Bonzini wrote:
Il 04/04/2013 21:40, Osier Yang ha scritto:
Depending on the pool type, this should be treated as file or block. You do this correctly in patch 8, but I think it is not complete. For example, will your patches allow SCSI passthrough of a volume inside an iSCSI pool? No, it will be another patch set. On top of Han cheng's patches. scsi-generic support for iSCSI pool would need his patches, but scsi-block shouldn't.
Oh, right, for a "lun" device, it's only valid for block type disk currently, so it can't work for a volume type disk (and the volume is of block type), I'm making the patch.
My impression is that you already have "half baked" support for this feature in patches 7/8, and you should either complete it, or leave it out of this series.
Paolo
I'm waiting for those patches go in first, or I will rebase/update it if Han doesn't have enough time to do it, and do the work after that.
On 04/04/2013 03:37 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
s/seclables/seclabels/
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
ACK, mechanically speaking what's here seems right - although I just get the feeling something is missing. I just don't have enough time yet to "just know" that the "<driver" and "<target" tags have a specific meaning and whether something else might be missing. The volume listed must be present in some pool and there's some sort of linkage that I don't see/get yet with this patch to validate that. Maybe this should have waited for the other patches you mentioned in the response to Paolo. My other hangup is my view of a volume is not a device like 'cdrom' rather it's more like /dev/sda# (on hpux it could have been /dev/rdisk/disk4). That is presented to the guest as a complete volume or of course we also allowed partitions. The example above using 'fc18.img' speaks "file" to me. John
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 @@ <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>
@@ -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");
On 06/04/13 06:24, John Ferlan wrote:
On 04/04/2013 03:37 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 s/seclables/seclabels/
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
ACK, mechanically speaking what's here seems right - although I just get the feeling something is missing. I just don't have enough time yet to "just know" that the "<driver" and "<target" tags have a specific meaning and whether something else might be missing.
The only things which could be missed is some tags could prevent volume type disk out of the door.
The volume listed must be present in some pool and there's some sort of linkage that I don't see/get yet with this patch to validate that
Maybe this should have waited for the other patches you mentioned in the response to Paolo.
Yeah, it should be done in driver, in later patches.
My other hangup is my view of a volume is not a device like 'cdrom' rather it's more like /dev/sda# (on hpux it could have been /dev/rdisk/disk4). That is presented to the guest as a complete volume or of course we also allowed partitions. The example above using 'fc18.img' speaks "file" to me.
Except network volume, all other types are supported, e.g. File, Dir, Block.
John
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 @@ <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>
@@ -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");
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/04/2013 03:37 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. 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 @@ <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>
@@ -1452,10 +1457,16 @@
Somewhere up here before this section there is: "The type attribute is either "file", "block", "dir", or "network" and refers to the underlying source for the disk." That list needs to be "updated" to include "volume". And naturally while not part of your changes - there's no example of "<disk type='dir'" in the above section... sigh. John
<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");
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/04/2013 03:37 PM, Osier Yang wrote:
This adds a new helper qemuTranslateDiskSourcePool which uses the storage pool/vol APIs to tranlsate the disk source before building
s/tranlsate/translate/
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.
How would 'network volume' differ from "<disk type='network'"? And all the variants therein? Still trying to figure the 'benefit' of the volume tag since each of the types looked up in the pool could also be specified as "<disk type='xxx'" types (where xxx is file, block, dir). But I'll press on with at least a mechanical review. Maybe someone else can chime in with the product level viewpoint.
--- 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;
Does initializing this matter? Ah perhaps the compiler complains...
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
consider "keep the compiler quiet" :-) although I understand the sentiment :-)
+ * 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) {
ACK, mechnically it seems what's here covers things. John
On 04/05/2013 05:24 PM, John Ferlan wrote:
On 04/04/2013 03:37 PM, Osier Yang wrote:
This adds a new helper qemuTranslateDiskSourcePool which uses the storage pool/vol APIs to tranlsate the disk source before building
s/tranlsate/translate/
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.
How would 'network volume' differ from "<disk type='network'"? And all the variants therein?
Still trying to figure the 'benefit' of the volume tag since each of the types looked up in the pool could also be specified as "<disk type='xxx'" types (where xxx is file, block, dir).
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.
But I'll press on with at least a mechanical review. Maybe someone else can chime in with the product level viewpoint.
--- 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;
Does initializing this matter? Ah perhaps the compiler complains...
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
consider "keep the compiler quiet" :-) although I understand the sentiment :-)
+ * 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) {
ACK, mechnically it seems what's here covers things.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/05/2013 07:32 PM, Eric Blake wrote:
On 04/05/2013 05:24 PM, John Ferlan wrote:
On 04/04/2013 03:37 PM, Osier Yang wrote:
This adds a new helper qemuTranslateDiskSourcePool which uses the storage pool/vol APIs to tranlsate the disk source before building
s/tranlsate/translate/
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.
How would 'network volume' differ from "<disk type='network'"? And all the variants therein?
Still trying to figure the 'benefit' of the volume tag since each of the types looked up in the pool could also be specified as "<disk type='xxx'" types (where xxx is file, block, dir).
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.
ahh.. something to consider putting in the domain html file as a reason to use volume as opposed to the other Of course I'd have concerns/thoughts that how do we "ensure" that volume "foo" in "bar" has the same storage considerations on both systems. I understand that's not for this code to decide, but migrating storage that isn't "shared" in some manner (whether that's via some adapter or nfs or something else not yet invented) means some sort of validation takes place on source and destination for "likeness". No sense in trying to migrate a 10g source to a 5g target. John
On 06/04/13 07:24, John Ferlan wrote:
On 04/04/2013 03:37 PM, Osier Yang wrote:
This adds a new helper qemuTranslateDiskSourcePool which uses the storage pool/vol APIs to tranlsate the disk source before building s/tranlsate/translate/
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.
How would 'network volume' differ from "<disk type='network'"? And all the variants therein?
Still trying to figure the 'benefit' of the volume tag since each of the types looked up in the pool could also be specified as "<disk type='xxx'" types (where xxx is file, block, dir).
Except Eric's feedback, see the discussion about "Migration with NPIV" here: http://www.redhat.com/archives/libvir-list/2012-November/msg00826.html
But I'll press on with at least a mechanical review. Maybe someone else can chime in with the product level viewpoint.
--- 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; Does initializing this matter? Ah perhaps the compiler complains...
0 means the VIR_STORAGE_VOL_FILE.
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
consider "keep the compiler quiet" :-) although I understand the sentiment :-)
That's more polite, changed. :-)
+ * 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) {
ACK, mechnically it seems what's here covers things.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
"startupPolicy" is only valid for file type storage volume, otherwise it fails on starting the domain. --- docs/formatdomain.html.in | 7 ++++--- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 9 +++++++-- src/qemu/qemu_command.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bdc815f..ce185a9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1467,11 +1467,12 @@ 0.7.5; <code>type='network'</code> since 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 + For a "file" or "volume" 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. - This is done by the <code>startupPolicy</code> attribute, accepting - these values: + (NB, <code>startupPolicy</code> is not valid for "volume" disk unless + the specified storage volume is of "file" type). This is done by the + <code>startupPolicy</code> attribute, accepting these values: <table class="top_table"> <tr> <td> mandatory </td> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46cccc4..4e7e712 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1103,6 +1103,9 @@ <attribute name="volume"> <ref name="volName"/> </attribute> + <optional> + <ref name="startupPolicy"/> + </optional> </element> </optional> <ref name="diskspec"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8538d5f..c1d2cbb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4180,6 +4180,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(cur, def) < 0) goto error; + startupPolicy = virXMLPropString(cur, "startupPolicy"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12890,9 +12891,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, /* 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", + if (def->srcpool) + virBufferAsprintf(buf, " <source pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'/>\n", startupPolicy); + else + virBufferAddLit(buf, "/>\n"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 03c7195..cdfe801 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2703,6 +2703,13 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, 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: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index 876eebe..a218e78 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu</emulator> <disk type='volume' device='cdrom'> - <source pool='blk-pool0' volume='blk-pool0-vol0'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'/> <target dev='hda' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> -- 1.8.1.4
On 04/04/2013 03:37 PM, Osier Yang wrote:
"startupPolicy" is only valid for file type storage volume, otherwise it fails on starting the domain. --- docs/formatdomain.html.in | 7 ++++--- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 9 +++++++-- src/qemu/qemu_command.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bdc815f..ce185a9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1467,11 +1467,12 @@ 0.7.5; <code>type='network'</code> since 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 + For a "file" or "volume" 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. - This is done by the <code>startupPolicy</code> attribute, accepting - these values: + (NB, <code>startupPolicy</code> is not valid for "volume" disk unless + the specified storage volume is of "file" type). This is done by the + <code>startupPolicy</code> attribute, accepting these values: <table class="top_table"> <tr> <td> mandatory </td> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46cccc4..4e7e712 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1103,6 +1103,9 @@ <attribute name="volume"> <ref name="volName"/> </attribute> + <optional> + <ref name="startupPolicy"/> + </optional> </element> </optional> <ref name="diskspec"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8538d5f..c1d2cbb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4180,6 +4180,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(cur, def) < 0) goto error; + startupPolicy = virXMLPropString(cur, "startupPolicy");
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
break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12890,9 +12891,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, /* 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", + if (def->srcpool) + virBufferAsprintf(buf, " <source pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'/>\n", startupPolicy); + else + virBufferAddLit(buf, "/>\n"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 03c7195..cdfe801 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2703,6 +2703,13 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, 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: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index 876eebe..a218e78 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu</emulator> <disk type='volume' device='cdrom'> - <source pool='blk-pool0' volume='blk-pool0-vol0'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'/> <target dev='hda' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/>
On 06/04/13 07:41, John Ferlan wrote:
On 04/04/2013 03:37 PM, Osier Yang wrote:
"startupPolicy" is only valid for file type storage volume, otherwise it fails on starting the domain. --- docs/formatdomain.html.in | 7 ++++--- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 9 +++++++-- src/qemu/qemu_command.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bdc815f..ce185a9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1467,11 +1467,12 @@ 0.7.5; <code>type='network'</code> since 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 + For a "file" or "volume" 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. - This is done by the <code>startupPolicy</code> attribute, accepting - these values: + (NB, <code>startupPolicy</code> is not valid for "volume" disk unless + the specified storage volume is of "file" type). This is done by the + <code>startupPolicy</code> attribute, accepting these values: <table class="top_table"> <tr> <td> mandatory </td> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46cccc4..4e7e712 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1103,6 +1103,9 @@ <attribute name="volume"> <ref name="volName"/> </attribute> + <optional> + <ref name="startupPolicy"/> + </optional> </element> </optional> <ref name="diskspec"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8538d5f..c1d2cbb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4180,6 +4180,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(cur, def) < 0) goto error; + startupPolicy = virXMLPropString(cur, "startupPolicy"); 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.
That's because we translate the source in the driver. Translating when parsing doesn't make sense, as inactive domain even might don't want to have the disk source existing.
John
break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -12890,9 +12891,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, /* 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", + if (def->srcpool) + virBufferAsprintf(buf, " <source pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'/>\n", startupPolicy); + else + virBufferAddLit(buf, "/>\n"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 03c7195..cdfe801 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2703,6 +2703,13 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, 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: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index 876eebe..a218e78 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu</emulator> <disk type='volume' device='cdrom'> - <source pool='blk-pool0' volume='blk-pool0-vol0'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'/> <target dev='hda' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/>
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
"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
On 04/04/2013 03:37 PM, Osier Yang wrote:
"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>
s/seclable/seclabel/
+ 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'/>
ACK, mechanically at least. Although unlike the startupPolicy code, there isn't any seclabel parsing code here. John
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 04/04/2013 03:38 PM, Osier Yang wrote:
To support "shareable" for volume type disk, we have to translate the source before trying to add the shared disk entry. To archieve
s/archeive/achieve/
the goal, this moves the helper qemuTranslateDiskSourcePool into src/qemu/qemu_conf.c, and introduce a internal only member (voltype)
s/a internal/an internal/
for struct _virDomainDiskSourcePoolDef, to record the underlying volume type, for use when building the drive string.
s/type,/type/
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,
ACK; however, I see paths to qemuBuildDriveStr() that don't go through qemuBuildCommandLine(), so just double check that you aren't missing a place to get that disk->src set for one of these pool/vol's. The point being that qemuBuildDriveStr() "had" code that did something before (albeit in this set of patches). Since you're moving it - just be sure there's no path that could enter qemuBuildDriveStr() that would need it. John
On 06/04/13 08:34, John Ferlan wrote:
On 04/04/2013 03:38 PM, Osier Yang wrote:
To support "shareable" for volume type disk, we have to translate the source before trying to add the shared disk entry. To archieve s/archeive/achieve/
the goal, this moves the helper qemuTranslateDiskSourcePool into src/qemu/qemu_conf.c, and introduce a internal only member (voltype) s/a internal/an internal/
for struct _virDomainDiskSourcePoolDef, to record the underlying volume type, for use when building the drive string. s/type,/type/
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,
ACK; however, I see paths to qemuBuildDriveStr() that don't go through qemuBuildCommandLine(), so just double check that you aren't missing a place to get that disk->src set for one of these pool/vol's. The point being that qemuBuildDriveStr() "had" code that did something before (albeit in this set of patches). Since you're moving it - just be sure there's no path that could enter qemuBuildDriveStr() that would need it.
Yeah, I expect there are still places missed, but as I replied for 0/8. Let's find them out step by step. Osier
Since the source is already translated before. This just adds the checking. Move !disk->shared and !disk->src to improve the performance a bit. --- src/qemu/qemu_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8ae690f..c7b8045 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1113,8 +1113,12 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, * for the shared disk is "sgio" setting, which is only * valid for block disk. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || - !disk->shared || !disk->src) + if (!disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0; qemuDriverLock(driver); @@ -1189,8 +1193,12 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, int ret = -1; int idx; - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || - !disk->shared || !disk->src) + if (!disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0; qemuDriverLock(driver); -- 1.8.1.4
On 04/04/2013 03:38 PM, Osier Yang wrote:
Since the source is already translated before. This just adds the checking. Move !disk->shared and !disk->src to improve the performance a bit. --- src/qemu/qemu_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
ACK, although my eyes hurt thinking about all those nots and ors to with an and... John
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8ae690f..c7b8045 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1113,8 +1113,12 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, * for the shared disk is "sgio" setting, which is only * valid for block disk. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || - !disk->shared || !disk->src) + if (!disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0;
qemuDriverLock(driver); @@ -1189,8 +1193,12 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, int ret = -1; int idx;
- if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || - !disk->shared || !disk->src) + if (!disk->shared || + !disk->src || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0;
qemuDriverLock(driver);
--- src/qemu/qemu_process.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3ac0c70..757f8b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3235,9 +3235,12 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + if (!disk->src || disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - !disk->src) + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0; sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL); -- 1.8.1.4
On 05/04/13 03:38, Osier Yang wrote:
--- src/qemu/qemu_process.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3ac0c70..757f8b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3235,9 +3235,12 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + if (!disk->src || disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - !disk->src) + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0;
sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL);
With the following missed diff squashed in:
On 04/04/2013 03:38 PM, Osier Yang wrote:
--- src/qemu/qemu_process.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Perhaps a MACRO would be better to describe: "!(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME &&
+ disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) "
Squeeze that into 7/8 and use it here. Just a thought as the whole construct is tough to read. ACK, in any case. John
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3ac0c70..757f8b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3235,9 +3235,12 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + if (!disk->src || disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - !disk->src) + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0;
sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL);
On 06/04/13 08:44, John Ferlan wrote:
On 04/04/2013 03:38 PM, Osier Yang wrote:
--- src/qemu/qemu_process.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Perhaps a MACRO would be better to describe:
"!(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME &&
+ disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK)
Sounds good, I will consider to create it in later patches.
"
Squeeze that into 7/8 and use it here.
Just a thought as the whole construct is tough to read.
ACK, in any case.
John
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3ac0c70..757f8b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3235,9 +3235,12 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. */ - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + if (!disk->src || disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - !disk->src) + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) return 0;
sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL);
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Like 7/8, 8/8, 9/8, there must be many places which also should be modified to not prevent the new volume type disk out of the door, but it's hard to find them out at once, disk related stuffs are always hairy. Anyway, it doesn't affect to let the basic go in first, because they are all additions, which are unlikely to introduce regressions. so please give reviewing. Thanks. On 05/04/13 03:37, Osier Yang wrote:
This is the 4th part to implement NPIV migration support [1].
Part 1 and part 2 are pushed.
Part 3 (v3: pending for review): https://www.redhat.com/archives/libvir-list/2013-March/msg01440.html
======
This introduces new XMLs to specify the disk source with storage like
<disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool="$pool_name" volume='$vol_name'/> <seclabel relabel='no'/> </source> <target dev='vdb' bus='virtio'/> </disk>
And before domain starting, and disk attaching/updating, the source represented by storage is translated into the real underlying storage volume.
Future work: * Support network volume * Support disk backing chain?
v1 - v2: * Invoke storage APIs to translate the source directly in qemu driver * 1/8 in v1 is pushed. * 2/8 in v1 is splitted (the code refactoring now is a standalone patch) * Support shared disk for volume type disk * Support sgio setting for volume type disk * No network volume support in v2
RFC - v1: * The XMLs are more simpler - only using pool name and volume name to specify disk source. * Support network pool (rbd, and sheepdog) * Support startupPolicy for volume type disk * Support seclabels for volume type disk * Fix bugs on disk source formating
Osier Yang (8): conf: New helper virDomainDiskSourceDefFormat to format the disk source Introduce new XMLs to specify disk source using libvirt storage qemu: Translate the pool disk source when building drive string Support startupPolicy for 'volume' disk Support seclabels for volume type disk qemu: Translate the pool disk source earlier qemu: Support shareable volume type disk qemu: Support sgio setting for volume type disk
docs/formatdomain.html.in | 28 ++- docs/schemas/domaincommon.rng | 24 ++ src/conf/domain_conf.c | 274 ++++++++++++++------- src/conf/domain_conf.h | 10 + src/qemu/qemu_command.c | 32 +++ src/qemu/qemu_command.h | 1 - src/qemu/qemu_conf.c | 68 ++++- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 16 +- src/qemu/qemu_process.c | 14 +- .../qemuxml2argv-disk-source-pool.xml | 37 +++ tests/qemuxml2xmltest.c | 1 + 13 files changed, 406 insertions(+), 106 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
On 04/05/2013 12:30 PM, Osier Yang wrote:
Like 7/8, 8/8, 9/8, there must be many places which also should be modified to not prevent the new volume type disk out of the door, but it's hard to find them out at once, disk related stuffs are always hairy. Anyway, it doesn't affect to let the basic go in first, because they are all additions, which are unlikely to introduce regressions. so please give reviewing. Thanks.
Better to find them now though, right! Seems you could use cscope to search on uses of VIR_DOMAIN_DISK_TYPE_BLOCK and others to ensure you cover as many cases as possible. John
On 05/04/13 03:37, Osier Yang wrote:
This is the 4th part to implement NPIV migration support [1].
Part 1 and part 2 are pushed.
Part 3 (v3: pending for review): https://www.redhat.com/archives/libvir-list/2013-March/msg01440.html
======
This introduces new XMLs to specify the disk source with storage like
<disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool="$pool_name" volume='$vol_name'/> <seclabel relabel='no'/> </source> <target dev='vdb' bus='virtio'/> </disk>
And before domain starting, and disk attaching/updating, the source represented by storage is translated into the real underlying storage volume.
Future work: * Support network volume * Support disk backing chain?
v1 - v2: * Invoke storage APIs to translate the source directly in qemu driver * 1/8 in v1 is pushed. * 2/8 in v1 is splitted (the code refactoring now is a standalone patch) * Support shared disk for volume type disk * Support sgio setting for volume type disk * No network volume support in v2 RFC - v1: * The XMLs are more simpler - only using pool name and volume name to specify disk source. * Support network pool (rbd, and sheepdog) * Support startupPolicy for volume type disk * Support seclabels for volume type disk * Fix bugs on disk source formating
Osier Yang (8): conf: New helper virDomainDiskSourceDefFormat to format the disk source Introduce new XMLs to specify disk source using libvirt storage qemu: Translate the pool disk source when building drive string Support startupPolicy for 'volume' disk Support seclabels for volume type disk qemu: Translate the pool disk source earlier qemu: Support shareable volume type disk qemu: Support sgio setting for volume type disk
docs/formatdomain.html.in | 28 ++- docs/schemas/domaincommon.rng | 24 ++ src/conf/domain_conf.c | 274 ++++++++++++++------- src/conf/domain_conf.h | 10 + src/qemu/qemu_command.c | 32 +++ src/qemu/qemu_command.h | 1 - src/qemu/qemu_conf.c | 68 ++++- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 16 +- src/qemu/qemu_process.c | 14 +- .../qemuxml2argv-disk-source-pool.xml | 37 +++ tests/qemuxml2xmltest.c | 1 + 13 files changed, 406 insertions(+), 106 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This allows one use block type volume as the disk source for device 'lun'. --- src/qemu/qemu_command.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c29796d..2fc63ae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3131,10 +3131,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskProtocolTypeToString(disk->protocol)); goto error; } - } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk device='lun' is not supported for type='%s'"), - virDomainDiskTypeToString(disk->type)); + } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is only valid for block type disk source")); goto error; } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) { -- 1.8.1.4
On 04/05/2013 01:07 PM, Osier Yang wrote:
This allows one use block type volume as the disk source for device 'lun'. --- src/qemu/qemu_command.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
ACK - seems reasonable. John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c29796d..2fc63ae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3131,10 +3131,11 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskProtocolTypeToString(disk->protocol)); goto error; } - } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk device='lun' is not supported for type='%s'"), - virDomainDiskTypeToString(disk->type)); + } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is only valid for block type disk source")); goto error; } if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) {
Pushed with the nits fixed, as these patches just do additions, it's unlikely to introduce regression. Future patches will resolve the places which prevent volume type disk out of the door. Thanks for the reviewing! Osier On 05/04/13 03:37, Osier Yang wrote:
This is the 4th part to implement NPIV migration support [1].
Part 1 and part 2 are pushed.
Part 3 (v3: pending for review): https://www.redhat.com/archives/libvir-list/2013-March/msg01440.html
======
This introduces new XMLs to specify the disk source with storage like
<disk type='volume' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source pool="$pool_name" volume='$vol_name'/> <seclabel relabel='no'/> </source> <target dev='vdb' bus='virtio'/> </disk>
And before domain starting, and disk attaching/updating, the source represented by storage is translated into the real underlying storage volume.
Future work: * Support network volume * Support disk backing chain?
v1 - v2: * Invoke storage APIs to translate the source directly in qemu driver * 1/8 in v1 is pushed. * 2/8 in v1 is splitted (the code refactoring now is a standalone patch) * Support shared disk for volume type disk * Support sgio setting for volume type disk * No network volume support in v2
RFC - v1: * The XMLs are more simpler - only using pool name and volume name to specify disk source. * Support network pool (rbd, and sheepdog) * Support startupPolicy for volume type disk * Support seclabels for volume type disk * Fix bugs on disk source formating
Osier Yang (8): conf: New helper virDomainDiskSourceDefFormat to format the disk source Introduce new XMLs to specify disk source using libvirt storage qemu: Translate the pool disk source when building drive string Support startupPolicy for 'volume' disk Support seclabels for volume type disk qemu: Translate the pool disk source earlier qemu: Support shareable volume type disk qemu: Support sgio setting for volume type disk
docs/formatdomain.html.in | 28 ++- docs/schemas/domaincommon.rng | 24 ++ src/conf/domain_conf.c | 274 ++++++++++++++------- src/conf/domain_conf.h | 10 + src/qemu/qemu_command.c | 32 +++ src/qemu/qemu_command.h | 1 - src/qemu/qemu_conf.c | 68 ++++- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 16 +- src/qemu/qemu_process.c | 14 +- .../qemuxml2argv-disk-source-pool.xml | 37 +++ tests/qemuxml2xmltest.c | 1 + 13 files changed, 406 insertions(+), 106 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
participants (4)
-
Eric Blake -
John Ferlan -
Osier Yang -
Paolo Bonzini