[libvirt] [PATCH] schemas: Allow all generic elements and attributes for all interfaces
There are some interface types (notably 'server' and 'client') which instead of allowing the default set of elements and attributes (like the rest do), try to enumerate only the elements they know of. This way it's, however, easy to miss something. For instance, the <address/> element was not mentioned at all. This resulted in a strange behavior: when such interface was added into XML, the address was automatically generated by parsing code. Later, the formatted XML hasn't passed the RNG schema. This became more visible once we've turned on the XML validation on domain XML changes: appending an empty line at the end of formatted XML (to trick virsh think the XML had changed) made libvirt to refuse the very same XML it formatted. Instead of trying to find each element and attribute we are missing in the schema, lets just allow all the elements and attributes like we're doing that for the rest of types. It's no harm if the schema is wider than our parser allows. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/schemas/domaincommon.rng | 28 +--- .../qemuxml2argv-interface-server.xml | 157 +++++++++++++++++++++ 2 files changed, 159 insertions(+), 26 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-interface-server.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9d6c1ee..d467dce 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2173,14 +2173,7 @@ </attribute> <empty/> </element> - <optional> - <element name="mac"> - <attribute name="address"> - <ref name="uniMacAddr"/> - </attribute> - <empty/> - </element> - </optional> + <ref name="interface-options"/> </interleave> </group> <group> @@ -2199,24 +2192,7 @@ </attribute> <empty/> </element> - <optional> - <element name="mac"> - <attribute name="address"> - <ref name="uniMacAddr"/> - </attribute> - <empty/> - </element> - </optional> - <optional> - <element name="model"> - <attribute name="type"> - <data type="string"> - <param name='pattern'>[a-zA-Z0-9\-_]+</param> - </data> - </attribute> - <empty/> - </element> - </optional> + <ref name="interface-options"/> </interleave> </group> <group> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-interface-server.xml b/tests/qemuxml2argvdata/qemuxml2argv-interface-server.xml new file mode 100644 index 0000000..9edf773 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-interface-server.xml @@ -0,0 +1,157 @@ +<domain type='kvm'> + <name>gentoo</name> + <uuid>a75aca4b-a02f-2bcb-4a91-c93cd848c34b</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <memoryBacking> + <hugepages> + <page size='1048576' unit='KiB' nodeset='0-3'/> + </hugepages> + </memoryBacking> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type> + <boot dev='hd'/> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='custom' match='exact'> + <model fallback='allow'>Haswell</model> + <vendor>Intel</vendor> + <feature policy='require' name='tm2'/> + <feature policy='require' name='est'/> + <feature policy='require' name='vmx'/> + <feature policy='require' name='osxsave'/> + <feature policy='require' name='smx'/> + <feature policy='require' name='ss'/> + <feature policy='require' name='ds'/> + <feature policy='require' name='vme'/> + <feature policy='require' name='dtes64'/> + <feature policy='require' name='abm'/> + <feature policy='require' name='ht'/> + <feature policy='require' name='acpi'/> + <feature policy='require' name='pbe'/> + <feature policy='require' name='tm'/> + <feature policy='require' name='pdcm'/> + <feature policy='require' name='pdpe1gb'/> + <feature policy='require' name='ds_cpl'/> + <feature policy='require' name='rdrand'/> + <feature policy='require' name='f16c'/> + <feature policy='require' name='xtpr'/> + <feature policy='require' name='monitor'/> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + <cell id='1' cpus='1' memory='1048576' unit='KiB'/> + <cell id='2' cpus='2' memory='1048576' unit='KiB'/> + <cell id='3' cpus='3' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='yes'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='floppy'> + <driver name='qemu' type='raw' cache='none'/> + <source file='/var/lib/libvirt/images/fd.img'/> + <target dev='fda' bus='fdc'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/gentoo.qcow2'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/OtherDemo.img'/> + <target dev='vdb' bus='virtio'/> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw' cache='none'/> + <source file='/home/zippy/tmp/install-amd64-minimal-20140619.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <shareable/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </controller> + <controller type='fdc' index='0'/> + <interface type='network'> + <mac address='52:54:00:d6:c0:0b'/> + <source network='default'/> + <bandwidth> + <inbound average='100'/> + <outbound average='100'/> + </bandwidth> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <interface type='server'> + <mac address='52:54:00:22:c9:42'/> + <source address='127.0.0.1' port='1234'/> + <bandwidth> + <inbound average='1234'/> + <outbound average='5678'/> + </bandwidth> + <model type='rtl8139'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </interface> + <interface type='client'> + <mac address='52:54:00:8c:b1:f8'/> + <source address='127.0.0.1' port='1234'/> + <model type='rtl8139'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <serial type='pty'> + <target port='1'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/gentoo.org.qemu.guest_agent.0'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes'/> + <sound model='ich6'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </sound> + <video> + <model type='cirrus' vram='16384' heads='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon> + </devices> +</domain> -- 2.0.5
On Wed, Jan 28, 2015 at 06:23:08PM +0100, Michal Privoznik wrote:
There are some interface types (notably 'server' and 'client') which instead of allowing the default set of elements and attributes (like the rest do), try to enumerate only the elements they know of. This way it's, however, easy to miss something. For instance, the <address/> element was not mentioned at all. This resulted in a strange behavior: when such interface was added into XML, the address was automatically generated by parsing code. Later, the formatted XML hasn't passed the RNG schema. This became more visible once we've turned on the XML validation on domain XML changes: appending an empty line at the end of formatted XML (to trick virsh think the XML had changed) made libvirt to refuse the very same XML it formatted.
Instead of trying to find each element and attribute we are missing in the schema, lets just allow all the elements and attributes like we're doing that for the rest of types. It's no harm if the schema is wider than our parser allows.
I'm pretty sure that separating it was the original intention, but until it's separated properly without breaking things, I agree with this fix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/schemas/domaincommon.rng | 28 +--- .../qemuxml2argv-interface-server.xml | 157 +++++++++++++++++++++
So this file is added only for domainschematest? Why not check it by xml2xml as well? This effectively allows interface-options to be used with any interface, why not move it after the <choice/> element, so we're not redundant? ACK if you fix those two things.
On 29.01.2015 11:25, Martin Kletzander wrote:
On Wed, Jan 28, 2015 at 06:23:08PM +0100, Michal Privoznik wrote:
There are some interface types (notably 'server' and 'client') which instead of allowing the default set of elements and attributes (like the rest do), try to enumerate only the elements they know of. This way it's, however, easy to miss something. For instance, the <address/> element was not mentioned at all. This resulted in a strange behavior: when such interface was added into XML, the address was automatically generated by parsing code. Later, the formatted XML hasn't passed the RNG schema. This became more visible once we've turned on the XML validation on domain XML changes: appending an empty line at the end of formatted XML (to trick virsh think the XML had changed) made libvirt to refuse the very same XML it formatted.
Instead of trying to find each element and attribute we are missing in the schema, lets just allow all the elements and attributes like we're doing that for the rest of types. It's no harm if the schema is wider than our parser allows.
I'm pretty sure that separating it was the original intention, but until it's separated properly without breaking things, I agree with this fix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/schemas/domaincommon.rng | 28 +--- .../qemuxml2argv-interface-server.xml | 157 +++++++++++++++++++++
So this file is added only for domainschematest? Why not check it by xml2xml as well?
Good point.
This effectively allows interface-options to be used with any interface, why not move it after the <choice/> element, so we're not redundant?
Not a good point. I mean, I don't think one is allowed to: <element> <interlave> <choice> <group/> <group/> <choice> </interleave> </element> simply because the choice groups may be defined by the value of an attribute to the top parent element. Well, at least if I did it that way I got an error from xmllint (and don't even get me started on it's error reporting capabilities. For a long while I thought we have the worst error reporting ever. Then I met xmllint).
ACK if you fix those two things.
So I'm fixing the first issue and pushing. Thanks. Michal
On Thu, Jan 29, 2015 at 04:12:15PM +0100, Michal Privoznik wrote:
On 29.01.2015 11:25, Martin Kletzander wrote:
On Wed, Jan 28, 2015 at 06:23:08PM +0100, Michal Privoznik wrote:
There are some interface types (notably 'server' and 'client') which instead of allowing the default set of elements and attributes (like the rest do), try to enumerate only the elements they know of. This way it's, however, easy to miss something. For instance, the <address/> element was not mentioned at all. This resulted in a strange behavior: when such interface was added into XML, the address was automatically generated by parsing code. Later, the formatted XML hasn't passed the RNG schema. This became more visible once we've turned on the XML validation on domain XML changes: appending an empty line at the end of formatted XML (to trick virsh think the XML had changed) made libvirt to refuse the very same XML it formatted.
Instead of trying to find each element and attribute we are missing in the schema, lets just allow all the elements and attributes like we're doing that for the rest of types. It's no harm if the schema is wider than our parser allows.
I'm pretty sure that separating it was the original intention, but until it's separated properly without breaking things, I agree with this fix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/schemas/domaincommon.rng | 28 +--- .../qemuxml2argv-interface-server.xml | 157 +++++++++++++++++++++
So this file is added only for domainschematest? Why not check it by xml2xml as well?
Good point.
This effectively allows interface-options to be used with any interface, why not move it after the <choice/> element, so we're not redundant?
Not a good point. I mean, I don't think one is allowed to: <element> <interlave> <choice> <group/> <group/> <choice> </interleave> </element>
simply because the choice groups may be defined by the value of an attribute to the top parent element. Well, at least if I did it that way I got an error from xmllint (and don't even get me started on it's error reporting capabilities. For a long while I thought we have the worst error reporting ever. Then I met xmllint).
Um... Yes! And I've found out why. It's simply because of that reference to the 'interface-options' definition. And you can't get that info from xmllint. Anyway, it is possible, albeit not so beautiful patch-wise. I'll Cc you on the patches that fix that.
ACK if you fix those two things.
So I'm fixing the first issue and pushing. Thanks.
Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik