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