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

[Cole Robinson]

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