On 06/12/2018 06:06 PM, Laine Stump wrote: > On 06/12/2018 10:32 AM, 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. > > So what happens if someone requests only LIVE for the hotplug of one > device, and then both LIVE and CONFIG for another? Does the 2nd device > get a different address in the running guest than it has in the > persistent config? > Hmmm... good question. For the live, then live+config case: my test live domain has: ... <adapter name='scsi_host5'/> ... <address type='drive' controller='0' bus='0' target='0' unit='0'/> ... <adapter name='scsi_host6'/> ... <address type='drive' controller='0' bus='0' target='0' unit='1'/> config domain has: ... <adapter name='scsi_host6'/> ... <address type='drive' controller='0' bus='0' target='0' unit='0'/> ... So, yes, different, but then again, unsupplied. We have a chicken and egg problem I think... Still if someone added scsi_host5 to the config afterwards it'd get unit='1'. Do we guarantee anything if someone doesn't supply the address? The virsh man page says: "Note: using of partial device definition XML files may lead to unexpected results as some fields may be autogenerated and thus match devices other than expected." WYSIWYG. John >> >> 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 ae8e0e898a..494141af4a 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,45 +8488,39 @@ 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) < 0) >> + if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps, >> + driver->xmlopt, parse_flags))) >> + goto cleanup; >> + >> + if (virDomainDefCompatibleDevice(vmdef, devConf, NULL) < 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) < 0) >> + if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps, >> + driver->xmlopt, parse_flags))) >> goto cleanup; >> >> - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0) >> + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) >> + goto cleanup; >> + >> + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL) < 0) >> + goto cleanup; >> + >> + if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0) >> goto cleanup; >> /* >> * update domain status forcibly because the domain status may be >> @@ -8549,9 +8544,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, >> >> cleanup: >> virDomainDefFree(vmdef); >> - if (dev != dev_copy) >> - virDomainDeviceDefFree(dev_copy); >> - virDomainDeviceDefFree(dev); >> + virDomainDeviceDefFree(devConf); >> + virDomainDeviceDefFree(devLive); >> virObjectUnref(cfg); >> virObjectUnref(caps); >> > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
