[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]