5:54 p.m.
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
5:54 p.m.
New subject: [libvirt] [PATCHv2 1/9] conf: better <disk> interleaving in schema
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(a)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
6:51 a.m.
New subject: [libvirt] [PATCHv2 1/9] conf: better <disk> interleaving in schema
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
5:54 p.m.
New subject: [libvirt] [PATCHv2 2/9] conf: move storage formats to common RNG file
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(a)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
6:55 a.m.
New subject: [libvirt] [PATCHv2 2/9] conf: move storage formats to common RNG file
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
5:54 p.m.
New subject: [libvirt] [PATCHv2 3/9] conf: restrict external snapshots to backing store formats
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(a)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
6:57 a.m.
New subject: [libvirt] [PATCHv2 3/9] conf: restrict external snapshots to backing store formats
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 */
5:54 p.m.
New subject: [libvirt] [PATCHv2 4/9] conf: set up for per-grammar overrides in schemas
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(a)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
2:09 p.m.
New subject: [libvirt] [PATCHv2 4/9] conf: set up for per-grammar overrides in schemas
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>
5:54 p.m.
New subject: [libvirt] [PATCHv2 5/9] conf: split <disk> schema into more pieces
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(a)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
4:43 p.m.
New subject: [libvirt] [PATCHv2 5/9] conf: split <disk> schema into more pieces
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(a)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>
4:08 p.m.
New subject: [libvirt] [PATCHv2 5/9] conf: split <disk> schema into more pieces
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
5:54 p.m.
New subject: [libvirt] [PATCHv2 6/9] conf: move storage source details to common RNG file
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(a)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
5:54 p.m.
New subject: [libvirt] [PATCHv2 7/9] conf: move <auth> and <encryption> to disk source
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(a)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
5:54 p.m.
New subject: [libvirt] [PATCHv2 8/9] conf: fix omission of <driver> in domain dumpxml
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(a)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
3:51 a.m.
New subject: [libvirt] [PATCHv2 8/9] conf: fix omission of <driver> in domain dumpxml
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
5:54 p.m.
New subject: [libvirt] [PATCHv2 9/9] RFC: conf: introduce <format> as synonym of <driver type=...>
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(a)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
4:26 a.m.
New subject: [libvirt] [PATCHv2 9/9] RFC: conf: introduce <format> as synonym of <driver type=...>
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
4:03 a.m.
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
3868
days inactive
3875
days old
18 comments
2 participants
participants (2)
-
Eric Blake -
Jiri Denemark