[libvirt] [PATCH] conf: fix bogus error when <boot order='1'/> is in an <interface type='hostdev'>
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
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
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!
participants (2)
-
Ján Tomko -
Laine Stump