On 07/13/2018 02:50 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >>> --- >>> 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. Okay, fair enough. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
