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

[Cole Robinson]

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