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

On 07/09/2018 10:32 AM, Michal Privoznik wrote: > 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? > Let's see: qemuDomainAttachDeviceFlags calls virDomainObjUpdateModificationImpact and then calls qemuDomainAttachDeviceLiveAndConfig, so if I'm reading your question properly, then I think we're good here. > 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)? Beyond the scope of the bz, but if one follows the various driver domainAttachDevice entry points they could get the answer. Yes, I had a recent patch for lxc for UpdateDevice (commit id fbe4a458), but all I did there was rearrange the code to account for an earlier change that had removed the live update option.