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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
>> ---
>>  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.

John

> 
> Otherwise, the patch looks good.
> 
> Michal
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux