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

[Michal Privoznik]

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 at 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 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?

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)?

Otherwise, the patch looks good.

Michal