[libvirt] [PATCH 3/8] qemu: Assign device addresses in PostParse
John Ferlan
jferlan at redhat.com
Mon Mar 14 18:42:55 UTC 2016
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.
<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.
John
(I am away from keyboard until later tonight if you read this today, but
figured I'd post something for you to consider).
>
> 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(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9044792..d697e99 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1369,7 +1369,7 @@ qemuCanonicalizeMachine(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> static int
> qemuDomainDefPostParse(virDomainDefPtr def,
> virCapsPtr caps,
> - unsigned int parseFlags ATTRIBUTE_UNUSED,
> + unsigned int parseFlags,
> void *opaque)
> {
> virQEMUDriverPtr driver = opaque;
> @@ -1408,6 +1408,17 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> if (virSecurityManagerVerify(driver->securityManager, def) < 0)
> goto cleanup;
>
Should this hunk go after qemuDomainDefAddDefaultDevices? Nothing in the
three interceding calls seems to add a device, controller, etc.
> + /* Device defaults are normally set after calling the driver specific
> + PostParse routine (this function), but we need them earlier. */
> + if (virDomainDefPostParseDevices(def, caps,
> + parseFlags, driver->xmlopt) < 0)
> + goto cleanup;
NIT: Add extra blank line here.
So this is the one I'm really struggling with mainly because we go
through the device iteration twice, where it seems the claim is that the
second iteration would be unnecessary (that's how I read the previous
discussion at least). Although if not necessary, then we wouldn't assign
addresses to whatever could have been changed by the second call.
So, I'm curious if using the recently added xmlopt->config.features bits
could/should somehow be used to avoid the second pass? That is after
this call could we set a xmlopt->config.features bit that could be
checked in the called function to not initiate the
virDomainDefPostParseDeviceIterator?
I guess the other option is to understand whether it's
qemuDomainDeviceDefPostParse that needs to be run or whether it's the
virDomainDeviceDefPostParseInternal and
virDomainDeviceDefPostParseCheckFeatures that needs to be run.
Guess I'm really thinking about some environment where there's 100's of
devices (or more) that we'll needlessly spin in while loops.
> + if (virDomainDefAddImplicitDevices(def) < 0)
> + goto cleanup;
Would calling this a second time be duplicitous? e.g.it's called in
virDomainDefPostParseInternal
> +
> + if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> virObjectUnref(qemuCaps);
> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
> index 97225f4..c0d7e94 100644
> --- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
> +++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
> @@ -29,7 +29,9 @@
> </disk>
> <controller type='usb' index='0'/>
> <controller type='pci' index='0' model='pci-root'/>
> - <controller type='scsi' index='0'/>
> + <controller type='scsi' index='0'>
> + <address type='spapr-vio' reg='0x2000'/>
> + </controller>
> <input type='keyboard' bus='usb'/>
> <input type='mouse' bus='usb'/>
> <graphics type='sdl'/>
> 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.
> if (!(vmdef = virDomainDefParseFile(xml, driver.caps, driver.xmlopt,
> (VIR_DOMAIN_DEF_PARSE_INACTIVE |
> parseFlags)))) {
> @@ -302,11 +307,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
> if (qemuProcessPrepareMonitorChr(&monitor_chr, domainLibDir) < 0)
> goto out;
>
> - virQEMUCapsSetList(extraFlags,
> - QEMU_CAPS_NO_ACPI,
> - QEMU_CAPS_DEVICE,
> - QEMU_CAPS_LAST);
> -
> if (STREQ(vmdef->os.machine, "pc") &&
> STREQ(vmdef->emulator, "/usr/bin/qemu-system-x86_64")) {
> VIR_FREE(vmdef->os.machine);
> @@ -316,12 +316,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
>
> virQEMUCapsFilterByMachineType(extraFlags, vmdef->os.machine);
>
> - if (qemuDomainAssignAddresses(vmdef, extraFlags, NULL)) {
> - if (flags & FLAG_EXPECT_ERROR)
> - goto ok;
> - goto out;
> - }
> -
> log = virtTestLogContentAndReset();
> VIR_FREE(log);
> virResetLastError();
> @@ -1420,7 +1414,7 @@ mymain(void)
> QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION);
> DO_TEST("pseries-vio-user-assigned",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
> - DO_TEST_ERROR("pseries-vio-address-clash",
> + DO_TEST_PARSE_ERROR("pseries-vio-address-clash",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
> DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM);
> DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI,
> @@ -1583,7 +1577,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
>
> - DO_TEST_ERROR("pcie-root-port-too-many",
> + DO_TEST_PARSE_ERROR("pcie-root-port-too-many",
> QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_IOH3420,
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 0735677..251effd 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -37,12 +37,11 @@ struct testInfo {
> };
>
> static int
> -qemuXML2XMLPreFormatCallback(virDomainDefPtr def, const void *opaque)
> +qemuXML2XMLPreFormatCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + const void *opaque ATTRIBUTE_UNUSED)
> {
> - const struct testInfo *info = opaque;
> -
> - if (qemuDomainAssignAddresses(def, info->qemuCaps, NULL))
> - return -1;
> + /* Unused for now. But can eventually be used to test
> + qemuAssignDeviceAliases for example */
>
> return 0;
> }
> @@ -151,9 +150,6 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
> goto cleanup;
> }
>
> - if (qemuDomainAssignAddresses(obj->def, data->qemuCaps, NULL))
> - goto cleanup;
> -
> /* format it back */
> if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL,
> VIR_DOMAIN_DEF_FORMAT_SECURE))) {
>
More information about the libvir-list
mailing list