On 07/25/2018 06:40 AM, Michal Privoznik wrote: > On 07/16/2018 11:14 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 >> performing 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 7d519c0714..3dbd5c62d2 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -8473,7 +8473,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 | >> @@ -8487,49 +8488,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; >> + >> + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) >> + goto cleanup; >> + >> + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL, >> VIR_DOMAIN_DEVICE_ACTION_ATTACH, >> true) < 0) >> goto cleanup; >> >> - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0) >> + if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0) >> goto cleanup; >> /* >> * update domain status forcibly because the domain status may be >> @@ -8553,9 +8548,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, >> >> cleanup: >> virDomainDefFree(vmdef); >> - if (dev != dev_copy) >> - virDomainDeviceDefFree(dev_copy); >> - virDomainDeviceDefFree(dev); >> + virDomainDeviceDefFree(devConf); >> + virDomainDeviceDefFree(devLive); >> virObjectUnref(cfg); >> virObjectUnref(caps); >> >> > > I'm failing to see why the other patches are necessary. I'm unable to > reproduce the bug after I applied only this single patch. > It also makes sense, because the source of error is that when parsing > device XML wrong vmdef was passed. Therefore, when postParse callback > tried to fill device address it was looking at wrong data (thinking > there's no such drive address) and returned the next free address which > was the same all the time. > > ACK to this patch. > > Michal > OK - fair enough. It's been a few weeks, but I thought I had gone through some testing with only this patch applied and attempting the attach of a <hostdev> without an <address> defined and was getting the same unit#, but now of course with just this applied I cannot. So much for short term memory. Thanks for the review - I'll drop 1 and 2 and just push 3 - John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
