<div dir="ltr">Hi John,<div>Thanks for the review.<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 17, 2018 at 3:45 AM, John Ferlan <span dir="ltr"><<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">$SUBJ:<br>
<br>
Is a bit long - goal is <= 70-ish characters<br></blockquote><div><br></div><div>Agree, I'll fix this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
On 05/14/2018 07:15 AM, Prerna Saxena wrote:<br>
> Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk.<br>
> Given that NVRAM data could be network-backed for high availability, this patch<br>
> defines XML spec for serving loader & nvram disks via the network.<br>
> <br>
> Signed-off-by: Prerna Saxena <<a href="mailto:saxenap.ltc@gmail.com">saxenap.ltc@gmail.com</a>><br>
> ---<br>
>  docs/schemas/domaincommon.rng | 108 ++++++++++++++++++++++++++++++<wbr>+++++-------<br>
>  1 file changed, 90 insertions(+), 18 deletions(-)<br>
> <br>
<br>
</span>First off: applying all the patches and running make w/ "check" and<br>
"syntax-check" fails during "check" w/ qemuxml2argvtest and<br>
qemuxml2xmltest failing.<br>
<br></blockquote><div><br></div><div>I had thought make rpm ran both, but later found that it did not. Have the next series ready which :</div><div>(1) combines all the related patches into ones that do not break the build.</div><div>(2) Pass make check and syntax-check.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Before posting patches those must pass for each patch. When I get to<br>
patch 2, the build breaks. While one can appreciate having less to<br>
review in each patch, it's not possible to accept or review patches<br>
which cause a build break. Shouldn't be up to the reviewer to figure out<br>
how to make the series work. The rule is - each patch must be separately<br>
compileable and capable of passing check and syntax-check. If one ever<br>
needs to git bisect to determine where a problem lies, it'd be very<br>
awful if the build broke.<br>
<br>
As for this patch in particular, there are two things going on in this<br>
patch - #1 altering the <loader> and <nvram> schema and #2 extracting<br>
out part of the diskSourceFile to be used in #1.  Ironically, Thing 2 is<br>
creating a referenced name to be included in part of Thing 1; however,<br>
Thing 1 is essentially a cut-n-paste of the same thing. All those<br>
elements that are cut-n-pasted are part of diskSourceNetwork, except you<br>
didn't include VxHS in your list.<br></blockquote><div><br></div><div>Did not include that since I was not sure if it is really useful.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Still, a question in my mind is whether you really need to format the<br>
file using source. If the goal is to provide the ability to access<br>
networked storage, why provide the option to allow someone to change<br>
their XML from:<br>
<br>
<loader type='rom'>/usr/lib/xen/boot/<wbr>hvmloader</loader><br>
<br>
to<br>
<br>
<loader type='rom' backing='file'><br>
  <source file='/usr/lib/xen/boot/<wbr>hvmloader'/><br>
