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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list