From a9efe2d70cd43a5c60163a1873a7adc146e9efaf Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Mon, 14 Apr 2014 16:54:12 -0600
Subject: [PATCH] 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@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 8c1724a3d0..7fc0cfffcb 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 f01312f533..b15fd63e9e 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 e96f76eae8..95d5be2904 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 0000000000..f01312f533
--- /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 0000000000..e96f76eae8
--- /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 c8a1c10691..788adbe386 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");
-- 
GitLab