Re: [libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.

$SUBJ: Is a bit long - goal is <= 70-ish characters On 05/14/2018 07:15 AM, Prerna Saxena wrote: First off: applying all the patches and running make w/ "check" and "syntax-check" fails during "check" w/ qemuxml2argvtest and qemuxml2xmltest failing. Before posting patches those must pass for each patch. When I get to patch 2, the build breaks. While one can appreciate having less to review in each patch, it's not possible to accept or review patches which cause a build break. Shouldn't be up to the reviewer to figure out how to make the series work. The rule is - each patch must be separately compileable and capable of passing check and syntax-check. If one ever needs to git bisect to determine where a problem lies, it'd be very awful if the build broke. As for this patch in particular, there are two things going on in this patch - #1 altering the <loader> and <nvram> schema and #2 extracting out part of the diskSourceFile to be used in #1. Ironically, Thing 2 is creating a referenced name to be included in part of Thing 1; however, Thing 1 is essentially a cut-n-paste of the same thing. All those elements that are cut-n-pasted are part of diskSourceNetwork, except you didn't include VxHS in your list. Still, a question in my mind is whether you really need to format the file using source. If the goal is to provide the ability to access networked storage, why provide the option to allow someone to change their XML from: <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> to <loader type='rom' backing='file'> <source file='/usr/lib/xen/boot/hvmloader'/> </loader> Yes, they are equivalent, but it seems the reason for it was because you wanted to format the former into the latter at one point in time. If you limit to network only, then perhaps your optional attribute changes to be network='yes' which means to parse a <source> which may clear up some of the "odd" pieces of the grammar below. This won't be equivalent to what you started with. Prior to this change absFilePath was required, now it would be optional. I would assume that if it's not optional here for absFilePath, then the <source> cannot be optional as well. This is slightly different as absFilePath is optional... RNG format not my area of expertise - I usually copy, paste, and pray it works. Still the above looks really strange with all those <group> .. </group>... After a bit of playing I came up with the following diffs from master - although I'm not quite sure if they work later on: For loader: - <ref name="absFilePath"/> + <optional> + <ref name="osLoaderNvramBacking"/> + </optional> + <ref name="osLoaderNvramSource"/> For nvram: - <ref name="absFilePath"/> + <ref name="osLoaderNvramBacking"/> + </optional> + <optional> + <ref name="osLoaderNvramSource"/> and just before <define name="ostypexen"> + + <define name="osLoaderNvramBacking"> + <attribute name="backing"> + <choice> + <value>file</value> + <value>network</value> + </choice> + </attribute> + </define> + + <define name="osLoaderNvramSource"> + <choice> + <group> + <ref name="absFilePath"/> + <empty/> + </group> + <group> + <ref name="diskSourceFileElement"/> + <ref name="diskSourceNetworkProtocolNBD"/> + <ref name="diskSourceNetworkProtocolGluster"/> + <ref name="diskSourceNetworkProtocolRBD"/> + <ref name="diskSourceNetworkProtocolISCSI"/> + <ref name="diskSourceNetworkProtocolHTTP"/> + <ref name="diskSourceNetworkProtocolSimple"/> + </group> + </choice> + </define> + But again - I'm not the expert... maybe someone else will have some ideas/help in this area. At the very least using the <define>'s in order to reduce the cut-copy-paste for loader and nvram is a must. How exactly the grammar has to look to make things work - that's TBD. I would have extracted this patch into it's own patch, but as noted above - I'm not convinced you need to have the <source> element using a file attribute. John