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

On 05/19/2016 01:15 PM, Cole Robinson wrote: > 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 Right. I forgot that. I'll grab your doc bits and squash them in. > > 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 Actually I was thinking the opposite :-) (not that it makes too much difference - how many different device types will we ever be able to actually choose between?) > - We don't raise an explicit error for drivers that don't support this type of > address allocation, like libxl. Is that specific to my method of supporting <address type='pci'/> ? Since they don't use any of the common address assignment functions, I hadn't even looked at what they do.

> 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 Yes and no. I did check all uses of it. It really only needs extra qualification in code that is part of the parser or postparse callbacks (of course, in order to only check those parts, you need to know where/what they are!). Once address assignment is done, anything with ADDRESS_TYPE_PCI is guaranteed to have a valid PCI address.

> > upsides: > - less magic > - I think it will make allocating address at hotplug time simpler as well I hadn't thought about that yet - more of the "90% of the coding effort going to 0.005% of the users" :-P

Thanks for putting up with my opinionated opinions :-) (and for reviewing, and for the test cases, and ....) I'll try to push it a bit later today.