On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
> Move the check for boot elements into a separate function
> and remove its dependency on the parser-supplied bootHash table.
>
> Reconstructing the hash table from the domain definition
> effectively duplicates the check for duplicate boot order
> values, also present in virDomainDeviceBootParseXML.
So the semantical difference is that places that call into the
post-parse infrastructure which construct the domain definition object
by other means that parsing XML will get the same treatment as when XML
is parsed.
>
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 89 +++++++++++++++++++++++-----
> tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 +
> tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 +
> tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 +
> tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 +
> 5 files changed, 78 insertions(+), 15 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d6ac47c629..f087a3680f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
[...]
> +
> +
> +static int
> +virDomainDefCheckBootOrder(virDomainDefPtr def)
[1]
> +{
> + virHashTablePtr bootHash = NULL;
> + int ret = -1;
> +
> + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
> + return 0;
Please do this check outside of this function. If this function is
invoked it should o it's job, especially since you also have other
conditions when to invoke it outside.
> +
> + if (!(bootHash = virHashCreate(5, NULL)))
> + goto cleanup;
> +
> + if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0)
> + goto cleanup;
> +
> + if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("per-device boot elements cannot be used"
> + " together with os/boot elements"));
> + goto cleanup;
> + }
> +
> + if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
> + def->os.nBootDevs = 1;
> + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
[1] this in contrast to the name of the function modifies stuff ...
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + virHashFree(bootHash);
> + return ret;
> +}
> +
> +
[...]
> @@ -4979,6 +5034,10 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
> if (virDomainDefRejectDuplicatePanics(def) < 0)
> return -1;
>
> + if (!(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) &&
> + virDomainDefCheckBootOrder(def) < 0)
Add the condition for checking HVM here.
> + return -1;
> +
> if (virDomainDefPostParseTimer(def) < 0)
> return -1;
>
[...]
> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> index afb9030681..a3d54ae3c1 100644
> --- a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> @@ -10,6 +10,7 @@
> <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel>
> <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd>
> <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os </cmdline>
> + <boot dev='hd'/>
This looks wrong here since we do have the 'kernel' and 'initrd'
elements. Given that it's caused by the semantic difference I think it's
okay.
> </os>
> <clock offset='variable' adjustment='0' basis='utc'/>
> <on_poweroff>destroy</on_poweroff>
ACK if you move out the condition and note the semantic impact in the
commit message.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
