[libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
John Ferlan
jferlan at redhat.com
Wed May 16 22:15:31 UTC 2018
$SUBJ:
Is a bit long - goal is <= 70-ish characters
On 05/14/2018 07:15 AM, Prerna Saxena wrote:
> Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk.
> Given that NVRAM data could be network-backed for high availability, this patch
> defines XML spec for serving loader & nvram disks via the network.
>
> Signed-off-by: Prerna Saxena <saxenap.ltc at gmail.com>
> ---
> docs/schemas/domaincommon.rng | 108 +++++++++++++++++++++++++++++++++++-------
> 1 file changed, 90 insertions(+), 18 deletions(-)
>
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.
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 0a6b29b..a6207db 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,42 @@
> </choice>
> </attribute>
> </optional>
> - <ref name="absFilePath"/>
> + <optional>
> + <attribute name="backing">
> + <choice>
> + <value>file</value>
> + <value>network</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> + <choice>
> + <group>
> + <ref name="absFilePath"/>
> + </group>
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.
> + <group>
> + <ref name="diskSourceFileElement"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolNBD"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolGluster"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolRBD"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolISCSI"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolHTTP"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolSimple"/>
> + </group>
> + </choice>
> + </optional>
> </element>
> </optional>
> <optional>
> @@ -287,7 +322,40 @@
> </attribute>
> </optional>
> <optional>
> - <ref name="absFilePath"/>
> + <attribute name="backing">
> + <choice>
> + <value>file</value>
> + <value>network</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> + <choice>
> + <group>
> + <ref name="absFilePath"/>
> + </group>
This is slightly different as absFilePath is optional...
> + <group>
> + <ref name="diskSourceFileElement"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolNBD"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolGluster"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolRBD"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolISCSI"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolHTTP"/>
> + </group>
> + <group>
> + <ref name="diskSourceNetworkProtocolSimple"/>
> + </group>
> + </choice>
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.
> </optional>
> </element>
> </optional>
> @@ -1494,25 +1562,29 @@
> </attribute>
> </optional>
> <optional>
> - <element name="source">
> - <optional>
> - <attribute name="file">
> - <ref name="absFilePath"/>
> - </attribute>
> - </optional>
> - <optional>
> - <ref name="storageStartupPolicy"/>
> - </optional>
> - <optional>
> - <ref name="encryption"/>
> - </optional>
> - <zeroOrMore>
> - <ref name='devSeclabel'/>
> - </zeroOrMore>
> - </element>
> + <ref name='diskSourceFileElement'/>
> </optional>
> </define>
>
> + <define name="diskSourceFileElement">
> + <element name="source">
> + <optional>
> + <attribute name="file">
> + <ref name="absFilePath"/>
> + </attribute>
> + </optional>
> + <optional>
> + <ref name="storageStartupPolicy"/>
> + </optional>
> + <optional>
> + <ref name="encryption"/>
> + </optional>
> + <zeroOrMore>
> + <ref name='devSeclabel'/>
> + </zeroOrMore>
> + </element>
> + </define>
> +
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
> <define name="diskSourceBlock">
> <attribute name="type">
> <value>block</value>
>
More information about the libvir-list
mailing list