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).
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).
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.
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
I wasn't asking how you could get the qemuCaps into a qemu-specific
callback function. I had thought that you were calling a common
callback for all hypervisors. I wasn't thinking clearly though - of
course it would be different for each hypervisor.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list