[libvirt] [PATCHv2 0/9] RelaxNG improvements for virStorageSource
changes since v1: ack'd patch pushed new patches 2-4 are a better rewrite of old patch 3/6 new patches 8 and 9 patch 9 is incomplete - I know where I want to be headed, but changing 400+ tests is taking a while. I tried: $ git grep -l '<driver.*type=' tests/*data/ | \ xargs sed ' /<driver .*type=\(\(.\)[a-zA-Z0-9]*\2\)/ h; /<source /{ x; s/<driver.*type=\(\(.\)[a-zA-Z0-9]*\2\).*>/<format type=\1>/; G; }' as a way to automate the majority of the tedium, with some limited success (it has false positives in the lxc <filesystem> elements, when we really only want to affect <disk> elements>), but have not completed the efforts. I'm posting the patches now, in the hopes that 1-8 can be approved, and 9 picked up by someone else if I'm unresponsive for a while (I'll be offline for much of this week due to the birth of my fourth child). Eric Blake (9): conf: better <disk> interleaving in schema conf: move storage formats to common RNG file conf: restrict external snapshots to backing store formats conf: set up for per-grammar overrides in schemas conf: split <disk> schema into more pieces conf: move storage source details to common RNG file conf: move <auth> and <encryption> to disk source conf: fix omission of <driver> in domain dumpxml RFC: conf: introduce <format> as synonym of <driver type=...> docs/formatdomain.html.in | 19 +- docs/schemas/domain.rng | 14 +- docs/schemas/domaincommon.rng | 288 ++------------------ docs/schemas/domainsnapshot.rng | 74 ++---- docs/schemas/storagecommon.rng | 293 +++++++++++++++++++++ docs/schemas/storagevol.rng | 13 +- src/conf/domain_conf.c | 36 ++- src/conf/snapshot_conf.c | 7 +- src/util/virstoragefile.c | 51 ++-- src/util/virstoragefile.h | 15 +- .../qemuxml2argv-disk-drive-copy-on-read.args | 5 + .../qemuxml2argv-disk-drive-copy-on-read.xml | 28 ++ .../qemuxml2argv-disk-drive-discard.xml | 10 +- .../qemuxml2argv-disk-source-pool.xml | 12 +- tests/qemuxml2argvtest.c | 3 + .../qemuxml2xmlout-disk-drive-discard.xml | 37 +++ .../qemuxml2xmlout-disk-source-pool.xml | 44 ++++ tests/qemuxml2xmltest.c | 5 +- 18 files changed, 574 insertions(+), 380 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml -- 1.9.0
In general, we try to make virt-xml-validate tolerant of input elements in any order when possible. However, as written, the RNG grammar did not permit <source> unless there was an explicit type= attribute (even though the C code manages just fine by defaulting to type='file'). After making the attribute optional on the 'file' branch, I noticed that the use of diskspec was now redundant with the branch when no <source> was supplied. View this patch with 'git diff -b' for a better picture of the schema change. * docs/schemas/domaincommon.rng (disk): Hoist 'diskspec' out of choice, make type='file' default, and still preserve interleave. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml: * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml: New files. * tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml: * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml: Reorder XML. * tests/qemuxml2xmltest.c (mymain): Cover new files. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 213 ++++++++++----------- .../qemuxml2argv-disk-drive-discard.xml | 8 +- .../qemuxml2argv-disk-source-pool.xml | 12 +- .../qemuxml2xmlout-disk-drive-discard.xml | 37 ++++ .../qemuxml2xmlout-disk-source-pool.xml | 44 +++++ tests/qemuxml2xmltest.c | 4 +- 6 files changed, 201 insertions(+), 117 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8c1724a..7fc0cff 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1196,119 +1196,118 @@ <optional> <ref name="snapshot"/> </optional> - <choice> - <group> - <attribute name="type"> - <value>file</value> - </attribute> - <interleave> + <interleave> + <choice> + <group> <optional> - <element name="source"> - <optional> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="startupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> + <attribute name="type"> + <value>file</value> + </attribute> </optional> - <ref name="diskspec"/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="dev"> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="startupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + </interleave> + </group> + <group> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="dev"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="startupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + </interleave> + </group> + <group> + <attribute name="type"> + <value>dir</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dir"> <ref name="absFilePath"/> </attribute> - </optional> - <optional> - <ref name="startupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>dir</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dir"> - <ref name="absFilePath"/> - </attribute> - <optional> - <ref name="startupPolicy"/> - </optional> - <empty/> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>network</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <ref name='diskSourceNetwork'/> - </element> - </optional> - <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> - <optional> - <attribute name="mode"> - <choice> - <value>host</value> - <value>direct</value> - </choice> + <optional> + <ref name="startupPolicy"/> + </optional> + <empty/> + </element> + </optional> + </interleave> + </group> + <group> + <attribute name="type"> + <value>network</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <ref name='diskSourceNetwork'/> + </element> + </optional> + </interleave> + </group> + <group> + <attribute name="type"> + <value>volume</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="pool"> + <ref name="genericName"/> </attribute> - </optional> - <optional> - <ref name="startupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> + <attribute name="volume"> + <ref name="volName"/> + </attribute> + <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>direct</value> + </choice> + </attribute> + </optional> + <optional> + <ref name="startupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + </interleave> + </group> + </choice> <ref name="diskspec"/> - </choice> + </interleave> </element> </define> <define name="diskSourceNetwork"> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml index f01312f..b15fd63 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml @@ -16,11 +16,13 @@ <on_crash>restart</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> - <disk type='file' device='disk'> - <driver name='qemu' type='qcow2' discard='unmap'/> + <!-- For this disk, intentionally stress parser resilience to + atypical ordering --> + <disk device='disk'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> <source file='/var/lib/libvirt/images/f14.img'/> <target dev='vda' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + <driver discard='unmap' name='qemu' type='qcow2'/> </disk> <disk type='file' device='cdrom'> <driver name='qemu' type='raw' discard='ignore'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index e96f76e..95d5be2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -14,15 +14,17 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> - <disk type='volume' device='cdrom'> - <source pool='pool-disk' volume='block+cdrom'> + <!-- For this disk, intentionally stress parser resilience to + atypical ordering --> + <disk device='cdrom' type='volume'> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + <readonly/> + <target bus='ide' dev='hda'/> + <source volume='block+cdrom' pool='pool-disk'> <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'/> </disk> <disk type='volume' device='cdrom'> <driver name='qemu' type='raw'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml new file mode 100644 index 0000000..f01312f --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>test</name> + <uuid>92d7a226-cfae-425b-a6d3-00bbf9ec5c9e</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.13'>hvm</type> + <boot dev='cdrom'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' discard='unmap'/> + <source file='/var/lib/libvirt/images/f14.img'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw' discard='ignore'/> + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml new file mode 100644 index 0000000..e96f76e --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml @@ -0,0 +1,44 @@ +<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='pool-disk' volume='block+cdrom'> + <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'/> + </disk> + <disk type='volume' device='cdrom'> + <driver name='qemu' type='raw'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </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'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c8a1c10..788adbe 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -282,10 +282,10 @@ mymain(void) DO_TEST("disk-scsi-lun-passthrough-sgio"); DO_TEST("disk-scsi-disk-vpd"); - DO_TEST("disk-source-pool"); + DO_TEST_DIFFERENT("disk-source-pool"); DO_TEST("disk-source-pool-mode"); - DO_TEST("disk-drive-discard"); + DO_TEST_DIFFERENT("disk-drive-discard"); DO_TEST("virtio-rng-random"); DO_TEST("virtio-rng-egd"); -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:12 -0600, Eric Blake wrote:
In general, we try to make virt-xml-validate tolerant of input elements in any order when possible. However, as written, the RNG grammar did not permit <source> unless there was an explicit type= attribute (even though the C code manages just fine by defaulting to type='file'). After making the attribute optional on the 'file' branch, I noticed that the use of diskspec was now redundant with the branch when no <source> was supplied.
View this patch with 'git diff -b' for a better picture of the schema change.
I did that and also checked the result. ACK Jirka
We had incomplete RelaxNG support for storage formats listed in virstoragefile.h: commit 027bf2e added 'vdi' but forgot to update the <volume> and <domain> xml lists; the <volume> list was also missing 'fat' and 'vhd'. Maintaining two lists is a recipe for them getting out of sync, so make the list common so that both contexts benefit the next time we add a format in a single location. * docs/schemas/domaincommon.rng (storageFormat): Move... * docs/schemas/storagecommon.rng: ...here, and add vdi. * docs/schemas/storagevol.rng (formatfile): Use common list. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 19 +------------------ docs/schemas/storagecommon.rng | 20 ++++++++++++++++++++ docs/schemas/storagevol.rng | 13 +------------ 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7fc0cff..59f3fdd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1500,24 +1500,7 @@ </attribute> </optional> </define> - <define name='storageFormat'> - <choice> - <value>raw</value> - <value>dir</value> - <value>bochs</value> - <value>cloop</value> - <value>cow</value> - <value>dmg</value> - <value>iso</value> - <value>qcow</value> - <value>qcow2</value> - <value>qed</value> - <value>vmdk</value> - <value>vpc</value> - <value>fat</value> - <value>vhd</value> - </choice> - </define> + <define name="driverCache"> <attribute name="cache"> <choice> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 54cf6b4..37b43c6 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -52,4 +52,24 @@ </element> </define> + <define name='storageFormat'> + <choice> + <value>raw</value> + <value>dir</value> + <value>bochs</value> + <value>cloop</value> + <value>cow</value> + <value>dmg</value> + <value>iso</value> + <value>qcow</value> + <value>qcow2</value> + <value>qed</value> + <value>vmdk</value> + <value>vpc</value> + <value>fat</value> + <value>vhd</value> + <value>vdi</value> + </choice> + </define> + </grammar> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index df8c1eb..3798476 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -212,18 +212,7 @@ <define name='formatfile'> <choice> <value>unknown</value> - <value>raw</value> - <value>dir</value> - <value>bochs</value> - <value>cloop</value> - <value>cow</value> - <value>dmg</value> - <value>iso</value> - <value>qcow</value> - <value>qcow2</value> - <value>qed</value> - <value>vmdk</value> - <value>vpc</value> + <ref name='storageFormat'/> </choice> </define> -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:13 -0600, Eric Blake wrote:
We had incomplete RelaxNG support for storage formats listed in virstoragefile.h: commit 027bf2e added 'vdi' but forgot to update the <volume> and <domain> xml lists; the <volume> list was also missing 'fat' and 'vhd'. Maintaining two lists is a recipe for them getting out of sync, so make the list common so that both contexts benefit the next time we add a format in a single location.
* docs/schemas/domaincommon.rng (storageFormat): Move... * docs/schemas/storagecommon.rng: ...here, and add vdi. * docs/schemas/storagevol.rng (formatfile): Use common list.
ACK Jirka
Domain snapshots should only permit an external snapshot into a storage format that permits a backing chain, since the new snapshot file necessarily must be backed by the existing file. The C code for the qemu driver is a little bit stricter in currently enforcing only qcow2 or qed, but at the XML parser level, including virt-xml-validate, it is fairly easy to enforce that a user can't request a 'raw' external snapshot. * docs/schemas/storagecommon.rng (storageFormat): Split out... (storageFormatBacking): ...new sublist. * docs/schemas/domainsnapshot.rng (disksnapshotdriver): Use new type. * src/util/virstoragefile.h (virStorageFileFormat): Rearrange for easier code management. * src/util/virstoragefile.c (virStorageFileFormat, fileTypeInfo): Likewise. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML): Use new marker to limit selection of formats. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domainsnapshot.rng | 2 +- docs/schemas/storagecommon.rng | 17 ++++++++++---- src/conf/snapshot_conf.c | 7 ++++-- src/util/virstoragefile.c | 51 ++++++++++++++++++++++------------------- src/util/virstoragefile.h | 15 ++++++++---- 5 files changed, 55 insertions(+), 37 deletions(-) diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 824a186..644da90 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -184,7 +184,7 @@ <element name='driver'> <optional> <attribute name='type'> - <ref name='storageFormat'/> + <ref name='storageFormatBacking'/> </attribute> </optional> <empty/> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 37b43c6..2d61abc 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -52,23 +52,30 @@ </element> </define> + <!-- split the list of known storage formats into two, those where + we know how to follow backing chains, and all others --> + <define name='storageFormatBacking'> + <choice> + <value>cow</value> + <value>qcow</value> + <value>qcow2</value> + <value>qed</value> + <value>vmdk</value> + </choice> + </define> <define name='storageFormat'> <choice> <value>raw</value> <value>dir</value> <value>bochs</value> <value>cloop</value> - <value>cow</value> <value>dmg</value> <value>iso</value> - <value>qcow</value> - <value>qcow2</value> - <value>qed</value> - <value>vmdk</value> <value>vpc</value> <value>fat</value> <value>vhd</value> <value>vdi</value> + <ref name='storageFormatBacking'/> </choice> </define> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 070e50d..b0e4700 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -158,9 +158,12 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, char *driver = virXMLPropString(cur, "type"); if (driver) { def->src.format = virStorageFileFormatTypeFromString(driver); - if (def->src.format <= 0) { + if (def->src.format < VIR_STORAGE_FILE_BACKING) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk snapshot driver '%s'"), + def->src.format <= 0 + ? _("unknown disk snapshot driver '%s'") + : _("disk format '%s' lacks backing file " + "support"), driver); VIR_FREE(driver); goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 94dddbc..42e9865 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -59,9 +59,10 @@ VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, "none", "raw", "dir", "bochs", - "cloop", "cow", "dmg", "iso", - "qcow", "qcow2", "qed", "vmdk", "vpc", - "fat", "vhd", "vdi") + "cloop", "dmg", "iso", + "vpc", "fat", "vhd", "vdi", + /* Formats with backing file below here */ + "cow", "qcow", "qcow2", "qed", "vmdk") VIR_ENUM_IMPL(virStorageFileFeature, VIR_STORAGE_FILE_FEATURE_LAST, @@ -198,11 +199,6 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_LITTLE_ENDIAN, -1, {0}, -1, 0, 0, -1, NULL, NULL }, - [VIR_STORAGE_FILE_COW] = { - 0, "OOOM", NULL, - LV_BIG_ENDIAN, 4, {2}, - 4+4+1024+4, 8, 1, -1, cowGetBackingStore, NULL - }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, * /usr/share/misc/magic lists double magic (both offsets @@ -216,6 +212,29 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_LITTLE_ENDIAN, -2, {0}, -1, 0, 0, -1, NULL, NULL }, + [VIR_STORAGE_FILE_VPC] = { + 0, "conectix", NULL, + LV_BIG_ENDIAN, 12, {0x10000}, + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL, NULL + }, + /* TODO: add getBackingStore function */ + [VIR_STORAGE_FILE_VDI] = { + 64, "\x7f\x10\xda\xbe", ".vdi", + LV_LITTLE_ENDIAN, 68, {0x00010001}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL, NULL}, + + /* Not direct file formats, but used for various drivers */ + [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, + -1, {0}, 0, 0, 0, 0, NULL, NULL }, + [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, + -1, {0}, 0, 0, 0, 0, NULL, NULL }, + + /* All formats with a backing store probe below here */ + [VIR_STORAGE_FILE_COW] = { + 0, "OOOM", NULL, + LV_BIG_ENDIAN, 4, {2}, + 4+4+1024+4, 8, 1, -1, cowGetBackingStore, NULL + }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", NULL, LV_BIG_ENDIAN, 4, {1}, @@ -238,22 +257,6 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_LITTLE_ENDIAN, 4, {1, 2}, 4+4+4, 8, 512, -1, vmdk4GetBackingStore, NULL }, - [VIR_STORAGE_FILE_VPC] = { - 0, "conectix", NULL, - LV_BIG_ENDIAN, 12, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL, NULL - }, - /* TODO: add getBackingStore function */ - [VIR_STORAGE_FILE_VDI] = { - 64, "\x7f\x10\xda\xbe", ".vdi", - LV_LITTLE_ENDIAN, 68, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL, NULL}, - - /* Not direct file formats, but used for various drivers */ - [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, - [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, {0}, 0, 0, 0, 0, NULL, NULL }, }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 55ea890..29f93c1 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -64,18 +64,23 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_DIR, VIR_STORAGE_FILE_BOCHS, VIR_STORAGE_FILE_CLOOP, - VIR_STORAGE_FILE_COW, VIR_STORAGE_FILE_DMG, VIR_STORAGE_FILE_ISO, - VIR_STORAGE_FILE_QCOW, - VIR_STORAGE_FILE_QCOW2, - VIR_STORAGE_FILE_QED, - VIR_STORAGE_FILE_VMDK, VIR_STORAGE_FILE_VPC, VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, VIR_STORAGE_FILE_VDI, + /* Not a format, but a marker: all formats below this point have + * libvirt support for following a backing chain */ + VIR_STORAGE_FILE_BACKING, + + VIR_STORAGE_FILE_COW = VIR_STORAGE_FILE_BACKING, + VIR_STORAGE_FILE_QCOW, + VIR_STORAGE_FILE_QCOW2, + VIR_STORAGE_FILE_QED, + VIR_STORAGE_FILE_VMDK, + VIR_STORAGE_FILE_LAST, }; -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:14 -0600, Eric Blake wrote:
Domain snapshots should only permit an external snapshot into a storage format that permits a backing chain, since the new snapshot file necessarily must be backed by the existing file. The C code for the qemu driver is a little bit stricter in currently enforcing only qcow2 or qed, but at the XML parser level, including virt-xml-validate, it is fairly easy to enforce that a user can't request a 'raw' external snapshot.
* docs/schemas/storagecommon.rng (storageFormat): Split out... (storageFormatBacking): ...new sublist. * docs/schemas/domainsnapshot.rng (disksnapshotdriver): Use new type. * src/util/virstoragefile.h (virStorageFileFormat): Rearrange for easier code management. * src/util/virstoragefile.c (virStorageFileFormat, fileTypeInfo): Likewise. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML): Use new marker to limit selection of formats.
While you are rearranging the lists of file formats, why not make them all consistent? ACK with the following patch squashed in. Jirka diff --git i/docs/schemas/storagecommon.rng w/docs/schemas/storagecommon.rng index 2d61abc..1015e8d 100644 --- i/docs/schemas/storagecommon.rng +++ w/docs/schemas/storagecommon.rng @@ -72,9 +72,9 @@ <value>dmg</value> <value>iso</value> <value>vpc</value> + <value>vdi</value> <value>fat</value> <value>vhd</value> - <value>vdi</value> <ref name='storageFormatBacking'/> </choice> </define> diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 42e9865..ea80c1d 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -60,7 +60,9 @@ VIR_ENUM_IMPL(virStorageFileFormat, "none", "raw", "dir", "bochs", "cloop", "dmg", "iso", - "vpc", "fat", "vhd", "vdi", + "vpc", "vdi", + /* Not direct file formats, but used for various drivers */ + "fat", "vhd", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk") diff --git i/src/util/virstoragefile.h w/src/util/virstoragefile.h index 29f93c1..1b8b14f 100644 --- i/src/util/virstoragefile.h +++ w/src/util/virstoragefile.h @@ -67,9 +67,11 @@ enum virStorageFileFormat { VIR_STORAGE_FILE_DMG, VIR_STORAGE_FILE_ISO, VIR_STORAGE_FILE_VPC, + VIR_STORAGE_FILE_VDI, + + /* Not direct file formats, but used for various drivers */ VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, - VIR_STORAGE_FILE_VDI, /* Not a format, but a marker: all formats below this point have * libvirt support for following a backing chain */
This patch is my first experience playing with nested grammars, as documented in http://relaxng.org/tutorial-20011203.html#IDA3PZR. I plan on doing more overrides in order to make the RelaxNG grammar mirror the C code refactoring into a common virStorageSource, but where different clients of that source do not support the same subset of functionality. By starting with something fairly easy to validate, I can make sure my later patches will be possible. This patch adds a use of the no-op <ref name='sourceStartupPolicy'/> to the disksnapshot definition, so that the snapshot version of a type='file' <source> more closely resembles the version in domaincommon. A future patch will merge the two files into using a common define, but this patch is sufficient for testing that adding <source startupPolicy='optional'/> in any of the tests/domainsnapshotxml2xmlin/*.xml files still gets rejected unless it occurs within the <domain> subelement, because the definition of startupPolicy is empty outside of domain.rng. * docs/schemas/storagecommon.rng (storageStartupPolicy) (storageSourceExtra): Create no-op defaults. * docs/schemas/domainsnapshot.rng (domain): Use nested grammar to avoid restricting <domain>. (storageSourceExtra): Create new override. (disksnapshot): Access overrides through common names. * docs/schemas/domaincommon.rng (disk): Access overrides through common names. * docs/schemas/domain.rng (storageStartupPolicy) (storageSourceExtra): Create new overrides. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domain.rng | 14 +++++++++++++- docs/schemas/domaincommon.rng | 11 ++++++----- docs/schemas/domainsnapshot.rng | 21 +++++++++++++++++---- docs/schemas/storagecommon.rng | 14 ++++++++++++++ 4 files changed, 50 insertions(+), 10 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index cf0be68..114b87e 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1,9 +1,21 @@ <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> - <!-- We handle only document defining a domain --> + <!-- Grammar for accepting a domain element, both as top level, and + also suitable for inclusion in domainsnapshot.rng --> <start> <ref name="domain"/> </start> <include href='domaincommon.rng'/> + + <define name='storageStartupPolicy' combine='choice'> + <!-- overrides the no-op version in storagecommon.rng --> + <ref name='startupPolicy'/> + </define> + + <define name='storageSourceExtra' combine='choice'> + <!-- overrides the no-op version in storagecommon.rng --> + <ref name='diskspec'/> + </define> + </grammar> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 59f3fdd..df2c839 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1213,7 +1213,7 @@ </attribute> </optional> <optional> - <ref name="startupPolicy"/> + <ref name="storageStartupPolicy"/> </optional> <optional> <ref name='devSeclabel'/> @@ -1235,7 +1235,7 @@ </attribute> </optional> <optional> - <ref name="startupPolicy"/> + <ref name="storageStartupPolicy"/> </optional> <optional> <ref name='devSeclabel'/> @@ -1255,7 +1255,7 @@ <ref name="absFilePath"/> </attribute> <optional> - <ref name="startupPolicy"/> + <ref name="storageStartupPolicy"/> </optional> <empty/> </element> @@ -1296,7 +1296,7 @@ </attribute> </optional> <optional> - <ref name="startupPolicy"/> + <ref name="storageStartupPolicy"/> </optional> <optional> <ref name='devSeclabel'/> @@ -1306,10 +1306,11 @@ </interleave> </group> </choice> - <ref name="diskspec"/> + <ref name="storageSourceExtra"/> </interleave> </element> </define> + <define name="diskSourceNetwork"> <attribute name="protocol"> <choice> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 644da90..bec12db 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -75,7 +75,12 @@ <ref name="UUID"/> </element> </element> - <ref name='domain'/> + <!-- Nested grammar ensures that any of our overrides of + storagecommon/domaincommon defines do not conflict + with any domain.rng overrides. --> + <grammar> + <include href='domain.rng'/> + </grammar> </choice> </optional> <optional> @@ -102,6 +107,11 @@ </choice> </define> + <define name='storageSourceExtra' combine='choice'> + <!-- overrides the no-op version in storagecommon.rng --> + <ref name='disksnapshotdriver'/> + </define> + <define name='disksnapshot'> <element name='disk'> <attribute name='name'> @@ -138,10 +148,13 @@ <ref name='absFilePath'/> </attribute> </optional> + <optional> + <ref name='storageStartupPolicy'/> + </optional> <empty/> </element> </optional> - <ref name='disksnapshotdriver'/> + <ref name='storageSourceExtra'/> </interleave> </group> <group> @@ -157,7 +170,7 @@ <empty/> </element> </optional> - <ref name='disksnapshotdriver'/> + <ref name='storageSourceExtra'/> </interleave> </group> <group> @@ -170,7 +183,7 @@ <ref name='diskSourceNetwork'/> </element> </optional> - <ref name='disksnapshotdriver'/> + <ref name='storageSourceExtra'/> </interleave> </group> </choice> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 2d61abc..372832d 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -79,4 +79,18 @@ </choice> </define> + <define name='storageStartupPolicy'> + <!-- Use a combine='choice' override in client files that want to + add additional attributes to a <source> sub-element + associated with a storage source --> + <notAllowed/> + </define> + + <define name='storageSourceExtra'> + <!-- Use a combine='choice' override in client files that want to + add additional elements as siblings of a <source> sub-element + associated with a storage source --> + <notAllowed/> + </define> + </grammar> -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:15 -0600, Eric Blake wrote:
This patch is my first experience playing with nested grammars, as documented in http://relaxng.org/tutorial-20011203.html#IDA3PZR. I plan on doing more overrides in order to make the RelaxNG grammar mirror the C code refactoring into a common virStorageSource, but where different clients of that source do not support the same subset of functionality. By starting with something fairly easy to validate, I can make sure my later patches will be possible.
This patch adds a use of the no-op <ref name='sourceStartupPolicy'/> to the disksnapshot definition, so that the snapshot version of a type='file' <source> more closely resembles the version in domaincommon. A future patch will merge the two files into using a common define, but this patch is sufficient for testing that adding <source startupPolicy='optional'/> in any of the tests/domainsnapshotxml2xmlin/*.xml files still gets rejected unless it occurs within the <domain> subelement, because the definition of startupPolicy is empty outside of domain.rng.
To prove this, I suggest adding new XML in domainsnapshotxml2xmlin that would use startupPolicy in the forbidden way. ACK with the following test squashed in. Jirka diff --git c/tests/domainsnapshotxml2xmlin/disk-invalid.xml i/tests/domainsnapshotxml2xmlin/disk-invalid.xml new file mode 100644 index 0000000..6d2bea4 --- /dev/null +++ i/tests/domainsnapshotxml2xmlin/disk-invalid.xml @@ -0,0 +1,10 @@ +<domainsnapshot> + <name>asdf</name> + <description>adsf</description> + <disks> + <disk name='vda' snapshot='external'> + <source file='/tmp/foo' startupPolicy='optional'/> + <driver/> + </disk> + </disks> +</domainsnapshot>
Disk snapshots use a subset of <disk> sources (no directory or pool support yet, and while domain disks support a startupPolicy, it doesn't make sense for snapshots). This patch lets the two RelaxNG grammars share a bit more code, as well as factoring things into pieces that will be easier to move to a common file for sharing with storage volumes. It relies on the ability to override definitions as part of an include, set up in the previous patch. The diff is a bit hard to read, because it mixes reindentation with refactoring; 'git diff -b --patience' may help. * docs/schemas/domaincommon.rng (disk): Refactor into pieces. (diskSource, diskSourceFile, diskSourceBlock, diskSourceDir) (diskSourceVolume: New defines. (diskSourceNetwork): Revise scope. * docs/schemas/domainsnapshot.rng (disksnapshot): Adjust. (disksnapshotsource): New define. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 329 +++++++++++++++++++++------------------- docs/schemas/domainsnapshot.rng | 63 ++------ 2 files changed, 179 insertions(+), 213 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index df2c839..7739fd8 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1196,176 +1196,187 @@ <optional> <ref name="snapshot"/> </optional> - <interleave> - <choice> - <group> - <optional> - <attribute name="type"> - <value>file</value> - </attribute> - </optional> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - </interleave> - </group> - <group> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="dev"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - </interleave> - </group> - <group> - <attribute name="type"> - <value>dir</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dir"> - <ref name="absFilePath"/> - </attribute> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <empty/> - </element> - </optional> - </interleave> - </group> - <group> - <attribute name="type"> - <value>network</value> + <ref name="diskSource"/> + </element> + </define> + + <define name="diskSource"> + <choice> + <ref name="diskSourceFile"/> + <ref name="diskSourceBlock"/> + <ref name="diskSourceDir"/> + <ref name="diskSourceNetwork"/> + <ref name="diskSourceVolume"/> + </choice> + </define> + + <define name="diskSourceFile"> + <optional> + <attribute name="type"> + <value>file</value> + </attribute> + </optional> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="file"> + <ref name="absFilePath"/> </attribute> - <interleave> - <optional> - <element name="source"> - <ref name='diskSourceNetwork'/> - </element> - </optional> - </interleave> - </group> - <group> - <attribute name="type"> - <value>volume</value> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="diskSourceBlock"> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="dev"> + <ref name="absFilePath"/> </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="pool"> - <ref name="genericName"/> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="diskSourceDir"> + <attribute name="type"> + <value>dir</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dir"> + <ref name="absFilePath"/> + </attribute> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <empty/> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="diskSourceNetwork"> + <attribute name="type"> + <value>network</value> + </attribute> + <interleave> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>rbd</value> + <value>sheepdog</value> + <value>gluster</value> + <value>iscsi</value> + <value>http</value> + <value>https</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <element name="host"> + <choice> + <group> + <optional> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>rdma</value> + </choice> </attribute> - <attribute name="volume"> - <ref name="volName"/> + </optional> + <attribute name="name"> + <choice> + <ref name="dnsName"/> + <ref name="ipAddr"/> + </choice> + </attribute> + <optional> + <attribute name="port"> + <ref name="unsignedInt"/> </attribute> - <optional> - <attribute name="mode"> - <choice> - <value>host</value> - <value>direct</value> - </choice> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - </interleave> - </group> - </choice> - <ref name="storageSourceExtra"/> - </interleave> - </element> + </optional> + </group> + <group> + <attribute name="transport"> + <value>unix</value> + </attribute> + <attribute name="socket"> + <ref name="absFilePath"/> + </attribute> + </group> + </choice> + </element> + </zeroOrMore> + <empty/> + </element> + <ref name='storageSourceExtra'/> + </interleave> </define> - <define name="diskSourceNetwork"> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>rbd</value> - <value>sheepdog</value> - <value>gluster</value> - <value>iscsi</value> - <value>http</value> - <value>https</value> - <value>ftp</value> - <value>ftps</value> - <value>tftp</value> - </choice> + <define name="diskSourceVolume"> + <attribute name="type"> + <value>volume</value> </attribute> - <optional> - <attribute name="name"/> - </optional> - <zeroOrMore> - <element name="host"> - <choice> - <group> - <optional> - <attribute name="transport"> - <choice> - <value>tcp</value> - <value>rdma</value> - </choice> - </attribute> - </optional> - <attribute name="name"> + <interleave> + <optional> + <element name="source"> + <attribute name="pool"> + <ref name="genericName"/> + </attribute> + <attribute name="volume"> + <ref name="volName"/> + </attribute> + <optional> + <attribute name="mode"> <choice> - <ref name="dnsName"/> - <ref name="ipAddr"/> + <value>host</value> + <value>direct</value> </choice> </attribute> - <optional> - <attribute name="port"> - <ref name="unsignedInt"/> - </attribute> - </optional> - </group> - <group> - <attribute name="transport"> - <value>unix</value> - </attribute> - <attribute name="socket"> - <ref name="absFilePath"/> - </attribute> - </group> - </choice> - </element> - </zeroOrMore> - <empty/> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> </define> + <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index bec12db..ef3135f 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -133,65 +133,20 @@ <value>external</value> </attribute> </optional> - <choice> - <group> - <optional> - <attribute name='type'> - <value>file</value> - </attribute> - </optional> - <interleave> - <optional> - <element name='source'> - <optional> - <attribute name='file'> - <ref name='absFilePath'/> - </attribute> - </optional> - <optional> - <ref name='storageStartupPolicy'/> - </optional> - <empty/> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </group> - <group> - <attribute name='type'> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dev"> - <ref name="absFilePath"/> - </attribute> - <empty/> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>network</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <ref name='diskSourceNetwork'/> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </group> - </choice> + <ref name="disksnapshotsource"/> </group> </choice> </element> </define> + <define name='disksnapshotsource'> + <choice> + <ref name='diskSourceFile'/> + <ref name='diskSourceBlock'/> + <ref name='diskSourceNetwork'/> + </choice> + </define> + <define name='disksnapshotdriver'> <optional> <element name='driver'> -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:16 -0600, Eric Blake wrote:
Disk snapshots use a subset of <disk> sources (no directory or pool support yet, and while domain disks support a startupPolicy, it doesn't make sense for snapshots). This patch lets the two RelaxNG grammars share a bit more code, as well as factoring things into pieces that will be easier to move to a common file for sharing with storage volumes. It relies on the ability to override definitions as part of an include, set up in the previous patch.
The diff is a bit hard to read, because it mixes reindentation with refactoring; 'git diff -b --patience' may help.
* docs/schemas/domaincommon.rng (disk): Refactor into pieces. (diskSource, diskSourceFile, diskSourceBlock, diskSourceDir) (diskSourceVolume: New defines. (diskSourceNetwork): Revise scope. * docs/schemas/domainsnapshot.rng (disksnapshot): Adjust. (disksnapshotsource): New define.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 329 +++++++++++++++++++++------------------- docs/schemas/domainsnapshot.rng | 63 ++------ 2 files changed, 179 insertions(+), 213 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index df2c839..7739fd8 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng ... diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index bec12db..ef3135f 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -133,65 +133,20 @@ <value>external</value> </attribute> </optional> - <choice> - <group> - <optional> - <attribute name='type'> - <value>file</value> - </attribute> - </optional> - <interleave> - <optional> - <element name='source'> - <optional> - <attribute name='file'> - <ref name='absFilePath'/> - </attribute> - </optional> - <optional> - <ref name='storageStartupPolicy'/> - </optional> - <empty/> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </group> ... - </choice> + <ref name="disksnapshotsource"/> </group> </choice> </element> </define>
+ <define name='disksnapshotsource'> + <choice> + <ref name='diskSourceFile'/> + <ref name='diskSourceBlock'/> + <ref name='diskSourceNetwork'/> + </choice> + </define>
This would allow <ref name='devSeclabel'/> in /domainsnapshot/disks/disk/source even though it was not allowed before. For example, the attached XML would be considered valid after this patch. And I see the follow-up patches add more stuff that is allowed in domain snapshot XML however there's no code that would actually use the new stuff, is it? I think we should take the changes in domaincommon.rng while leaving domainsnapshot.rng alone. You started playing with making some stuff usable only in domain XML and another stuff only in domain snapshot XML but unfortunately there's a lot of other elements that would need similar treatment. Making both XMLs reuse the same source schema is a great idea but it's going to be a time-consuming task and the possibility for the result to become even less readable than two distinct definitions is very high. ACK to the domaincommon.rng part. Jirka diff --git c/tests/domainsnapshotxml2xmlin/disk-seclabel-invalid.xml i/tests/domainsnapshotxml2xmlin/disk-seclabel-invalid.xml new file mode 100644 index 0000000..528c646 --- /dev/null +++ i/tests/domainsnapshotxml2xmlin/disk-seclabel-invalid.xml @@ -0,0 +1,11 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <disks> + <disk name='hde' snapshot='external' type='file'> + <source file='/path/to/new2'> + <seclabel model='dac' relabel='no'/> + </source> + </disk> + </disks> +</domainsnapshot>
On 04/15/2014 03:43 PM, Jiri Denemark wrote:
On Mon, Apr 14, 2014 at 16:54:16 -0600, Eric Blake wrote:
Disk snapshots use a subset of <disk> sources (no directory or pool support yet, and while domain disks support a startupPolicy, it doesn't make sense for snapshots). This patch lets the two RelaxNG grammars share a bit more code, as well as factoring things into pieces that will be easier to move to a common file for sharing with storage volumes. It relies on the ability to override definitions as part of an include, set up in the previous patch.
The diff is a bit hard to read, because it mixes reindentation with refactoring; 'git diff -b --patience' may help.
+ <define name='disksnapshotsource'> + <choice> + <ref name='diskSourceFile'/> + <ref name='diskSourceBlock'/> + <ref name='diskSourceNetwork'/> + </choice> + </define>
This would allow <ref name='devSeclabel'/> in /domainsnapshot/disks/disk/source even though it was not allowed before.
Indeed, but that is intentional - it SHOULD be possible to specify the label that libvirt applies to the new element of a backing chain. But I'm fine splitting the work to add seclabel support in snapshots as a later commit.
For example, the attached XML would be considered valid after this patch. And I see the follow-up patches add more stuff that is allowed in domain snapshot XML however there's no code that would actually use the new stuff, is it?
Not yet, so saving the RNG changes until the code changes are also in place is fine.
I think we should take the changes in domaincommon.rng while leaving domainsnapshot.rng alone. You started playing with making some stuff usable only in domain XML and another stuff only in domain snapshot XML but unfortunately there's a lot of other elements that would need similar treatment. Making both XMLs reuse the same source schema is a great idea but it's going to be a time-consuming task and the possibility for the result to become even less readable than two distinct definitions is very high.
ACK to the domaincommon.rng part.
Thanks for picking up on this and committing the uncontroversial parts while I was offline last week. Now I get to review your series, and rebase my remaining work on top of the additional progress. :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Start reflecting the C source code by moving RelaxNG grammar for virStorageSource into a common file. There's still more to move, but doing it in pieces makes it easier to validate. * docs/schemas/domaincommon.rng (diskSourceFile, diskSourceBlock) (diskSourceDir, diskSourceNetwork, diskSourceVolume) (devSeclabel): Move... * docs/schemas/storagecommon.rng (storageSourceFile) (storageSourceBlock, storageSourceDir, storageSourceNetwork) (storageSourceVolume, devSeclabel): ...and rename. * docs/schemas/domainsnapshot.rng (disksnapshotsource): Update client. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 215 ++-------------------------------------- docs/schemas/domainsnapshot.rng | 6 +- docs/schemas/storagecommon.rng | 205 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 215 insertions(+), 211 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7739fd8..a3e5278 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -71,6 +71,7 @@ </element> </define> <define name="seclabel"> + <!-- see also devseclabel in storagecommon.rng --> <element name="seclabel"> <optional> <attribute name='model'> @@ -149,42 +150,7 @@ </choice> </element> </define> - <define name="devSeclabel"> - <element name="seclabel"> - <!-- A per-device seclabel override is more limited, either - relabel=no or a <label> must be present on input; - output also can include labelskip=yes. --> - <optional> - <attribute name='model'> - <text/> - </attribute> - </optional> - <choice> - <group> - <attribute name='relabel'> - <value>no</value> - </attribute> - </group> - <group> - <attribute name='labelskip'> - <value>yes</value> - </attribute> - </group> - <group> - <optional> - <attribute name='relabel'> - <value>yes</value> - </attribute> - </optional> - <oneOrMore> - <element name='label'> - <text/> - </element> - </oneOrMore> - </group> - </choice> - </element> - </define> + <define name="hvs"> <attribute name="type"> <choice> @@ -1202,181 +1168,14 @@ <define name="diskSource"> <choice> - <ref name="diskSourceFile"/> - <ref name="diskSourceBlock"/> - <ref name="diskSourceDir"/> - <ref name="diskSourceNetwork"/> - <ref name="diskSourceVolume"/> + <ref name="storageSourceFile"/> + <ref name="storageSourceBlock"/> + <ref name="storageSourceDir"/> + <ref name="storageSourceNetwork"/> + <ref name="storageSourceVolume"/> </choice> </define> - <define name="diskSourceFile"> - <optional> - <attribute name="type"> - <value>file</value> - </attribute> - </optional> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </define> - - <define name="diskSourceBlock"> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="dev"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </define> - - <define name="diskSourceDir"> - <attribute name="type"> - <value>dir</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dir"> - <ref name="absFilePath"/> - </attribute> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <empty/> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </define> - - <define name="diskSourceNetwork"> - <attribute name="type"> - <value>network</value> - </attribute> - <interleave> - <element name="source"> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>rbd</value> - <value>sheepdog</value> - <value>gluster</value> - <value>iscsi</value> - <value>http</value> - <value>https</value> - <value>ftp</value> - <value>ftps</value> - <value>tftp</value> - </choice> - </attribute> - <optional> - <attribute name="name"/> - </optional> - <zeroOrMore> - <element name="host"> - <choice> - <group> - <optional> - <attribute name="transport"> - <choice> - <value>tcp</value> - <value>rdma</value> - </choice> - </attribute> - </optional> - <attribute name="name"> - <choice> - <ref name="dnsName"/> - <ref name="ipAddr"/> - </choice> - </attribute> - <optional> - <attribute name="port"> - <ref name="unsignedInt"/> - </attribute> - </optional> - </group> - <group> - <attribute name="transport"> - <value>unix</value> - </attribute> - <attribute name="socket"> - <ref name="absFilePath"/> - </attribute> - </group> - </choice> - </element> - </zeroOrMore> - <empty/> - </element> - <ref name='storageSourceExtra'/> - </interleave> - </define> - - <define name="diskSourceVolume"> - <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> - <optional> - <attribute name="mode"> - <choice> - <value>host</value> - <value>direct</value> - </choice> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name='devSeclabel'/> - </optional> - </element> - </optional> - <ref name='storageSourceExtra'/> - </interleave> - </define> - <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index ef3135f..fc630a6 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -141,9 +141,9 @@ <define name='disksnapshotsource'> <choice> - <ref name='diskSourceFile'/> - <ref name='diskSourceBlock'/> - <ref name='diskSourceNetwork'/> + <ref name='storageSourceFile'/> + <ref name='storageSourceBlock'/> + <ref name='storageSourceNetwork'/> </choice> </define> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 372832d..e1db740 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -93,4 +93,209 @@ <notAllowed/> </define> + <define name="devSeclabel"> + <element name="seclabel"> + <!-- A per-device seclabel override is more limited than + seclabel in domaincommon.rng. Either + relabel=no or a <label> must be present on input; + output also can include labelskip=yes. --> + <optional> + <attribute name='model'> + <text/> + </attribute> + </optional> + <choice> + <group> + <attribute name='relabel'> + <value>no</value> + </attribute> + </group> + <group> + <attribute name='labelskip'> + <value>yes</value> + </attribute> + </group> + <group> + <optional> + <attribute name='relabel'> + <value>yes</value> + </attribute> + </optional> + <oneOrMore> + <element name='label'> + <text/> + </element> + </oneOrMore> + </group> + </choice> + </element> + </define> + + <define name="storageSourceFile"> + <optional> + <attribute name="type"> + <value>file</value> + </attribute> + </optional> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="storageSourceBlock"> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <optional> + <attribute name="dev"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="storageSourceDir"> + <attribute name="type"> + <value>dir</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dir"> + <ref name="absFilePath"/> + </attribute> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <empty/> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="storageSourceNetwork"> + <attribute name="type"> + <value>network</value> + </attribute> + <interleave> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>rbd</value> + <value>sheepdog</value> + <value>gluster</value> + <value>iscsi</value> + <value>http</value> + <value>https</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <element name="host"> + <choice> + <group> + <optional> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>rdma</value> + </choice> + </attribute> + </optional> + <attribute name="name"> + <choice> + <ref name="dnsName"/> + <ref name="ipAddr"/> + </choice> + </attribute> + <optional> + <attribute name="port"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </group> + <group> + <attribute name="transport"> + <value>unix</value> + </attribute> + <attribute name="socket"> + <ref name="absFilePath"/> + </attribute> + </group> + </choice> + </element> + </zeroOrMore> + <empty/> + </element> + <ref name='storageSourceExtra'/> + </interleave> + </define> + + <define name="storageSourceVolume"> + <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> + <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>direct</value> + </choice> + </attribute> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name='devSeclabel'/> + </optional> + </element> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + </grammar> -- 1.9.0
In a backing chain, each file of the chain can have a different authorization or encryption; therefore, this information should be associated with the common storage source elements. * docs/schemas/domaincommon.rng (diskspec): Drop source-related portions... (disksource): ...and include common types here instead. (diskAuth, diskAuthSecret): Move... * docs/schemas/storagecommon.rng (diskAuth, diskAuthSecret): ...here. (storageSourceCommon): New define. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 33 ---------------------------- docs/schemas/storagecommon.rng | 50 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a3e5278..78fed88 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1007,9 +1007,6 @@ <optional> <ref name='diskMirror'/> </optional> - <optional> - <ref name="diskAuth"/> - </optional> <ref name="target"/> <optional> <ref name="deviceBoot"/> @@ -1035,9 +1032,6 @@ </element> </optional> <optional> - <ref name="encryption"/> - </optional> - <optional> <ref name="diskIoTune"/> </optional> <optional> @@ -3960,33 +3954,6 @@ </optional> </element> </define> - <define name="diskAuth"> - <element name="auth"> - <attribute name="username"> - <ref name="genericName"/> - </attribute> - <ref name="diskAuthSecret"/> - </element> - </define> - - <define name='diskAuthSecret'> - <element name='secret'> - <attribute name='type'> - <choice> - <value>ceph</value> - <value>iscsi</value> - </choice> - </attribute> - <choice> - <attribute name='uuid'> - <ref name="UUID"/> - </attribute> - <attribute name='usage'> - <ref name='genericName'/> - </attribute> - </choice> - </element> - </define> <define name='diskIoTune'> <element name="iotune"> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index e1db740..4c0b719 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -6,6 +6,34 @@ <!-- This schema is not designed for standalone use; another file must include both this file and basictypes.rng --> + <define name='diskAuthSecret'> + <element name='secret'> + <attribute name='type'> + <choice> + <value>ceph</value> + <value>iscsi</value> + </choice> + </attribute> + <choice> + <attribute name='uuid'> + <ref name="UUID"/> + </attribute> + <attribute name='usage'> + <ref name='genericName'/> + </attribute> + </choice> + </element> + </define> + + <define name="diskAuth"> + <element name="auth"> + <attribute name="username"> + <ref name="genericName"/> + </attribute> + <ref name="diskAuthSecret"/> + </element> + </define> + <define name='encryption'> <element name='encryption'> <attribute name='format'> @@ -131,6 +159,18 @@ </element> </define> + <define name='storageSourceCommon'> + <interleave> + <optional> + <ref name="encryption"/> + </optional> + <optional> + <ref name="diskAuth"/> + </optional> + <ref name='storageSourceExtra'/> + </interleave> + </define> + <define name="storageSourceFile"> <optional> <attribute name="type"> @@ -153,7 +193,7 @@ </optional> </element> </optional> - <ref name='storageSourceExtra'/> + <ref name='storageSourceCommon'/> </interleave> </define> @@ -177,7 +217,7 @@ </optional> </element> </optional> - <ref name='storageSourceExtra'/> + <ref name='storageSourceCommon'/> </interleave> </define> @@ -197,7 +237,7 @@ <empty/> </element> </optional> - <ref name='storageSourceExtra'/> + <ref name='storageSourceCommon'/> </interleave> </define> @@ -261,7 +301,7 @@ </zeroOrMore> <empty/> </element> - <ref name='storageSourceExtra'/> + <ref name='storageSourceCommon'/> </interleave> </define> @@ -294,7 +334,7 @@ </optional> </element> </optional> - <ref name='storageSourceExtra'/> + <ref name='storageSourceCommon'/> </interleave> </define> -- 1.9.0
I noticed that depending on the <driver> attributes the user passed in, the output may omit the <driver> element altogether. For example, the rerror_policy has had this problem since commit 4bb4109 in Oct 2011. But in adding testsuite coverage to expose it, I found another problem: the C code is just fine without a driver name, but the XML validator required either a name or a cache mode. * src/conf/domain_conf.c (virDomainDiskDefFormat): Update conditional. * docs/schemas/domaincommon.rng (diskDriver): Simplify. * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml: * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args: New files. * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml: Enhance test. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml: Likewise. * tests/qemuxml2argvtest.c (mymain): New test. * tests/qemuxml2xmltest.c (mymain): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 20 +++++----------- src/conf/domain_conf.c | 4 +++- .../qemuxml2argv-disk-drive-copy-on-read.args | 5 ++++ .../qemuxml2argv-disk-drive-copy-on-read.xml | 28 ++++++++++++++++++++++ .../qemuxml2argv-disk-drive-discard.xml | 2 +- tests/qemuxml2argvtest.c | 3 +++ .../qemuxml2xmlout-disk-drive-discard.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 8 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 78fed88..09f426a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1254,20 +1254,12 @@ --> <define name="diskDriver"> <element name="driver"> - <choice> - <group> - <ref name="driverFormat"/> - <optional> - <ref name="driverCache"/> - </optional> - </group> - <group> - <optional> - <ref name="driverFormat"/> - </optional> - <ref name="driverCache"/> - </group> - </choice> + <optional> + <ref name="driverFormat"/> + </optional> + <optional> + <ref name="driverCache"/> + </optional> <optional> <ref name="driverErrorPolicy"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 726c8ba..05fa3f9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14963,7 +14963,9 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); if (def->src.driverName || def->src.format > 0 || def->cachemode || - def->ioeventfd || def->event_idx || def->copy_on_read) { + def->error_policy || def->rerror_policy || def->iomode || + def->ioeventfd || def->event_idx || def->copy_on_read || + def->discard) { virBufferAddLit(buf, "<driver"); if (def->src.driverName) virBufferAsprintf(buf, " name='%s'", def->src.driverName); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args new file mode 100644 index 0000000..f743b6b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0,\ +format=raw,copy-on-read=on -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml new file mode 100644 index 0000000..c15ca93 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml @@ -0,0 +1,28 @@ +<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='block' device='disk'> + <driver copy_on_read='on'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml index b15fd63..de2855a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml @@ -25,7 +25,7 @@ <driver discard='unmap' name='qemu' type='qcow2'/> </disk> <disk type='file' device='cdrom'> - <driver name='qemu' type='raw' discard='ignore'/> + <driver discard='ignore'/> <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> <target dev='hdc' bus='ide'/> <readonly/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 13ed4f6..d43a4de 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -739,6 +739,9 @@ mymain(void) DO_TEST("disk-drive-cache-unsafe", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_CACHE_UNSAFE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-copy-on-read", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, + QEMU_CAPS_DRIVE_COPY_ON_READ, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd-export", diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml index f01312f..f20b3b9 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml @@ -23,7 +23,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> <disk type='file' device='cdrom'> - <driver name='qemu' type='raw' discard='ignore'/> + <driver discard='ignore'/> <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> <target dev='hdc' bus='ide'/> <readonly/> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 788adbe..bf9d736 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -187,6 +187,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-copy-on-read"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-nbd-export"); DO_TEST("disk-drive-network-nbd-ipv6"); -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:19 -0600, Eric Blake wrote:
I noticed that depending on the <driver> attributes the user passed in, the output may omit the <driver> element altogether. For example, the rerror_policy has had this problem since commit 4bb4109 in Oct 2011. But in adding testsuite coverage to expose it, I found another problem: the C code is just fine without a driver name, but the XML validator required either a name or a cache mode.
* src/conf/domain_conf.c (virDomainDiskDefFormat): Update conditional. * docs/schemas/domaincommon.rng (diskDriver): Simplify. * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.xml: * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-copy-on-read.args: New files. * tests/qemuxml2argvdata/qemuxml2argv-disk-drive-discard.xml: Enhance test. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml: Likewise. * tests/qemuxml2argvtest.c (mymain): New test. * tests/qemuxml2xmltest.c (mymain): Likewise.
ACK Jirka
Still an RFC, since this claims over 400 tests are impacted: $ git grep '<driver.*type=\(\(.\)[a-zA-Z0-9]*\2\)' tests/*data/ | wc Most uses of <driver> are per-disk - they affect the way the hypervisor exposes storage to the guest, regardless of how many host resources are involved. But one attribute, the format, is something associated with each host resource, since it is possible to have a chain of files of different formats. What's more, the <volume> XML of storage volume APIs already has a <format type='...'/> element. This patch adds a <format type='...'/> element to each <disk>'s <source> within a <domain>. For back-compat, we still output the format of the top-level resource in the <driver> element; and accept either spelling on input (if both spellings are given, they must match); but once the XML is extended to show full backing chain in <domain>s, the backing files will have only a <format> element, and no extra <driver> elements. Along with documenting, accepting, and outputting the new field, this patch has to update every single test that had a <driver> format to tolerate the new output. For tests where input differs from output, I left the input alone to prove the old style still works; there is also a new test to prove the new style as input in isolation produces the correct output. * docs/schemas/storagecommon.rng (storageSourceCommon): Parse new element. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Parse new format element; ensure sane definition. (virDomainDiskSourceFormat): Output new element. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 19 ++++++++++++++++++- docs/schemas/storagecommon.rng | 7 +++++++ src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7f90455..53eda93 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1887,6 +1887,15 @@ write I/O operations per second.</dd> </dl> </dd> + <dt><code>format</code></dt> + <dd> + The optional format element includes a <code>type</code> + attribute that records the storage format used by the host + resource mentioned in the <code>source</code> + element. This element matches the format shown for the XML + of a storage volume, when inspecting a host resource via the + storage pool APIs. <span class="since">Since 1.2.4</span> + </dd> <dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details @@ -1901,7 +1910,15 @@ supports a name of "tap", "tap2", "phy", or "file", with a type of "aio", while qemu only supports a name of "qemu", but multiple types including "raw", "bochs", "qcow2", and - "qed". + "qed".<br/> + <span class="since">Since 1.2.4</span>, the type attribute + is also shown in the <code>format</code> element as its + preferred location (since a chain of backing files may + have separate types per file, while the majority of + the <code>driver</code> element applies to the device as a + whole rather than to individual backing files). On input, + either location may specify a format; if both locations + are used, the format must be identical between the two. </li> <li> The optional <code>cache</code> attribute controls the diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 4c0b719..68ce5b9 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -162,6 +162,13 @@ <define name='storageSourceCommon'> <interleave> <optional> + <element name='format'> + <attribute name='type'> + <ref name='storageFormat'/> + </attribute> + </element> + </optional> + <optional> <ref name="encryption"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 05fa3f9..e226758 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5111,6 +5111,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *sgio = NULL; char *driverName = NULL; char *driverType = NULL; + char *format = NULL; const char *source = NULL; char *target = NULL; char *trans = NULL; @@ -5819,14 +5820,34 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->product = product; product = NULL; - if (driverType) { - def->src.format = virStorageFileFormatTypeFromString(driverType); + if (format) { + def->src.format = virStorageFileFormatTypeFromString(format); if (def->src.format <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown driver format value '%s'"), + format); + goto error; + } + } + + /* For back-compat, we also accept <driver type='format'> as a + * synonym for the newer <format type='format'>. If both are + * given, they must match. */ + if (driverType) { + int value = virStorageFileFormatTypeFromString(driverType); + if (value <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown driver format value '%s'"), driverType); goto error; + } else if (def->src.format && def->src.format != value) { + virReportError(VIR_ERR_XML_ERROR, + _("conflicting formats '%s' and '%s'"), + driverType, + virStorageFileFormatTypeToString(def->src.format)); + goto error; } + def->src.format = value; } if (mirrorFormat) { @@ -5858,6 +5879,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(usageType); VIR_FREE(authUUID); VIR_FREE(authUsage); + VIR_FREE(format); VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(mirror); @@ -14799,11 +14821,17 @@ virDomainDiskSourceFormat(virBufferPtr buf, { size_t n; const char *startupPolicy = NULL; + const char *format = NULL; if (policy) startupPolicy = virDomainStartupPolicyTypeToString(policy); + if (src->format) + format = virStorageFileFormatTypeToString(src->format); if (src->path || src->nhosts > 0 || src->srcpool || startupPolicy) { + /* No <format> element unless <source> provided. */ + virBufferEscapeString(buf, "<format type='%s'>\n", format); + switch ((enum virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: virBufferAddLit(buf, "<source"); -- 1.9.0
On Mon, Apr 14, 2014 at 16:54:20 -0600, Eric Blake wrote:
Still an RFC, since this claims over 400 tests are impacted: $ git grep '<driver.*type=\(\(.\)[a-zA-Z0-9]*\2\)' tests/*data/ | wc
Most uses of <driver> are per-disk - they affect the way the hypervisor exposes storage to the guest, regardless of how many host resources are involved. But one attribute, the format, is something associated with each host resource, since it is possible to have a chain of files of different formats. What's more, the <volume> XML of storage volume APIs already has a <format type='...'/> element.
This patch adds a <format type='...'/> element to each <disk>'s <source> within a <domain>. For back-compat, we still output the format of the top-level resource in the <driver> element; and accept either spelling on input (if both spellings are given, they must match); but once the XML is extended to show full backing chain in <domain>s, the backing files will have only a <format> element, and no extra <driver> elements.
Hmm, I don't like the idea at all. It seem the only benefit of this patch is a more reusable parts of XML schema for the price of complicating everything else. Jirka
On Mon, Apr 14, 2014 at 16:54:11 -0600, Eric Blake wrote:
changes since v1: ack'd patch pushed new patches 2-4 are a better rewrite of old patch 3/6 new patches 8 and 9 patch 9 is incomplete - I know where I want to be headed, but changing 400+ tests is taking a while. I tried:
$ git grep -l '<driver.*type=' tests/*data/ | \ xargs sed ' /<driver .*type=\(\(.\)[a-zA-Z0-9]*\2\)/ h; /<source /{ x; s/<driver.*type=\(\(.\)[a-zA-Z0-9]*\2\).*>/<format type=\1>/; G; }'
as a way to automate the majority of the tedium, with some limited success (it has false positives in the lxc <filesystem> elements, when we really only want to affect <disk> elements>), but have not completed the efforts. I'm posting the patches now, in the hopes that 1-8 can be approved, and 9 picked up by someone else if I'm unresponsive for a while (I'll be offline for much of this week due to the birth of my fourth child).
Eric Blake (9): conf: better <disk> interleaving in schema conf: move storage formats to common RNG file conf: restrict external snapshots to backing store formats conf: set up for per-grammar overrides in schemas conf: split <disk> schema into more pieces conf: move storage source details to common RNG file conf: move <auth> and <encryption> to disk source conf: fix omission of <driver> in domain dumpxml RFC: conf: introduce <format> as synonym of <driver type=...>
I pushed the patches I ACKed (1, 2, 3, 4, 5, and 8) with the suggested additions. I think the rest can be left for the future if needed. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark