[Date Prev][Date Next] [Thread Prev][Thread Next]
Re: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
- From: Cole Robinson <crobinso redhat com>
- To: Laine Stump <laine laine org>, libvir-list redhat com
- Subject: Re: [libvirt] [PATCH 4/4] conf: permit auto-assignment of controller indexes
- Date: Mon, 16 May 2016 18:26:13 -0400
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
Yeah I forgot about all those :/ addPCIe, etc. Note, those bits are already
called as part of virDomainDefPostParse, via the driver specific
> 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
2) per device qemuDomainDeviceDefPostParse
3) per device virDomainDeviceDefPostParseInternal
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
[Date Prev][Date Next] [Thread Prev][Thread Next]