On 03/14/2016 02:42 PM, John Ferlan wrote:

On 03/08/2016 11:36 AM, Cole Robinson wrote:

...

It's just a combination of AddImplicitControllers, and AddConsoleCompat. Every caller that wants ImplicitControllers also wants the ConsoleCompat AFAICT, so lump them together. We also need it for future patches. --- src/conf/domain_conf.c | 19 ++++++++++++++----- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/vmx/vmx.c | 2 +- src/vz/vz_sdk.c | 2 +- 6 files changed, 21 insertions(+), 12 deletions(-)

Not an issue, but a note below...

ACK -

Thanks, pushed now

John

...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 39cedbd..40b1929 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3842,9 +3842,6 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefPostParseMemory(def, parseFlags) < 0) return -1;

- if (virDomainDefAddConsoleCompat(def) < 0) - return -1; - if (virDomainDefRejectDuplicateControllers(def) < 0) return -1;

@@ -3854,7 +3851,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefPostParseTimer(def) < 0) return -1;

- if (virDomainDefAddImplicitControllers(def) < 0) + if (virDomainDefAddImplicitDevices(def) < 0) return -1;

/* clean up possibly duplicated metadata entries */ @@ -18289,7 +18286,7 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) * in the XML. This is for compat with existing apps which will * not know/care about <controller> info in the XML */ -int +static int virDomainDefAddImplicitControllers(virDomainDefPtr def) { if (virDomainDefAddDiskControllersForType(def, @@ -18324,6 +18321,18 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) return 0; }

+int +virDomainDefAddImplicitDevices(virDomainDefPtr def) +{ + if (virDomainDefAddConsoleCompat(def) < 0) + return -1; + + if (virDomainDefAddImplicitControllers(def) < 0) + return -1; + + return 0; +} + virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 06305f0..6f044d2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2697,7 +2697,7 @@ virDomainObjPtr virDomainObjParseFile(const char *filename, bool virDomainDefCheckABIStability(virDomainDefPtr src, virDomainDefPtr dst);

-int virDomainDefAddImplicitControllers(virDomainDefPtr def); +int virDomainDefAddImplicitDevices(virDomainDefPtr def);

virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def, unsigned int iothread_id); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3a1b9e1..5399117 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -200,7 +200,7 @@ virDomainControllerRemove; virDomainControllerTypeToString; virDomainCpuPlacementModeTypeFromString; virDomainCpuPlacementModeTypeToString; -virDomainDefAddImplicitControllers; +virDomainDefAddImplicitDevices; virDomainDefAddUSBController; virDomainDefCheckABIStability; virDomainDefCheckDuplicateDiskInfo; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 102fade..9c60518 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7924,7 +7924,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ dev->data.disk = NULL; if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) - if (virDomainDefAddImplicitControllers(vmdef) < 0) + if (virDomainDefAddImplicitDevices(vmdef) < 0) return -1; if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; @@ -7949,7 +7949,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainHostdevInsert(vmdef, hostdev)) return -1; dev->data.hostdev = NULL; - if (virDomainDefAddImplicitControllers(vmdef) < 0) + if (virDomainDefAddImplicitDevices(vmdef) < 0) return -1; if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; @@ -7991,7 +7991,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (qemuDomainChrInsert(vmdef, dev->data.chr) < 0) return -1; dev->data.chr = NULL; - if (virDomainDefAddImplicitControllers(vmdef) < 0) + if (virDomainDefAddImplicitDevices(vmdef) < 0) return -1; if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 4fd0a1d..893c77a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1691,7 +1691,7 @@ virVMXParseConfig(virVMXContext *ctx, }

/* def:controllers */ - if (virDomainDefAddImplicitControllers(def) < 0) { + if (virDomainDefAddImplicitDevices(def) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add controllers"));

Do we care about the error message here?

Existing, but perhaps notable... Interesting order in vmx code to add implicit controller/devices before adding defined controller devices. Although I suppose that SCSI controller range check would need adjustment too.

I just left the error as is. I think the ideal thing would be to get most callers to use virDomainDefAddImplicitDevices via virDomainDefPostParse rather than calling it manually. There's a few other usages spread around too. - Cole

$SUBJ: "domain: Introduce virDomainDefPostParseDevices" On 03/08/2016 11:36 AM, Cole Robinson wrote:

We will use this in upcoming patches

Export the virDomainDefPostParseDevices rather than having it only be callable during virDomainDefPostParse. Future patches will be able to make use of the optimization.

--- src/conf/domain_conf.c | 33 +++++++++++++++++++++++---------- src/conf/domain_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40b1929..bc4e369 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4191,12 +4191,11 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, data->parseFlags, data->xmlopt); }

