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

[Michal Privoznik]

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