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