conf: fix bogus error when <boot order='1'/> is in an <interface type='hostdev'>

[libvirt] [PATCH] conf: fix bogus error when <boot order='1'/> is in an <interface type='hostdev'>

Posted by Laine Stump 5 years, 7 months ago

virDomainDefCollectBootOrder() is called for every item on the list
for each type of device. Since an <interface type='hostdev'> is on
both the list of hostdev devices and the list of network devices, it
will be counted twice, and the code that checks for multiple devices
with the same boot order will give a false positive.

To remedy this, we make sure to return early for hostdev devices that
have a parent.type != NONE.

This was introduced in commit 5b75a4, which was first in libvirt-4.4.0.

Resolves: https://bugzilla.redhat.com/1601318
Signed-off-by: Laine Stump <laine@laine.org>
---
 src/conf/domain_conf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 77cc73744f..71a2fb0426 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5062,6 +5062,14 @@ virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED,
     if (info->bootIndex == 0)
         return 0;
 
+    if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
+        dev->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE) {
+        /* This hostdev is a child of a higher level device
+         * (e.g. interface), and thus already being counted on the
+         * list for the other device type.
+         */
+        return 0;
+    }
     if (virAsprintf(&order, "%u", info->bootIndex) < 0)
         goto cleanup;
 
-- 
2.14.4

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

Re: [libvirt] [PATCH] conf: fix bogus error when <boot order='1'/> is in an <interface type='hostdev'>

Posted by Ján Tomko 5 years, 7 months ago

conf: fix boot order with <interface type='hostdev'>

You don't need the whole reproducer in the commit summary

On Thu, Sep 06, 2018 at 09:14:26PM -0400, Laine Stump wrote:
>virDomainDefCollectBootOrder() is called for every item on the list
>for each type of device. Since an <interface type='hostdev'> is on
>both the list of hostdev devices and the list of network devices, it
>will be counted twice, and the code that checks for multiple devices
>with the same boot order will give a false positive.
>
>To remedy this, we make sure to return early for hostdev devices that
>have a parent.type != NONE.
>
>This was introduced in commit 5b75a4, which was first in libvirt-4.4.0.
>

Yay, me!

>Resolves: https://bugzilla.redhat.com/1601318
>Signed-off-by: Laine Stump <laine@laine.org>
>---
> src/conf/domain_conf.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 77cc73744f..71a2fb0426 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -5062,6 +5062,14 @@ virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED,
>     if (info->bootIndex == 0)
>         return 0;
>
>+    if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
>+        dev->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE) {
>+        /* This hostdev is a child of a higher level device
>+         * (e.g. interface), and thus already being counted on the
>+         * list for the other device type.
>+         */
>+        return 0;
>+    }

An extra newline here would be nice.

>     if (virAsprintf(&order, "%u", info->bootIndex) < 0)
>         goto cleanup;
>

But more importantly, a test case to catch it in the future.

With the test case added:
Reviewed-by: Ján Tomko <jtomko@redhat.com>

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

Re: [libvirt] [PATCH] conf: fix bogus error when <boot order='1'/> is in an <interface type='hostdev'>

Posted by Laine Stump 5 years, 7 months ago

On 09/07/2018 06:41 AM, Ján Tomko wrote:
> conf: fix boot order with <interface type='hostdev'>
>
> You don't need the whole reproducer in the commit summary

But what you've given isn't a correct description :-) The boot order
isn't being fixed; the false error is being eliminated.

>
> On Thu, Sep 06, 2018 at 09:14:26PM -0400, Laine Stump wrote:
>> virDomainDefCollectBootOrder() is called for every item on the list
>> for each type of device. Since an <interface type='hostdev'> is on
>> both the list of hostdev devices and the list of network devices, it
>> will be counted twice, and the code that checks for multiple devices
>> with the same boot order will give a false positive.
>>
>> To remedy this, we make sure to return early for hostdev devices that
>> have a parent.type != NONE.
>>
>> This was introduced in commit 5b75a4, which was first in libvirt-4.4.0.
>>
>
> Yay, me!

Truthfully? "Yay *me*" :-/. It was my poor decision (which, as usual,
seemed like a good idea at the time) that led to the requirement for
this exception to be scattered all over the code, and I regret it more
every day. It's just about reached the necessary level of regret to get
rid of it, but I need to make sure that doing so won't create some
*other* regression.

Your part in it was only to naively believe that the data structure you
were working with was laid out with a bit of common sense.

>
>> Resolves: https://bugzilla.redhat.com/1601318
>> Signed-off-by: Laine Stump <laine@laine.org>
>> ---
>> src/conf/domain_conf.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 77cc73744f..71a2fb0426 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -5062,6 +5062,14 @@ virDomainDefCollectBootOrder(virDomainDefPtr
>> def ATTRIBUTE_UNUSED,
>>     if (info->bootIndex == 0)
>>         return 0;
>>
>> +    if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
>> +        dev->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE) {
>> +        /* This hostdev is a child of a higher level device
>> +         * (e.g. interface), and thus already being counted on the
>> +         * list for the other device type.
>> +         */
>> +        return 0;
>> +    }
>
> An extra newline here would be nice.

I don't know. That seems pretty extreme.

>
>>     if (virAsprintf(&order, "%u", info->bootIndex) < 0)
>>         goto cleanup;
>>
>
> But more importantly, a test case to catch it in the future.

Yeah, at least an xml2xml test. I don't know if there's sufficient
mocking to permit an xml2argv test these days (there was no mocking in
the tests at all back when <interface type='hostdev'> was added), but
I'll give that a try too.

> With the test case added:
> Reviewed-by: Ján Tomko <jtomko@redhat.com>

Thanks!

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