[libvirt] [PATCH] qemu: match controller index for LIVE+CONFIG when doing hotplug

[Ján Tomko]

On Wed, Jun 22, 2016 at 03:00:47PM -0400, Laine Stump wrote:
>An attempt to attach a new scsi controller with both --live and
>--config but without specifying an index, e.g.:
>
>  <controller model="virtio-scsi" type="scsi" />
>
>led to this error:
>
>  internal error: Cannot parse controller index -1
>
>This was because unspecified indexes are auto-assigned during
>virDomainDefPostParse(), which doesn't happen for hotplugged devices
>until after the device has been added to the domainDef, but
>qemuDomainAttachFlags() makes a copy of the device (for feeding to
>qemuDomainAttachDeviceLive() *before* it's added to the config, and
>the copying function actually formats the device object and then
>re-parses it into a new object.
>
>Since qemuDomainAttachDeviceConfig() consumes the device object
>pointer (i.e. sets it to NULL in the original virDomainDeviceDef) we
>can't just wait to make the copy. Instead, we need to make a *shallow*
>copy of the virDomainDeviceDef prior to
>qemuDomainAttachDeviceConfig(), then make a deep copy of the shallow
>copy.
>
>This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344899
>---
> src/qemu/qemu_driver.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index fa05046..f3e17e2 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -8199,6 +8199,7 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
>     virDomainObjPtr vm = NULL;
>     virDomainDefPtr vmdef = NULL;
>     virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>+    virDomainDeviceDef dev_shallow;
>     int ret = -1;
>     unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
>                                VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
>@@ -8237,22 +8238,19 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
>     if (dev == NULL)
>         goto endjob;
>
>-    if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
>-        flags & VIR_DOMAIN_AFFECT_LIVE) {
>-        /* If we are affecting both CONFIG and LIVE
>-         * create a deep copy of device as adding
>-         * to CONFIG takes one instance.
>-         */
>-        dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt);
>-        if (!dev_copy)
>-            goto endjob;
>-    }
>-
>     if (priv->qemuCaps)
>         qemuCaps = virObjectRef(priv->qemuCaps);
>     else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator)))
>         goto endjob;
>
>+    /* Save away the pointer to the device object before it is
>+     * potentially swallowed up by qemuDomainAttachDeviceConfig().
>+     * This will allow us to make a copy of the device after any
>+     * modifications made by virDomainDefPostParse() (which is called
>+     * after the new device is added to the config
>+     */
>+    dev_shallow = *dev;
>+

This looks fragile and complicated, and I've already managed to break it
with the XML you provided:

$ virsh attach-device f24 cont --live
Device attached successfully

$ virsh attach-device f24 cont --live --config
error: Failed to attach device from cont
error: operation failed: target scsi:1 already exists

AFAIK the reason we create a deep copy instead of parsing it again is
our generation of MAC addresses in the parser:
commit 1e0534a770208be6b848c961785db20467deb2fc
    qemu: Don't parse device twice in attach/detach

So the question is: how should we treat the combination of --live and
--config? Try to make the persistent device as close as the live one?
Or try to make it match the persistent config (while the live one would
match the live config?)

Either way, it would be nicer to get the device definition stable
before we even try to add it to the persistent definition.

Also, calling virDomainDefPostParse after device coldplug is strange,
we should be adding a device that does not need ajdustments.

Jan