[libvirt] [PATCH v2 2/2] qemu: Use the correct vm def on cold attach

[John Ferlan]

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