On 03/14/2016 02:42 PM, John Ferlan wrote:
On 03/08/2016 11:36 AM, Cole Robinson wrote:
In order to make this work, we need to short circuit the normal virDomainDefPostParse ordering, and manually add stock devices ourselves, since we need them in the XML before assigning addresses.
The motivation for this is that upcoming patches will want to perform generic PostParse checks that need to run _after_ the driver has assigned all its addresses.
Do you mean you need to perform the generic DevicePostParse checks after the driver has assigned all its addresses or the generic (domain) PostParse checks. Based on reading patch 6 of this series, it seems the latter, but the ImplicitDevices check is involved.
After patch #6 I want the control flow to be virDomainDefPostParse-> qemuDomainPostParse AddImplicitDevices qemuDomainAssignAddresses virDomainCheckUnallocatedDeviceAddrs AddImplicitDevices needs to come before qemuDomainAssignAddresses, so the implicit controllers get addresses allocated by the qemu driver virDomainCheckUnallocatedDeviceAddrs needs to come after qemuDomainAssignAddresses. I want virDomainCheckUnallocatedDeviceAddrs in generic code, so we don't need to cram it in every HVs PostParse callback.
<snip>
Peter objected to this here: https://www.redhat.com/archives/libvir-list/2016-January/msg00200.html
Suggesting adding an extra PostParse callback instead. I argued that change isn't necessarily sufficient either, and that this method should be safe since all these functions already need to be idemptotent.
The preceding hunk seems to have been more relevant for something after the '---' so as to not be included in git history.
</snip>
Even without the upcoming patches - it seems to me this patch is designed to ensure that once the qemuDomainDefPostParse code adds DefaultDevices that we make sure that those devices and the existing ones for the domain go through the device post parse processing before adding implicit devices and assigning addresses for any devices without one.
The key of course being the assign addresses which needs to be called after each device has been addressed.
So the problems are:
1. We don't add the ImplicitDevices early enough 2. We don't assign the DeviceAddress early enough
where "early enough" is defined as before virDomainDefPostParseInternal during virDomainDefPostParse. The chicken/egg problem being that the PostParseInternal function calls virDomainDefAddImplicitControllers.
Another "option" it seems would be to add a 3rd callback mechanism to assign addresses for all domains (if supported/necessary). This would be called in virDomainDefPostParse before the *DefPostParseInternal. Going this way I don't think you need current patch 2...
So starting with the implicit devices - it doesn't seem there is anything in the *PostParseInternal that's adding a device, so instead of the current patch 2, can we move that call to virDomainDefPostParse?
Then patch 3 could add a call to a device address assignment callback, such as the following:
virDomainDefPostParse .domainDefPostCallback (qemuDomainDefPostParse) ... qemuDomainAddDefaultDevices ...
virDomainDeviceInfoIterateInternal for each device .devicesPostParseCallback
virDomainDefAddImplicitControllers
.deviceAssignAddrsPostParseCallback (qemuDomainAssignAddresses)
virDomainDefPostParseInternal
As compared to how I read this patch:
virDomainDefPostParse .domainDefPostCallback (qemuDomainDefPostParse) ... qemuDomainAddDefaultDevices virDomainDefPostParseDevices for each device .devicesPostParseCallback virDomainDefAddImplicitDevices qemuDomainAssignAddresses ...
virDomainDefPostParseDevices NOTE: Duplicated for qemu... for each device and shouldn't do anything .devicesPostParseCallback
virDomainDefPostParseInternal
FWIW: The rest of this was written first, then I started trying to figure out what problem was being solved... I'll leave the comments as is since they point out my thinking while reviewing.
Thank you for your comments. I think the idea of adding a new post parse callback specifically for assigning addresses is a good one; giving it an explicit purpose makes it more clear when hv drivers should actually implement it. And it's probably the lowest effort way to implement all this :) I'm not going to respond in detail to every point, since if your above suggestion will eliminate some of the code you responded to.
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d29073d..aaec9de 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -277,6 +277,11 @@ static int testCompareXMLToArgvFiles(const char *xml, if (virBitmapParse("0-3", '\0', &nodeset, 4) < 0) goto out;
+ virQEMUCapsSetList(extraFlags, + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_DEVICE, + QEMU_CAPS_LAST); +
Why is this moved unless perhaps the goal was to use the flags in the following call... The 'extraFlags' is only used later anyway in the virQEMUCapsFilterByMachineType. Since qemuDomainAssignAddresses was removed and the series involves erroring during parse, I started to wonder... especially since the removed call used the extraFlags.
The code now does virDomainDefParseFile->...->qemuDomainAssignAddresses and the latter bit depends heavily on QEMU_CAPS flags. extraFlags in this context is already indirectly wedged into the fake qemu driver state, so its implicitly sent to qemuDomainAssignAddresses - Cole