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(a)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(a)redhat.com>
Thanks!