6:08 p.m.
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(a)redhat.com>
> ---
> 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);
>