Hi John,

Thanks for the review.

On Thu, May 17, 2018 at 3:45 AM, John Ferlan <jferlan@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@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