On 03/08/2016 11:36 AM, Cole Robinson wrote: > In order to make this work, we need to short circuit the normal > virDomainDefPostParse ordering, and manually add stock devices > ourselves, since we need them in the XML before assigning addresses. > > The motivation for this is that upcoming patches will want to perform > generic PostParse checks that need to run _after_ the driver has > assigned all its addresses. > Do you mean you need to perform the generic DevicePostParse checks after the driver has assigned all its addresses or the generic (domain) PostParse checks. Based on reading patch 6 of this series, it seems the latter, but the ImplicitDevices check is involved. <snip> > Peter objected to this here: > https://www.redhat.com/archives/libvir-list/2016-January/msg00200.html > > Suggesting adding an extra PostParse callback instead. I argued > that change isn't necessarily sufficient either, and that this > method should be safe since all these functions already need to be > idemptotent. The preceding hunk seems to have been more relevant for something after the '---' so as to not be included in git history. </snip> Even without the upcoming patches - it seems to me this patch is designed to ensure that once the qemuDomainDefPostParse code adds DefaultDevices that we make sure that those devices and the existing ones for the domain go through the device post parse processing before adding implicit devices and assigning addresses for any devices without one. The key of course being the assign addresses which needs to be called after each device has been addressed. So the problems are: 1. We don't add the ImplicitDevices early enough 2. We don't assign the DeviceAddress early enough where "early enough" is defined as before virDomainDefPostParseInternal during virDomainDefPostParse. The chicken/egg problem being that the PostParseInternal function calls virDomainDefAddImplicitControllers. Another "option" it seems would be to add a 3rd callback mechanism to assign addresses for all domains (if supported/necessary). This would be called in virDomainDefPostParse before the *DefPostParseInternal. Going this way I don't think you need current patch 2... So starting with the implicit devices - it doesn't seem there is anything in the *PostParseInternal that's adding a device, so instead of the current patch 2, can we move that call to virDomainDefPostParse? Then patch 3 could add a call to a device address assignment callback, such as the following: virDomainDefPostParse .domainDefPostCallback (qemuDomainDefPostParse) ... qemuDomainAddDefaultDevices ... virDomainDeviceInfoIterateInternal for each device .devicesPostParseCallback virDomainDefAddImplicitControllers .deviceAssignAddrsPostParseCallback (qemuDomainAssignAddresses) virDomainDefPostParseInternal As compared to how I read this patch: virDomainDefPostParse .domainDefPostCallback (qemuDomainDefPostParse) ... qemuDomainAddDefaultDevices virDomainDefPostParseDevices for each device .devicesPostParseCallback virDomainDefAddImplicitDevices qemuDomainAssignAddresses ... virDomainDefPostParseDevices NOTE: Duplicated for qemu... for each device and shouldn't do anything .devicesPostParseCallback virDomainDefPostParseInternal FWIW: The rest of this was written first, then I started trying to figure out what problem was being solved... I'll leave the comments as is since they point out my thinking while reviewing. John (I am away from keyboard until later tonight if you read this today, but figured I'd post something for you to consider). > > The lone test suite change is due to DomainNativeToXML now calling > qemuDomainAssignAddresses... apparently that's the only test which > hits qemu specific address logic. > > There's still quite a few manual callers of qemuDomainAssignAddresses > that could be dropped too but it would need additional testing, and > they shouldn't disrupt anything in the interim since extra calls > will be no-ops. > --- > src/qemu/qemu_domain.c | 13 ++++++++++++- > tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml | 4 +++- > tests/qemuxml2argvtest.c | 20 +++++++------------- > tests/qemuxml2xmltest.c | 12 ++++-------- > 4 files changed, 26 insertions(+), 23 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 9044792..d697e99 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -1369,7 +1369,7 @@ qemuCanonicalizeMachine(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) > static int > qemuDomainDefPostParse(virDomainDefPtr def, > virCapsPtr caps, > - unsigned int parseFlags ATTRIBUTE_UNUSED, > + unsigned int parseFlags, > void *opaque) > { > virQEMUDriverPtr driver = opaque; > @@ -1408,6 +1408,17 @@ qemuDomainDefPostParse(virDomainDefPtr def, > if (virSecurityManagerVerify(driver->securityManager, def) < 0) > goto cleanup; > Should this hunk go after qemuDomainDefAddDefaultDevices? Nothing in the three interceding calls seems to add a device, controller, etc. > + /* Device defaults are normally set after calling the driver specific > + PostParse routine (this function), but we need them earlier. */ > + if (virDomainDefPostParseDevices(def, caps, > + parseFlags, driver->xmlopt) < 0) > + goto cleanup; NIT: Add extra blank line here. So this is the one I'm really struggling with mainly because we go through the device iteration twice, where it seems the claim is that the second iteration would be unnecessary (that's how I read the previous discussion at least). Although if not necessary, then we wouldn't assign addresses to whatever could have been changed by the second call. So, I'm curious if using the recently added xmlopt->config.features bits could/should somehow be used to avoid the second pass? That is after this call could we set a xmlopt->config.features bit that could be checked in the called function to not initiate the virDomainDefPostParseDeviceIterator? I guess the other option is to understand whether it's qemuDomainDeviceDefPostParse that needs to be run or whether it's the virDomainDeviceDefPostParseInternal and virDomainDeviceDefPostParseCheckFeatures that needs to be run. Guess I'm really thinking about some environment where there's 100's of devices (or more) that we'll needlessly spin in while loops. > + if (virDomainDefAddImplicitDevices(def) < 0) > + goto cleanup; Would calling this a second time be duplicitous? e.g.it's called in virDomainDefPostParseInternal > + > + if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) > + goto cleanup; > + > ret = 0; > cleanup: > virObjectUnref(qemuCaps); > diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml > index 97225f4..c0d7e94 100644 > --- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml > +++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml > @@ -29,7 +29,9 @@ > </disk> > <controller type='usb' index='0'/> > <controller type='pci' index='0' model='pci-root'/> > - <controller type='scsi' index='0'/> > + <controller type='scsi' index='0'> > + <address type='spapr-vio' reg='0x2000'/> > + </controller> > <input type='keyboard' bus='usb'/> > <input type='mouse' bus='usb'/> > <graphics type='sdl'/> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index d29073d..aaec9de 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -277,6 +277,11 @@ static int testCompareXMLToArgvFiles(const char *xml, > if (virBitmapParse("0-3", '\0', &nodeset, 4) < 0) > goto out; > > + virQEMUCapsSetList(extraFlags, > + QEMU_CAPS_NO_ACPI, > + QEMU_CAPS_DEVICE, > + QEMU_CAPS_LAST); > + Why is this moved unless perhaps the goal was to use the flags in the following call... The 'extraFlags' is only used later anyway in the virQEMUCapsFilterByMachineType. Since qemuDomainAssignAddresses was removed and the series involves erroring during parse, I started to wonder... especially since the removed call used the extraFlags. > if (!(vmdef = virDomainDefParseFile(xml, driver.caps, driver.xmlopt, > (VIR_DOMAIN_DEF_PARSE_INACTIVE | > parseFlags)))) { > @@ -302,11 +307,6 @@ static int testCompareXMLToArgvFiles(const char *xml, > if (qemuProcessPrepareMonitorChr(&monitor_chr, domainLibDir) < 0) > goto out; > > - virQEMUCapsSetList(extraFlags, > - QEMU_CAPS_NO_ACPI, > - QEMU_CAPS_DEVICE, > - QEMU_CAPS_LAST); > - > if (STREQ(vmdef->os.machine, "pc") && > STREQ(vmdef->emulator, "/usr/bin/qemu-system-x86_64")) { > VIR_FREE(vmdef->os.machine); > @@ -316,12 +316,6 @@ static int testCompareXMLToArgvFiles(const char *xml, > > virQEMUCapsFilterByMachineType(extraFlags, vmdef->os.machine); > > - if (qemuDomainAssignAddresses(vmdef, extraFlags, NULL)) { > - if (flags & FLAG_EXPECT_ERROR) > - goto ok; > - goto out; > - } > - > log = virtTestLogContentAndReset(); > VIR_FREE(log); > virResetLastError(); > @@ -1420,7 +1414,7 @@ mymain(void) > QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION); > DO_TEST("pseries-vio-user-assigned", > QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); > - DO_TEST_ERROR("pseries-vio-address-clash", > + DO_TEST_PARSE_ERROR("pseries-vio-address-clash", > QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); > DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM); > DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI, > @@ -1583,7 +1577,7 @@ mymain(void) > QEMU_CAPS_DEVICE_VIDEO_PRIMARY, > QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); > > - DO_TEST_ERROR("pcie-root-port-too-many", > + DO_TEST_PARSE_ERROR("pcie-root-port-too-many", > QEMU_CAPS_DEVICE_PCI_BRIDGE, > QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, > QEMU_CAPS_DEVICE_IOH3420, > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c > index 0735677..251effd 100644 > --- a/tests/qemuxml2xmltest.c > +++ b/tests/qemuxml2xmltest.c > @@ -37,12 +37,11 @@ struct testInfo { > }; > > static int > -qemuXML2XMLPreFormatCallback(virDomainDefPtr def, const void *opaque) > +qemuXML2XMLPreFormatCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, > + const void *opaque ATTRIBUTE_UNUSED) > { > - const struct testInfo *info = opaque; > - > - if (qemuDomainAssignAddresses(def, info->qemuCaps, NULL)) > - return -1; > + /* Unused for now. But can eventually be used to test > + qemuAssignDeviceAliases for example */ > > return 0; > } > @@ -151,9 +150,6 @@ testCompareStatusXMLToXMLFiles(const void *opaque) > goto cleanup; > } > > - if (qemuDomainAssignAddresses(obj->def, data->qemuCaps, NULL)) > - goto cleanup; > - > /* format it back */ > if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL, > VIR_DOMAIN_DEF_FORMAT_SECURE))) { > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list