[PATCH] schema: Re-structure schema for <filesystem> to avoid broken validation
The validation of a '<filesystem type='mount'>' device fails if the elements inside are not ordered in the order in the schema despite using <interleave>. This is a bug in libxml2's validator as removing the '<optional>' property from the definition of the 'type' attribute with 'mount' variable fixes the problem. I've reported it as another instance of a seemingly related issue: https://gitlab.gnome.org/GNOME/libxml2/-/issues/131 Meanwhile libvirt can re-arrange the schema by extracting the common bits into a new definition and referencing them from each of the choice groups explicitly. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/392 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 365 +++++++++++++++--------------- 1 file changed, 186 insertions(+), 179 deletions(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 5ff15e8787..d346442510 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -2860,205 +2860,212 @@ </interleave> </element> </define> + + <define name="filesystemCommon"> + <interleave> + <element name="target"> + <attribute name="dir"/> + <empty/> + </element> + <optional> + <attribute name="accessmode"> + <choice> + <value>passthrough</value> + <value>mapped</value> + <value>squash</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="multidevs"> + <choice> + <value>default</value> + <value>remap</value> + <value>forbid</value> + <value>warn</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="fmode"> + <ref name="createMode"/> + </attribute> + </optional> + <optional> + <attribute name="dmode"> + <ref name="createMode"/> + </attribute> + </optional> + <optional> + <element name="readonly"> + <empty/> + </element> + </optional> + <optional> + <ref name="deviceBoot"/> + </optional> + <optional> + <ref name="alias"/> + </optional> + <optional> + <ref name="acpi"/> + </optional> + <optional> + <ref name="address"/> + </optional> + <optional> + <element name="space_hard_limit"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="space_soft_limit"> + <ref name="scaledInteger"/> + </element> + </optional> + </interleave> + </define> + <define name="filesystem"> <element name="filesystem"> - <interleave> - <choice> - <group> - <attribute name="type"> - <value>file</value> - </attribute> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <element name="source"> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - <empty/> - </element> - </interleave> - </group> - <group> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <element name="source"> - <attribute name="dev"> - <ref name="absFilePath"/> - </attribute> - <empty/> - </element> - </interleave> - </group> - <group> - <!-- type="mount" is default --> + <optional> + <attribute name="model"> + <choice> + <value>virtio</value> + <value>virtio-transitional</value> + <value>virtio-non-transitional</value> + </choice> + </attribute> + </optional> + <choice> + <group> + <attribute name="type"> + <value>file</value> + </attribute> + <interleave> <optional> - <attribute name="type"> - <value>mount</value> - </attribute> + <ref name="fsDriver"/> </optional> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <optional> - <ref name="fsBinary"/> - </optional> - <element name="source"> - <choice> - <group> - <attribute name="dir"> - <ref name="absDirPath"/> - </attribute> - </group> - <group> - <attribute name="socket"> - <ref name="absFilePath"/> - </attribute> - </group> - </choice> - <empty/> - </element> - </interleave> - </group> - <group> - <optional> - <attribute name="type"> - <value>bind</value> + <element name="source"> + <attribute name="file"> + <ref name="absFilePath"/> </attribute> + <empty/> + </element> + <ref name="filesystemCommon"/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <ref name="fsDriver"/> </optional> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <element name="source"> - <attribute name="dir"> - <ref name="absDirPath"/> - </attribute> - <empty/> - </element> - </interleave> - </group> - <group> - <attribute name="type"> - <value>template</value> - </attribute> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <element name="source"> - <attribute name="name"> - <ref name="genericName"/> - </attribute> - <empty/> - </element> - </interleave> - </group> - <group> - <attribute name="type"> - <value>ram</value> - </attribute> - <interleave> - <optional> - <ref name="fsDriver"/> - </optional> - <element name="source"> - <attribute name="usage"> - <ref name="unsignedLong"/> - </attribute> - <optional> - <attribute name="units"> - <ref name="unit"/> - </attribute> - </optional> - <empty/> - </element> - </interleave> - </group> - </choice> - <interleave> - <element name="target"> - <attribute name="dir"/> - <empty/> - </element> + <element name="source"> + <attribute name="dev"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + <ref name="filesystemCommon"/> + </interleave> + </group> + <group> + <!-- type="mount" is default --> <optional> - <attribute name="accessmode"> - <choice> - <value>passthrough</value> - <value>mapped</value> - <value>squash</value> - </choice> + <attribute name="type"> + <value>mount</value> </attribute> </optional> - <optional> - <attribute name="multidevs"> + <interleave> + <optional> + <ref name="fsDriver"/> + </optional> + <optional> + <ref name="fsBinary"/> + </optional> + <element name="source"> <choice> - <value>default</value> - <value>remap</value> - <value>forbid</value> - <value>warn</value> + <group> + <attribute name="dir"> + <ref name="absDirPath"/> + </attribute> + </group> + <group> + <attribute name="socket"> + <ref name="absFilePath"/> + </attribute> + </group> </choice> - </attribute> - </optional> - <optional> - <attribute name="fmode"> - <ref name="createMode"/> - </attribute> - </optional> - <optional> - <attribute name="dmode"> - <ref name="createMode"/> - </attribute> - </optional> - <optional> - <element name="readonly"> <empty/> </element> - </optional> - <optional> - <ref name="deviceBoot"/> - </optional> - <optional> - <ref name="alias"/> - </optional> - <optional> - <ref name="acpi"/> - </optional> + <ref name="filesystemCommon"/> + </interleave> + </group> + <group> <optional> - <ref name="address"/> + <attribute name="type"> + <value>bind</value> + </attribute> </optional> - </interleave> - <interleave> - <optional> - <element name="space_hard_limit"> - <ref name="scaledInteger"/> + <interleave> + <optional> + <ref name="fsDriver"/> + </optional> + <element name="source"> + <attribute name="dir"> + <ref name="absDirPath"/> + </attribute> + <empty/> </element> - </optional> - <optional> - <element name="space_soft_limit"> - <ref name="scaledInteger"/> + <ref name="filesystemCommon"/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>template</value> + </attribute> + <interleave> + <optional> + <ref name="fsDriver"/> + </optional> + <element name="source"> + <attribute name="name"> + <ref name="genericName"/> + </attribute> + <empty/> </element> - </optional> - </interleave> - <optional> - <attribute name="model"> - <choice> - <value>virtio</value> - <value>virtio-transitional</value> - <value>virtio-non-transitional</value> - </choice> + <ref name="filesystemCommon"/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>ram</value> </attribute> - </optional> - </interleave> + <interleave> + <optional> + <ref name="fsDriver"/> + </optional> + <element name="source"> + <attribute name="usage"> + <ref name="unsignedLong"/> + </attribute> + <optional> + <attribute name="units"> + <ref name="unit"/> + </attribute> + </optional> + <empty/> + </element> + <ref name="filesystemCommon"/> + </interleave> + </group> + </choice> </element> </define> + <define name="fsDriver"> <element name="driver"> <!-- Annoying inconsistency. "disk" uses "name" -- 2.37.3
On a Thursday in 2022, Peter Krempa wrote:
The validation of a '<filesystem type='mount'>' device fails if the elements inside are not ordered in the order in the schema despite using <interleave>. This is a bug in libxml2's validator as removing the '<optional>' property from the definition of the 'type' attribute with 'mount' variable fixes the problem.
I've reported it as another instance of a seemingly related issue:
Fun!
Meanwhile libvirt can re-arrange the schema by extracting the common bits into a new definition and referencing them from each of the choice groups explicitly.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/392 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 365 +++++++++++++++--------------- 1 file changed, 186 insertions(+), 179 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Oct 13, 2022 at 02:57:33PM +0200, Peter Krempa wrote:
The validation of a '<filesystem type='mount'>' device fails if the elements inside are not ordered in the order in the schema despite using <interleave>. This is a bug in libxml2's validator as removing the '<optional>' property from the definition of the 'type' attribute with 'mount' variable fixes the problem.
I've reported it as another instance of a seemingly related issue:
https://gitlab.gnome.org/GNOME/libxml2/-/issues/131
Meanwhile libvirt can re-arrange the schema by extracting the common bits into a new definition and referencing them from each of the choice groups explicitly.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/392 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 365 +++++++++++++++--------------- 1 file changed, 186 insertions(+), 179 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> We've had many of these schema ordering problems, and I wondered if we've got them all yet. To the surprise of absolutely no one, the answer is no. If we take the view that *every* single element should be re-orderable, which is reasonable as I've seen mgmt apps generate libvirt XML in totally arbitrary ordering, then this dumb script reverses everything we have in our samples: $ cat tests/xmlrngfuzz.py #!/usr/bin/python from lxml import etree import sys tree = etree.parse(sys.stdin) root = tree.getroot() def reverse(node): newe = [] for e in list(node): node.remove(e) newe.append(e) newe.reverse() for e in newe: node.append(e) pass for e in newe: reverse(e) reverse(root) print(etree.tostring(tree, pretty_print=True).decode('utf8')) $ for i in `find -name '*.xml' ` do echo $1 ./xmlrngfuzz.py < $i > ${i%%xml}rev.xml done $ ./build/tests/virschematest ... Some tests failed. Run them using: VIR_TEST_DEBUG=1 VIR_TEST_RANGE=2-3,7-8,11-15,17,19,21,26-27,29,32,34,39,41-44,51,54,58,62,71,93,108,139,206,221,306,309,357,369,410,470,504,513,529,568-569,590,648,651,660,678,719,741,771-772,775,854,883,921-922,933,1059,1150,1171,1177,1218,1254,1268,1288,1338,1345,1386,1398,1500,1541,1573,1649,1663,1674-1675,1686,1735,1760,1779,1782,1787,1791,1814,1896,1971,1980,2015,2056,2077,2114,2146,2174,2188,2209-2210,2214-2215,2217-2218,2231,2237,2240,2245,2258,2265,2267,2274,2278,2292,2299,2304,2306,2328,2343,2369,2374,2383,2397,2420,2424,2432,2451,2457-2458,2465,2477,2498,2517,2522,2533,2539,2542,2544,2546,2563,2584,2605,2615,2623-2624,2632,2637,2645,2653,2655,2681,2686,2700,2706,2724,2734,2747,2750,2753,2767,2791,2804,2810,2835-2836,2838,2841,2844,2858,2865,2876,2906,2934,2940,2942,2950,2959,2977,2985-2986,2988,2999,3019-3020,3033,3051,3054-3055,3065,3088,3114,3117,3122,3130-3131,3134,3144,3167,3171,3188,3213,3216,3222,3232,3236,3250,3275,3290,3292,3302,3307,3355,3370,3387,3400,3420,3427,3429,3436,3453,3467,3477,3486,3517,3519-3521,3523-3525,3530,3533-3536,3539,3543,3550,3578,3600,3649,3652,3657,3664,3668,3675,3693,3700,3704,3708,3743,3748,3768,3805,3808,3814,3885,3891,3913,3925-3926,3938,3947,3949,3960-3961,3964-3965,3998,4001,4004,4014,4031,4036,4048,4050,4056,4088,4093,4104,4166,4169-4170,4172-4173,4179,4181,4183,4185,4188-4191,4234,4240,4401,4477,4544,4550,4559-4562,4566-4567,4569,4571-4573,4576-4577,4579-4581,4583-4585,4587-4589,4592,4594,4597,4601-4602,4604-4605,4608-4610,4615,4621,4624,4626,4628,4633,4635,4639,4641,4643,4645-4646,4648-4651,4653-4654,4657,4659-4660,4663-4664,4679,4695,4700,4706,4715,4719,4739,4742,5107,5111,5113-5117,5121-5122,5125-5126,5130,5132-5133,5135,5137-5139,5145-5146,5149,5153-5155,5157,5163-5165,5167,5169-5173,5179-5180,5184,5187-5189,5191-5194,5197-5201,5203,5206-5207,5212-5213,5215-5216,5219,5222,5225,5230,5234,5236-5239,5242,5246,5248-5251,5253-5256,5260,5262,5264-5265,5267,5269-5272,5281-5284,5286-5288,5291,5293,5295,5298,5300-5302,5328-5330,5332,5335,5340,5343-5344,5347,5349-5350,5353-5357,5359,5362-5365,5368,5371,5373-5374,5376,5378-5381,5385,5387,5394-5395,5397,5401-5402,5404,5406-5408,5410,5412,5414,5416-5418,5421-5423,5427-5429,5432,5434-5436,5438-5440,5444-5446,5448-5451,5455-5456,5463,5466-5467,5469,5472,5475,5477-5478,5480-5481,5487-5488,5491,5493-5494,5513,5515,5529,5595,5607,5671,5673,5730,5772,5972,5981,5985,6004,6020,6022-6023,6045,6057,6078,6103,6105,6116,6124,6135 ./build/tests/virschematest Some work todo.... With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Oct 13, 2022 at 14:25:30 +0100, Daniel P. Berrangé wrote:
On Thu, Oct 13, 2022 at 02:57:33PM +0200, Peter Krempa wrote:
The validation of a '<filesystem type='mount'>' device fails if the elements inside are not ordered in the order in the schema despite using <interleave>. This is a bug in libxml2's validator as removing the '<optional>' property from the definition of the 'type' attribute with 'mount' variable fixes the problem.
I've reported it as another instance of a seemingly related issue:
https://gitlab.gnome.org/GNOME/libxml2/-/issues/131
Meanwhile libvirt can re-arrange the schema by extracting the common bits into a new definition and referencing them from each of the choice groups explicitly.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/392 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domaincommon.rng | 365 +++++++++++++++--------------- 1 file changed, 186 insertions(+), 179 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
We've had many of these schema ordering problems, and I wondered if we've got them all yet. To the surprise of absolutely no one, the answer is no.
I'm not surprised about that. I was surprised though about what this patch fixes as the schema is actually correct in this case. [...]
reverse(root) print(etree.tostring(tree, pretty_print=True).decode('utf8'))
$ for i in `find -name '*.xml' ` do echo $1 ./xmlrngfuzz.py < $i > ${i%%xml}rev.xml done
This also includes stuff like: - '*-invalid.xml' files - these are excluded from validation as they are intentionally invalid, but no longer match exclusion pattern after reversal - output-only XML such as capabilities/domcapabilities - we are strictly defining the order there, I'm not sure if it makes sense to allow interleaving Disregarding the above it's not that bad. I've seen ~40 instances e.g. in qemuxml2argvdata boiling down to 7 actual mistakes in the scheama. I'll have a look at some more input xmls, but I don't feel like we should touch the schema for output-only ones.
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa