[libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.
Prerna
saxenap.ltc at gmail.com
Mon May 21 09:43:31 UTC 2018
Hi John,
Thanks for the review.
On Thu, May 17, 2018 at 3:45 AM, John Ferlan <jferlan at redhat.com> wrote:
> $SUBJ:
>
> Is a bit long - goal is <= 70-ish characters
>
Agree, I'll fix this.
>
> 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.
>
>
I had thought make rpm ran both, but later found that it did not. Have the
next series ready which :
(1) combines all the related patches into ones that do not break the build.
(2) Pass make check and syntax-check.
> 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.
>
Did not include that since I was not sure if it is really useful.
>
> 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.
>
>
Just accounting for network source alone using <source> and directly
specifying absolute paths
does not look like a clean design to me. When the <source> tag can describe
all storage sources,
we can unify the parsing and formatting of data using the helpers for
virStorageSource*.
If not, redundant code needs to be maintained for treating the two types
separately.
Please note that I am maintaining helpers to format-as-you-read, because
that seems to be a requirement.
But the underlying implementation should be able to unify code treating
these two formats as mere
rendering choices.
> > 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.
>
absFilePath will become optional as there is now > 1 way to specify the
storage. This is an intended side effect of broadening the spec.
>
> I would assume that if it's not optional here for absFilePath, then the
> <source> cannot be optional as well.
>
A user can specify either absFilePath, or the description of the backing
source using the various diskSource* elements.
Individually, neither of them are mandatory, but at least one of those
needs to be provided.
The code takes care of this case but unfortunately I could not find a way
to specify "exactly one of.. " relation in the schema.
>
> > + <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.
>
>
I believe we need that for consistency's sake. I do not see any benefit of
using "source" for just network elements alone, and directly specifying
file paths the old way. I agree it seems like a less invasive way, but it
makes both the code and documentation very counter-intuitive.
My next set of patches which pass unit tests and syntax-check are ready,
and I think I will post them to the list.
Maybe it is easier to discuss the XML semantics over a saner patchqueue.
Thanks,
Prerna
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180521/38641286/attachment-0001.htm>
More information about the libvir-list
mailing list