[3/8] qemu: Assign device addresses in PostParse

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.

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 lone test suite change is due to DomainNativeToXML now calling
qemuDomainAssignAddresses... apparently that's the only test which
hits qemu specific address logic.

There's still quite a few manual callers of qemuDomainAssignAddresses
that could be dropped too but it would need additional testing, and
they shouldn't disrupt anything in the interim since extra calls
will be no-ops.
---
 src/qemu/qemu_domain.c                               | 13 ++++++++++++-
 tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml |  4 +++-
 tests/qemuxml2argvtest.c                             | 20 +++++++-------------
 tests/qemuxml2xmltest.c                              | 12 ++++--------
 4 files changed, 26 insertions(+), 23 deletions(-)

-- 
2.5.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list