[libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach

[John Ferlan]

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