[libvirt] [REPOST PATCHv2 0/2] Alter qemu live/cold device attach algorithm
REPOST of: https://www.redhat.com/archives/libvir-list/2018-June/msg00965.html Resolves conflicts with commit 5e9b150f and 4ad54a417 (adding @live and restoring the @action to virDomainDefCompatibleDevice method). The only change from the previous posting is an update to the commit message in patch 1 to futher describe why the alteration is being done and what the comparable is. John Ferlan (2): qemu: Check for existing hostdev address for cold attach device qemu: Use the correct vm def on cold attach src/qemu/qemu_driver.c | 58 +++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) -- 2.14.4
Prior to the hostdev being inserted in the hostdevs list, add a check during qemuDomainAttachDeviceConfig to determine whether the new/incoming <hostdev ...> device is providing the same <address> as some existing hostdev on the list and if so fail the cold attach. This cannot be done during virDomainHostdevDefPostParse because that is called after virDomainDefParseXML would have inserted a hostdev onto the hostdevs list and thus would have a "conflict" with itself. Therefore, the post parse processing can only compare if the current hostdev address conflicts with a SCSI <disk> address. By comparison this is similar to the validation phase checks in virDomainDefCheckDuplicateDriveAddresses that occur during define/startup processing but are not run during config attach of a live guest. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..ef1abe3f68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, _("device is already in the domain configuration")); return -1; } + if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a device with the same address already exists ")); + return -1; + } if (virDomainHostdevInsert(vmdef, hostdev)) return -1; dev->data.hostdev = NULL; -- 2.14.4
On 07/06/2018 08:50 PM, John Ferlan wrote:
Prior to the hostdev being inserted in the hostdevs list, add a check during qemuDomainAttachDeviceConfig to determine whether the new/incoming <hostdev ...> device is providing the same <address> as some existing hostdev on the list and if so fail the cold attach.
This cannot be done during virDomainHostdevDefPostParse because that is called after virDomainDefParseXML would have inserted a hostdev onto the hostdevs list and thus would have a "conflict" with itself. Therefore, the post parse processing can only compare if the current hostdev address conflicts with a SCSI <disk> address.
By comparison this is similar to the validation phase checks in virDomainDefCheckDuplicateDriveAddresses that occur during define/startup processing but are not run during config attach of a live guest.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..ef1abe3f68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, _("device is already in the domain configuration")); return -1; } + if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a device with the same address already exists ")); + return -1; + } if (virDomainHostdevInsert(vmdef, hostdev)) return -1; dev->data.hostdev = NULL;
I think hostdevs are not the only type of device suffering from this. In fact, I've just tested disks and libvirt accepts attaching another disk onto same <address/> happily. I wonder if this should go into virDomainDefCompatibleDevice() (now that we have @action there ;-) ). Michal
On 07/09/2018 10:32 AM, Michal Privoznik wrote:
On 07/06/2018 08:50 PM, John Ferlan wrote:
Prior to the hostdev being inserted in the hostdevs list, add a check during qemuDomainAttachDeviceConfig to determine whether the new/incoming <hostdev ...> device is providing the same <address> as some existing hostdev on the list and if so fail the cold attach.
This cannot be done during virDomainHostdevDefPostParse because that is called after virDomainDefParseXML would have inserted a hostdev onto the hostdevs list and thus would have a "conflict" with itself. Therefore, the post parse processing can only compare if the current hostdev address conflicts with a SCSI <disk> address.
By comparison this is similar to the validation phase checks in virDomainDefCheckDuplicateDriveAddresses that occur during define/startup processing but are not run during config attach of a live guest.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..ef1abe3f68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, _("device is already in the domain configuration")); return -1; } + if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a device with the same address already exists ")); + return -1; + } if (virDomainHostdevInsert(vmdef, hostdev)) return -1; dev->data.hostdev = NULL;
I think hostdevs are not the only type of device suffering from this. In fact, I've just tested disks and libvirt accepts attaching another disk onto same <address/> happily.
I wonder if this should go into virDomainDefCompatibleDevice() (now that we have @action there ;-) ).
It's a case of myopia for the bug I'm working on as listed in patch2. What I did is no different than the VIR_DOMAIN_DEVICE_RNG check in the same function. Still, if I change this patch to add: if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live && data.newInfo && data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDefHasDeviceAddress(def, data.newInfo)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("a device with the same address already exists ")); return -1; } to virDomainDefCompatibleDevice and remove the (new)hostdev and (existing)rng checks, then I believe it covers the existing cases. Tks - John
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:
Prior to the hostdev being inserted in the hostdevs list, add a check during qemuDomainAttachDeviceConfig to determine whether the new/incoming <hostdev ...> device is providing the same <address> as some existing hostdev on the list and if so fail the cold attach.
This cannot be done during virDomainHostdevDefPostParse because that is called after virDomainDefParseXML would have inserted a hostdev onto the hostdevs list and thus would have a "conflict" with itself. Therefore, the post parse processing can only compare if the current hostdev address conflicts with a SCSI <disk> address.
By comparison this is similar to the validation phase checks in virDomainDefCheckDuplicateDriveAddresses that occur during define/startup processing but are not run during config attach of a live guest.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..ef1abe3f68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, _("device is already in the domain configuration")); return -1; } + if (dev->data.hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a device with the same address already exists ")); + return -1; + } if (virDomainHostdevInsert(vmdef, hostdev)) return -1; dev->data.hostdev = NULL;
I think hostdevs are not the only type of device suffering from this. In fact, I've just tested disks and libvirt accepts attaching another disk onto same <address/> happily.
I wonder if this should go into virDomainDefCompatibleDevice() (now that we have @action there ;-) ).
It's a case of myopia for the bug I'm working on as listed in patch2.
What I did is no different than the VIR_DOMAIN_DEVICE_RNG check in the same function.
Still, if I change this patch to add:
if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live && data.newInfo && data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDefHasDeviceAddress(def, data.newInfo)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("a device with the same address already exists ")); return -1; }
to virDomainDefCompatibleDevice and remove the (new)hostdev and (existing)rng checks, then I believe it covers the existing cases.
Yes, this looks reasonable. Michal
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@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; + + 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 @@ -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); -- 2.14.4
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@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? 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)? Otherwise, the patch looks good. Michal
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@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. John
Otherwise, the patch looks good.
Michal
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@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
participants (2)
-
John Ferlan -
Michal Privoznik