</loader><br>
<br>
Yes, they are equivalent, but it seems the reason for it was because you<br>
wanted to format the former into the latter at one point in time.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If you limit to network only, then perhaps your optional attribute<br>
changes to be network='yes' which means to parse a <source> which may<br>
clear up some of the "odd" pieces of the grammar below.<br>
<span class=""><br></span></blockquote><div><br></div><div>Just accounting for network source alone using <source> and directly specifying absolute paths </div><div>does not look like a clean design to me. When the <source> tag can describe all storage sources, </div><div>we can unify the parsing and formatting of data using the helpers for virStorageSource*.</div><div>If not, redundant code needs to be maintained for treating the two types separately.</div><div><br></div><div>Please note that I am maintaining helpers to format-as-you-read, because that seems to be a requirement.</div><div>But the underlying implementation should be able to unify code treating these two formats as mere </div><div>rendering choices.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> diff --git a/docs/schemas/domaincommon.<wbr>rng b/docs/schemas/domaincommon.<wbr>rng<br>
> index 0a6b29b..a6207db 100644<br>
> --- a/docs/schemas/domaincommon.<wbr>rng<br>
> +++ b/docs/schemas/domaincommon.<wbr>rng<br>
> @@ -276,7 +276,42 @@<br>
>                  </choice><br>
>                </attribute><br>
>              </optional><br>
> -            <ref name="absFilePath"/><br>
> +            <optional><br>
> +              <attribute name="backing"><br>
> +                <choice><br>
> +                  <value>file</value><br>
> +                  <value>network</value><br>
> +                </choice><br>
> +              </attribute><br>
> +            </optional><br>
> +            <optional><br>
> +              <choice><br>
> +                 <group><br>
> +                  <ref name="absFilePath"/><br>
> +                 </group><br>
<br>
</span>This won't be equivalent to what you started with. Prior to this change<br>
absFilePath was required, now it would be optional.<br></blockquote><div><br></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">absFilePath will become optional as there is now > 1 way to specify the storage. This is an intended side effect of broadening the spec.</span><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would assume that if it's not optional here for absFilePath, then the<br>
<source> cannot be optional as well.<br></blockquote><div><br></div><div>A user can specify either absFilePath, or the description of the backing source using the various diskSource* elements.</div><div>Individually, neither of them are mandatory, but at least one of those needs to be provided.</div><div>The code takes care of this case but unfortunately I could not find a way to specify "exactly one of.. " relation in the schema. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +                 <group><br>
> +                  <ref name="diskSourceFileElement"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolNBD"/<wbr>><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolGlust<wbr>er"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolRBD"/<wbr>><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolISCSI<wbr>"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolHTTP"<wbr>/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolSimpl<wbr>e"/><br>
> +                 </group><br>
> +              </choice><br>
> +            </optional><br>
>            </element><br>
>          </optional><br>
>          <optional><br>
> @@ -287,7 +322,40 @@<br>
>                </attribute><br>
>              </optional><br>
>              <optional><br>
> -              <ref name="absFilePath"/><br>
> +              <attribute name="backing"><br>
> +                <choice><br>
> +                  <value>file</value><br>
> +                  <value>network</value><br>
> +                </choice><br>
> +              </attribute><br>
> +            </optional><br>
> +            <optional><br>
> +              <choice><br>
> +                 <group><br>
> +                  <ref name="absFilePath"/><br>
> +                 </group><br>
<br>
</div></div>This is slightly different as absFilePath is optional...<br>
<span class=""><br>
> +                 <group><br>
> +                  <ref name="diskSourceFileElement"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolNBD"/<wbr>><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolGlust<wbr>er"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolRBD"/<wbr>><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolISCSI<wbr>"/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolHTTP"<wbr>/><br>
> +                 </group><br>
> +                 <group><br>
> +                  <ref name="<wbr>diskSourceNetworkProtocolSimpl<wbr>e"/><br>
> +                 </group><br>
> +              </choice><br>
<br>
<br>
<br>
</span>RNG format not my area of expertise - I usually copy, paste, and pray it<br>
works. Still the above looks really strange with all those <group> ..<br>
</group>... After a bit of playing I came up with the following diffs<br>
from master - although I'm not quite sure if they work later on:<br>
<br>
For loader:<br>
<span class=""><br>
-            <ref name="absFilePath"/><br>
+            <optional><br>
</span>+              <ref name="osLoaderNvramBacking"/><br>
+            </optional><br>
+            <ref name="osLoaderNvramSource"/><br>
<br>
<br>
For nvram:<br>
<span class=""><br>
-              <ref name="absFilePath"/><br>
</span>+              <ref name="osLoaderNvramBacking"/><br>
+            </optional><br>
+            <optional><br>
+              <ref name="osLoaderNvramSource"/><br>
<br>
<br>
and just before <define name="ostypexen"><br>
<br>
+<br>
+  <define name="osLoaderNvramBacking"><br>
<span class="">+    <attribute name="backing"><br>
+      <choice><br>
+        <value>file</value><br>
+        <value>network</value><br>
+      </choice><br>
+    </attribute><br>
</span>+  </define><br>
+<br>
+  <define name="osLoaderNvramSource"><br>
<span class="">+    <choice><br>
+      <group><br>
+        <ref name="absFilePath"/><br>
</span>+        <empty/><br>
<span class="">+      </group><br>
+      <group><br>
+        <ref name="diskSourceFileElement"/><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolNBD"/<wbr>><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolGlust<wbr>er"/><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolRBD"/<wbr>><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolISCSI<wbr>"/><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolHTTP"<wbr>/><br>
</span><span class="">+        <ref name="<wbr>diskSourceNetworkProtocolSimpl<wbr>e"/><br>
+      </group><br>
+    </choice><br>
</span>+  </define><br>
+<br>
<br>
But again - I'm not the expert... maybe someone else will have some<br>
ideas/help in this area.<br>
<br>
At the very least using the <define>'s in order to reduce the<br>
cut-copy-paste for loader and nvram is a must. How exactly the grammar<br>
has to look to make things work - that's TBD.<br>
<div><div class="h5"><br>
>              </optional><br>
>            </element><br>
>          </optional><br>
> @@ -1494,25 +1562,29 @@<br>
>        </attribute><br>
>      </optional><br>
>      <optional><br>
> -      <element name="source"><br>
> -        <optional><br>
> -          <attribute name="file"><br>
> -            <ref name="absFilePath"/><br>
> -          </attribute><br>
> -        </optional><br>
> -        <optional><br>
> -          <ref name="storageStartupPolicy"/><br>
> -        </optional><br>
> -        <optional><br>
> -          <ref name="encryption"/><br>
> -        </optional><br>
> -        <zeroOrMore><br>
> -          <ref name='devSeclabel'/><br>
> -        </zeroOrMore><br>
> -      </element><br>
> +      <ref name='diskSourceFileElement'/><br>
>      </optional><br>
>    </define><br>
>  <br>
> +  <define name="diskSourceFileElement"><br>
> +    <element name="source"><br>
> +      <optional><br>
> +        <attribute name="file"><br>
> +          <ref name="absFilePath"/><br>
> +        </attribute><br>
> +      </optional><br>
> +      <optional><br>
> +        <ref name="storageStartupPolicy"/><br>
> +      </optional><br>
> +      <optional><br>
> +        <ref name="encryption"/><br>
> +      </optional><br>
> +      <zeroOrMore><br>
> +        <ref name='devSeclabel'/><br>
> +      </zeroOrMore><br>
> +    </element><br>
> +  </define><br>
> +<br>
<br>
</div></div>I would have extracted this patch into it's own patch, but as noted<br>
above - I'm not convinced you need to have the <source> element using a<br>
file attribute.<br>
<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>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.</div><div><br></div><div>My next set of patches which pass unit tests and syntax-check are ready, and I think I will post them to the list.</div><div>Maybe it is easier to discuss the XML semantics over a saner patchqueue.</div></div><br></div></div><div class="gmail_extra">Thanks,</div><div class="gmail_extra">Prerna</div></div>