- int -virDomainDefPostParse(virDomainDefPtr def, - virCapsPtr caps, - unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt) +virDomainDefPostParseDevices(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt) { int ret; struct virDomainDefPostParseDeviceIteratorData data = { @@ -4206,6 +4205,23 @@ virDomainDefPostParse(virDomainDefPtr def, .parseFlags = parseFlags, };

+ if ((ret = virDomainDeviceInfoIterateInternal(def, + virDomainDefPostParseDeviceIterator, + true, + &data)) < 0) + return ret; + + return 0;

I think you could just "return virDomainDeviceInfoIterateInternal(...);" It only returns 0 or -1 anyway. I'm not sure this is necessary - it works, but do we really want to open up just device post processing? Beyond your usage in patch 3, is there another need for it? My concern being missing some subsequent domain check such as virDomainDefPostParseInternal (or worse needing to add code somewhere that eventually adds a call here). The rest of my thoughts are in patch 3... John

+} + +int +virDomainDefPostParse(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt) +{ + int ret; + /* call the domain config callback */ if (xmlopt->config.domainPostParseCallback) { ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags, @@ -4215,13 +4231,10 @@ virDomainDefPostParse(virDomainDefPtr def, }

/* iterate the devices */ - if ((ret = virDomainDeviceInfoIterateInternal(def, - virDomainDefPostParseDeviceIterator, - true, - &data)) < 0) + if ((ret = virDomainDefPostParseDevices(def, caps, + parseFlags, xmlopt)) < 0) return ret;

- if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0) return ret;

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6f044d2..aba53a2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2476,6 +2476,11 @@ virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, virDomainXMLOptionPtr xmlopt); +int +virDomainDefPostParseDevices(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt);

static inline bool virDomainObjIsActive(virDomainObjPtr dom) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5399117..51d2721 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -235,6 +235,7 @@ virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; +virDomainDefPostParseDevices; virDomainDefSetMemoryInitial; virDomainDefSetMemoryTotal; virDomainDefSetVcpus;

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))) {

On 03/21/2016 02:44 PM, John Ferlan wrote:

On 03/08/2016 11:36 AM, Cole Robinson wrote:

...

If a bare device <address type='pci'/> is specified, set an internal flag address->auto_allocate. Individual hv drivers can then check for this and act on it if they want, nothing is allocated in generic code.

If drivers allocate an address, they are expected to unset auto_allocate. Generic domain conf code then checks at XML format time to ensure no device addresses still have auto_allocate set; this ensures we aren't formatting any bogus address XML, and informing the user if their request didn't work. Add a genericxml2xml test case for this.

The auto_allocate property is a part of the generic address structure and not the PCI specific bits, this will make it easier to reuse with other address types too.

One note: we detect <address type='pci'/> by counting it's XML properties, rather than comparing specifically against parsed values, which seems easier to maintain. --- docs/schemas/domaincommon.rng | 5 +++- src/conf/domain_conf.c | 29 +++++++++++++++++++++- src/conf/domain_conf.h | 1 + .../generic-pci-autofill-addr.xml | 27 ++++++++++++++++++++ tests/genericxml2xmltest.c | 17 +++++++++---- 5 files changed, 72 insertions(+), 7 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-pci-autofill-addr.xml

Will also need to update 'formatdomain.html.in' to describe the new allowance of just "<address type='pci'>...

Darn I must have remembered and then forgot to update the docs like 10 times :/

Would this be useful if "multifunction='on'" without specifying an address? e.g.:

<address type='pci' multifunction='on'>

Maybe? Honestly I don't know much about when multifunction should be used.

Could other address types find the functionality useful? That is, I want address type 'drive' or 'usb', but I have no idea how to fill in the rest, I want the hv to do it for me.

My intent with how this was implemented is that it shouldn't take much change in generic domain_conf.c code to handle this for other address types: position of auto_allocate in the address structure, and the generic device iterator validating that all addresses were allocated.

...

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6ca937c..d083250 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4471,7 +4471,10 @@ <attribute name="type"> <value>pci</value> </attribute> - <ref name="pciaddress"/> + <choice> + <ref name="pciaddress"/> + <empty/> + </choice> </group> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bc4e369..bbc42a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3827,6 +3827,23 @@ virDomainDefPostParseTimer(virDomainDefPtr def) }

+ static int ^ Extraneous space

...

+virDomainCheckUnallocatedDeviceAddrs(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *data ATTRIBUTE_UNUSED) +{ + if (!info->auto_allocate) + return 0; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("driver didn't allocate requested address type '%s' for device '%s'"), + virDomainDeviceAddressTypeToString(info->type), + virDomainDeviceTypeToString(dev->type)); + return -1; +} + + static int virDomainDefPostParseInternal(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -3851,6 +3868,11 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefPostParseTimer(def) < 0) return -1;

+ /* ensure the driver filled in any auto_allocate addrs */ + if (virDomainDeviceInfoIterate(def, virDomainCheckUnallocatedDeviceAddrs, + NULL) < 0) + return -1; +

Why couldn't this go in virDomainDefPostParseDeviceIterator?

That is, rather than have yet another iterator through the devices, call virDomainCheckUnallocatedDeviceAddrs right after or at the end of virDomainDeviceDefPostParse...

As the patches stand, it won't work with due to the ordering: this validation check needs to come after qemuDomainAssignAddresses. When the patches are reworked it may be able to be combined with DeviceDefPostParse

...

if (virDomainDefAddImplicitDevices(def) < 0) return -1;

@@ -4872,8 +4894,13 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,

switch ((virDomainDeviceAddressType) info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) + if (virXMLPropertyCount(address) == 1) {

Wish there was some other way than counting attributes, but I guess since "type" is an attribute we can hopefully guarantee that only "type=<something>" has been supplied.

...

+ /* Bare <address type='pci'/> is a request to allocate + the address. */ + info->auto_allocate = true; + } else if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) { goto cleanup; + } break;

case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index aba53a2..dd9d0b1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -346,6 +346,7 @@ struct _virDomainDeviceInfo { */ char *alias; int type; /* virDomainDeviceAddressType */ + bool auto_allocate;

The name is generic enough to make me think it could work for other address types, but only PCI is supported.

Yup, that's what I was going for. - Cole