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

[Laine Stump]

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 at 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 at redhat.com>

Thanks!