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@xxxxxxxxx> > --- > 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 ... -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
