On 05/19/2016 03:27 PM, Laine Stump wrote: > 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. > My patches had an explicit PostParse check in generic code that would throw an error if <address type='pci'/> was never correctly filled in by the hypervisor, so <address type='pci/> would explicitly fail for all non-qemu drivers. It's a minor point > >> 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. > Agreed, my point was just that we need to be cognizant of the distinction whenever new code is added >> >> 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 > Yeah it's certainly not a blocker for this series, but hotplug will be relevant for aarch64 eventually > 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. No worries, thanks for implementing it :) - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list