4:03 p.m.
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