Hi John, Thanks for the review. On Thu, May 17, 2018 at 3:45 AM, John Ferlan <jfer...@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....@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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list