[libvirt] [ v3 1/4] Schema: Introduce XML schema for network-backed loader and nvram elements.

[John Ferlan]

That's a long $SUBJ....

On 05/21/2018 07:10 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.
> 

...and these are long lines too.

I'm still not convinced you have the "optimal" split of patches.

Eventually though this patch should be split up (more on that in patch 2
review). In the long run, I think you should alter <loader> first, then
alter <nvram> - and it needs to be done in a slower, more progressive
manner.  The schema would be included into a patch when the <source>
parse/format is done in domain_conf and tests would be added in order to
validate (and as has been pointed out to me recently, we should use the
genericxml2xmltest for at least the parse/format tests).

After reviewing the patches, I'm still a bit "confused" with respect to
how one can uniquely "name" or find the <loader> or <nvram> file when
using the network <source> protocol. Eventually "something" has to be
opened for the loader (e.g. CODE file) and nvram (e.g. VARS file). The
current syntax and the changed "file" syntax would allow whatever name
is chosen, but since the network syntax may not (e.g. the case of iSCSI)
provide a name, but only a /dev/sgN device. So how is the file found?
Isn't that something QEMU will need?

Consider these two <loader> examples:

tests/qemuxml2argvdata/bios-nvram-secure.xml:    <loader readonly='yes'
secure='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.secboot.fd</loader>

tests/qemuxml2argvdata/bios-nvram.xml:    <loader readonly='yes'
type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>

or these two <nvram> examples:

tests/qemuxml2argvdata/q35-acpi-uefi.xml:
<nvram>/var/lib/libvirt/qemu/nvram/guest_VARS.fd</nvram>

tests/qemuxml2xmloutdata/bios-nvram.xml:
<nvram>/usr/share/OVMF/OVMF_VARS.fd</nvram>

With a network path such as your example:

+    <nvram>
+      <source protocol='iscsi' name='iqn.1992-01.com.example/0'>
+        <host name='example.org' port='6000'/>
+      </source>
+    </nvram>

How do you distinguish? Does that mean the someone has to "know" what's
on each exposed LUN?


> Signed-off-by: Prerna Saxena <saxenap.ltc at gmail.com>
> ---
>  docs/schemas/domaincommon.rng | 108 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 90 insertions(+), 18 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 71ac3d0..64f4319 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>

You don't need "backing"... Either the old format is found or some sort
of <source...> format is found. If <source> is found, then you know that
you have either a "file" or "protocol" attribute and can use that to key
or set the StorageSourcePtr->type to FILE or NETWORK prior to calling
the storage source parse function.

> +            <optional>
> +              <choice>
> +                 <group>

NB: Each of the <group> and </group> entries are indented by 1 space too
many.

> +                  <ref name="absFilePath"/>
> +                 </group>
> +                 <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>

This entire hunk is duplicated for loader and nvram, so make it a
<define name="sourceLoaderNvram"> (or something like that) and it can
then be used to replace the <ref name="absFilePath"/> with <ref
name="sourceLoaderNvram"/>


John

...