[libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

[Peter Krempa]

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 at redhat.com>
> ---
>  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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180529/e7d144b7/attachment-0001.sig>