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