Re: [libvirt] [PATCH 0/6] auto-assign addresses when <address type='pci'/> is specified

On 05/18/2016 03:23 PM, Laine Stump wrote: > This is an alternative to Cole's series that permits <address > type='pci'/> to force assignment of a PCI address, which is > particularly useful on platforms that could connect the same device in > different ways (e.g. aarch64/virt). > > Here is Cole's last iteration of the series: > > https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html > > I had expressed a dislike of the "auto_allocate" flag that his series > temporarily adds to the address (while simultaneously changing the > address type to NONE) and suggested just changing all the necessary > places to check for a valid PCI address instead of just checking the > address type. He replied that this wasn't as simple as it looked, so I > decided to try it; turns out he's right. But I still like this method > better because it's not playing tricks with the address type, or > adding a temporary private attribute to what should be pure config > data. > > Your opinion may vary though :-) >

> Note that patch 5/6 incorporates the same test case that Cole used in > his penultimate patch, and I've added his patch to check the case of > aarch64 at the end as well. > ACK series, but it's missing formatdomain.html.in changes. You can grab the check from my patch #2 I'm fine with this approach but I'll just list the downsides - Less generalizable, for adding additional address types in the future, but that's hypothetical at this point - We don't raise an explicit error for drivers that don't support this type of address allocation, like libxl. If it matters we could add a domain XML parse feature to throw an explicit error though - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and needs to be considered carefully in other parts of the code upsides: - less magic - I think it will make allocating address at hotplug time simpler as well