Hi John,
Thanks for the review.
On Thu, May 17, 2018 at 3:45 AM, John Ferlan 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
> > ---
> > 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 and 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:
>
> /usr/lib/xen/boot/hvmloader
>
> to
>
>
>
>
>
> 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 which may
> clear up some of the "odd" pieces of the grammar below.
>
>
Just accounting for network source alone using and directly
specifying absolute paths
does not look like a clean design to me. When the 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 @@
> >
> >
> >
> > -
> > +
> > +
> > +
> > + file
> > + network
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
>
> 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
> 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.
>
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
