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 <jfer...@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);
>>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list