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.

Signed-off-by: Ján Tomko <jtomko@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
+virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED,
+                             virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+                             virDomainDeviceInfoPtr info,
+                             void *data)
+{
+    virHashTablePtr bootHash = data;
+    char *order = NULL;
+    int ret = -1;
+
+    if (info->bootIndex == 0)
+        return 0;
+
+    if (virAsprintf(&order, "%u", info->bootIndex) < 0)
+        goto cleanup;
+
+    if (virHashLookup(bootHash, order)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("boot order '%s' used for more than one device"),
+                       order);
+        goto cleanup;
+    }
+
+    if (virHashAddEntry(bootHash, order, (void *) 1) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(order);
+    return ret;
+}
+
+
+static int
+virDomainDefCheckBootOrder(virDomainDefPtr def)
+{
+    virHashTablePtr bootHash = NULL;
+    int ret = -1;
+
+    if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
+        return 0;
+
+    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;
+    }
+
+    ret = 0;
+
+ cleanup:
+    virHashFree(bootHash);
+    return ret;
+}
+
+
 static int
 virDomainDefPostParseCommon(virDomainDefPtr def,
                             struct virDomainDefPostParseDeviceIteratorData *data,
-                            virHashTablePtr bootHash)
+                            virHashTablePtr bootHash ATTRIBUTE_UNUSED)
 {
     size_t i;
 
@@ -4953,20 +5022,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
         return -1;
     }
 
-    if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && bootHash) {
-        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"));
-            return -1;
-        }
-
-        if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
-            def->os.nBootDevs = 1;
-            def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
-        }
-    }
-
     if (virDomainVcpuDefPostParse(def) < 0)
         return -1;
 
@@ -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)
+        return -1;
+
     if (virDomainDefPostParseTimer(def) < 0)
         return -1;
 
diff --git a/tests/qemuargv2xmldata/nomachine-aarch64.xml b/tests/qemuargv2xmldata/nomachine-aarch64.xml
index eb8f9db803..9492423389 100644
--- a/tests/qemuargv2xmldata/nomachine-aarch64.xml
+++ b/tests/qemuargv2xmldata/nomachine-aarch64.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='aarch64' machine='virt'>hvm</type>
+    <boot dev='hd'/>
   </os>
   <features>
     <gic version='2'/>
diff --git a/tests/qemuargv2xmldata/nomachine-ppc64.xml b/tests/qemuargv2xmldata/nomachine-ppc64.xml
index 439f9e9ac6..1f15a950e3 100644
--- a/tests/qemuargv2xmldata/nomachine-ppc64.xml
+++ b/tests/qemuargv2xmldata/nomachine-ppc64.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='ppc64' machine='pseries'>hvm</type>
+    <boot dev='hd'/>
   </os>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/qemuargv2xmldata/nomachine-x86_64.xml b/tests/qemuargv2xmldata/nomachine-x86_64.xml
index 71a36f0833..33cde4c55a 100644
--- a/tests/qemuargv2xmldata/nomachine-x86_64.xml
+++ b/tests/qemuargv2xmldata/nomachine-x86_64.xml
@@ -6,6 +6,7 @@
   <vcpu placement='static'>1</vcpu>
   <os>
     <type arch='x86_64' machine='pc-0.11'>hvm</type>
+    <boot dev='hd'/>
   </os>
   <features>
     <acpi/>
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'/>
   </os>
   <clock offset='variable' adjustment='0' basis='utc'/>
   <on_poweroff>destroy</on_poweroff>
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:
>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.
>

How about:
Now it will also be run on domains created by other means than XML
parsing, since it will be run even for code paths that did not supply
the bootHash table before.

>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@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 ...

I can rename it to virDomainDefBootOrderPostParse

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, May 29, 2018 at 09:55:18 +0200, Ján Tomko wrote:
> On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:
> > 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.
> > 
> 
> How about:
> Now it will also be run on domains created by other means than XML
> parsing, since it will be run even for code paths that did not supply
> the bootHash table before.

Sounds goog to me.

> 
> > 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@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 ...
> 
> I can rename it to virDomainDefBootOrderPostParse

ACK to that

> 
> Jano


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list