Re: [libvirt] [REPOST PATCHv2 2/2] qemu: Use the correct vm def on cold attach

On 07/06/2018 08:50 PM, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1559867 > > When attaching a device to the domain we need to be sure > to use the correct domain definition (vm->def or vm->newDef) > when calling virDomainDeviceDefParse because the post parse > processing algorithms that may assign an address for the > device will use whatever domain definition was passed in. > > Additionally, some devices (SCSI hostdev and SCSI disk) use > algorithms that rely on knowing what already exists of the > other type when generating the new device's address. Using > the wrong VM definition could result in duplicated addresses. > > In the case of the bz, two hostdev's with no domain address > provided were added to the running domain's config only. > However, the parsing algorithm used the live domain in > order to figure out the host device address resulting in > the same address being used and a subsequent start failing > due to duplicate address. > > Fix this by separating the checks/code into CONFIG and LIVE > processing using the correct definition for each block and > peforming cleanup for both options as necessary. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++---------------------------- > 1 file changed, 23 insertions(+), 29 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index ef1abe3f68..60085befea 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8469,7 +8469,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, > { > virDomainDefPtr vmdef = NULL; > virQEMUDriverConfigPtr cfg = NULL; > - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; > + virDomainDeviceDefPtr devConf = NULL; > + virDomainDeviceDefPtr devLive = NULL; > int ret = -1; > virCapsPtr caps = NULL; > unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | > @@ -8483,49 +8484,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, > if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > goto cleanup; > > - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, > - caps, driver->xmlopt, > - parse_flags); > - if (dev == NULL) > - goto cleanup; > - > - if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0) > - goto cleanup; > - > - 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 cleanup; > - } > - > + /* The config and live post processing address auto-generation algorithms > + * rely on the correct vm->def or vm->newDef being passed, so call the > + * device parse based on which definition is in use */ > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > - /* Make a copy for updated domain. */ > vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); > if (!vmdef) > goto cleanup; > > - if (virDomainDefCompatibleDevice(vmdef, dev, NULL, > + if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps, > + driver->xmlopt, parse_flags))) > + goto cleanup; > + > + if (virDomainDefCompatibleDevice(vmdef, devConf, NULL, > VIR_DOMAIN_DEVICE_ACTION_ATTACH, > false) < 0) > goto cleanup; > - if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps, > + > + if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps, > parse_flags, > driver->xmlopt)) < 0) > goto cleanup; > } > > if (flags & VIR_DOMAIN_AFFECT_LIVE) { > - if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL, > + if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps, > + driver->xmlopt, parse_flags))) > + goto cleanup; > + In the light of 84de7fbfdb2f528e05d98c09e3260fe0fc0739f9: shouldn't we parse this as live XML?

Also don't other drivers suffer the same problem (even though I vaguely recall you posting a patch to forbid live attach for lxc driver)?

Otherwise, the patch looks good. Michal