[libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes

[Cole Robinson]

On 05/15/2016 02:57 PM, Laine Stump wrote:
> On 05/15/2016 02:38 PM, Cole Robinson wrote:
>> On 05/15/2016 02:30 PM, Laine Stump wrote:
>>> On 05/14/2016 10:56 AM, Cole Robinson wrote:
>>>> I agree with the goal of the patch, but I think all the index assignment code
>>>> should be moved to somewhere in the PostParse call path. The fact that the
>>>> controller ParseXML now needs to act on the entire domain def is a giveaway
>>>> that it's in the wrong place.
>>> I originally did it that way, but there was some problem with it, either
>>> actual or imagined/potential. I *think* possibly the problem was that
>>> auto-added controllers (which is done in the driver-specific postparse, called
>>> prior to the common postparse) are added when an existing controller of the
>>> desired index can't be found, but if an index was added in postparse, I would
>>> want it to be done in the common postparse (since it is a requirement for
>>> *all* drivers);
>> Hmm. Most implicit controllers are added in common code actually, however
>> there are some added in qemu code it seems, for PCI, which is probably what
>> you were referring to.
> 
> There are several controllers/devices added which are not just hypervisor
> specific, but specific to particular machinetypes/arches within that
> hypervisor. In particular, qemuDomainDefAddDefaultDevices.
> 
>>   In that case we may need to do two passes of index
>> assignment, or when adding a PCI controller outside common code we just
>> informally require the hv driver to assign the index themselves.
> 
> It's not that the new controller needs an index assigned. It's that the
> controller is "maybe added" depending on whether of not there is already a
> controller of the requested type at a particular index (usually 0). For
> example, when we add a default USB controller in
> qemuDomainDefAddDefaultDevices().
> 

Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
called as part of virDomainDefPostParse, via the driver specific
qemuDomainDefPostParse

> As for doing two passes, where would the first pass be run? I don't want it to
> be done in the driver-specific postparse because then it would need to be
> called separately for every driver, which is prone to people not doing it. And
> the common postparse is too late to do any good. The only place we're left
> with, other than in the controller parser itself, is  the top-level domain
> parser function prior to calling the driver-specific postparse.
> 

So the current flow is

virDomainDefPostParse
  1) qemuDomainDefPostParse
  2) per device qemuDomainDeviceDefPostParse
  3) per device virDomainDeviceDefPostParseInternal
  4) virDomainDefAddImplicitDevices
<later>
5) qemuDomainAssignAddresses

For qemu, it looks like controllers can be added in step 1, 4, and 5. At the
very least I suspect we need a pass after step #4 since ImplicitControllers
can be added. That may cover everything that your original approach covers,
since all those places aren't hitting the XML parser anyways and instead
building controllers manually AFAICT.

> Also there is the case where only a single controller is parsed - the toplevel
> domain parse is never called there in the first place (of course I'm skeptical
> that is ever called for any controller - are any controllers hotpluggable?)
> 
> 
>> Unfortunately these types of ordering problems are basically unavoidable in
>> certain cases... there's no one size fits all ordering for validation, setting
>> defaults, adding default devices, and assigning addresses.
> 
> Yeah, there's no way to deal with this in the current postparse callback
> framework, and the only way to solve all possible ordering problems in that
> way would be, it seems, to call all of the callbacks repeatedly in a loop
> until it went through an entire cycle without any change :-P
> 

Right. I don't think we need to come up with a 100% future proof solution at
this point, just one that works for all cases we presently care about.

>>
>> > also is the potential problem that PCI controller indexes must
>>> be in place in order for the PCI address auto-assignment to work, and you had
>>> previously expressed a desire to move that up into one of the postparse
>>> functions rather than having it be a separate function that must be
>>> independently called - if that happened, it would also be done in the
>>> driver-specific postparse.
>> I posted patches yesterday adding address assignment to PostParse, but it
>> comes at the very end of the common PostParse usage, to match how
>> qemuDomainAssignAddresses is presently used.
> 
> Interesting, I need to look at that. Considering that the way addresses are
> assigned could change based on the machinetype/arch, how can you do that in
> the common postparse (which doesn't know about things like qemu capabilities)?
> 

The driver specific PostParse callbacks can take an opaque value, which is
where we get qemuCaps for example. See
src/qemu/qemu_domain.c:qemuDomainDefPostParse

- Cole