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

[Cole Robinson]

On 05/19/2016 08:39 PM, Laine Stump wrote:
> On 05/16/2016 06:26 PM, Cole Robinson wrote:
>> 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
> 
> Exactly - the driver-specific post-parse is called before the common
> post-parse, and the controller indexes need to be known by the time those
> controllers are added, because they are added at *specific indexes* depending
> on whether or not there is already a controller of that exact type/index. The
> auto-indexing is the same for all drivers though, so if it goes in a
> post-parse callback it should go in the common postparse, which happens too late.
> 
> (note that we don't need to worry about auto-indexing controllers added by
> post-parse functions - they are already using the same method to give each
> controller an index immediately).
> 

Okay, sorry for the confusion. Still though, putting it inline with the XML
parsing is not a good idea.

> 
>>> 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.
> 
> The only controllers that need this auto-index step are those added to the
> config by the user. None of the controllers added later need it (because they
> all determine their own index).
> 

Ah okay, that makes it simpler then IMO. Stick the index assignment code
before the hvspecific/qemuDomainDefPostParse call in virDomainDefPostParse.
Yeah the ordering will look weird, but we can cram stuff in PostParse, test
that it works, and formalize things or come up with a better ordering later,
once we have something testable and working. Mixing things into the XML parser
is when stuff becomes really hard to decouple

> 
>>> 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.
> 
> 
> Yeah, that was just generalizing on the point that even the current set of
> callbacks is insufficient to solve this, and if we add more callbacks, we'll
> eventually end up with another problem that all *those* callbacks can't solve.
> 
> If you put too much stuff into callback functions, at some point you end up
> with code that is associated with the same functionality but split up into
> different unrelated parts of the code, that it becomes impossible for a
> newcomer to navigate.
> 

Yes the web of postparse checks is confusing at the moment. But a big part of
the confusion is because we have things like validation, setting defaults,
assigning address, add adding default devices all sprinkled informally across
multiple different places like generic postparse, hv postparse, XML building,
cli building, opencoded, because we never had a clear plan. Peter's patches
for formalizing a validation callback is a step in the right direction but it
will get worse before it gets better, like with any major code transition.

But what I absolutely think is part of the solution is making the XML parser
be strictly about serializing the XML into a virDomainDef, with as little side
effects as possible. There's already a multitude of little issues stemming
from validation checks or default setting in the XML parser that manual
virDomainDef builders can't leverage. Maybe no other drivers need the index
assignment stuff, dunno, but having it separate from the XML parser is
basically the only way they will be able to consume this. And putting it in
generic PostParse means those drivers get it automatically.

- Cole