[PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy
Laine Stump
laine at redhat.com
Thu Jan 6 02:42:38 UTC 2022
On 1/4/22 11:59 AM, Michal Privoznik wrote:
> In a few places (e.g. device attach/detach/update) we are given a
> device XML, parse it but then need a copy of parsed data so that
> the original can be passed to function handling the request over
> inactive XML and the copy is then passed to function handling the
> operation over live XML. Note, both functions consume passed
> device on success, hence the need for copy.
>
> The problem is in combination of how the copy is obtained and
> where is passed. The copy is done by calling
> virDomainDeviceDefCopy() which does only inactive copy, i.e. no
> live information is copied over (e.g. no aliases).
>
> Then, this copy (inactive XML effectively) is passed to function
> handling live part of the operation (e.g.
> qemuDomainUpdateDeviceLive()) and the definition containing all
> the juicy, live bits is passed to function handling inactive part
> of the operation (e.g. qemuDomainUpdateDeviceConfig()).
>
> This is rather suboptimal, and XML copies should be passed to
> their respective functions.
s/suboptimal/incorrect/ :-)
It's funny that the comment at the top of virDomainDeviceDefCopy
explicitly says that it's a copy of the inactive/config object, and that
every single place that function is called, its result is being put into
the live domain :-P
At first I was concerned that there were no matching chunks for
qemuDomainAttachDeviceFlags(), but then I took a look and realized that
function parses the XML twice instead of parsing it once and making a
copy (and I suppose I should mention the moment of cringe when, while
looking at that function, I came across some piece of ugly treachery I
had added to make sure the MAC addresses match between the copies. It
seems like this cringe moment happens every time I open a file these
days...)
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2036895
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
Reviewed-by: Laine Stump <laine at redhat.com>
> ---
> src/lxc/lxc_driver.c | 10 +++++-----
> src/qemu/qemu_driver.c | 8 ++++----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index fe583ccb76..7bc39120ee 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -4308,17 +4308,17 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
> false) < 0)
> goto endjob;
>
> - if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
> + if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev_copy)) < 0)
> goto endjob;
> }
>
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
> + if (virDomainDefCompatibleDevice(vm->def, dev, NULL,
> VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> true) < 0)
> goto endjob;
>
> - if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev_copy)) < 0)
> + if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev)) < 0)
> goto endjob;
> /*
> * update domain status forcibly because the domain status may be
> @@ -4475,12 +4475,12 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
> if (!vmdef)
> goto endjob;
>
> - if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0)
> + if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev_copy)) < 0)
> goto endjob;
> }
>
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0)
> + if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev)) < 0)
> goto endjob;
> /*
> * update domain status forcibly because the domain status may be
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b8537a4260..b1255da9f2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8020,7 +8020,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>
> /* virDomainDefCompatibleDevice call is delayed until we know the
> * device we're going to update. */
> - if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, priv->qemuCaps,
> + if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev_copy, priv->qemuCaps,
> parse_flags,
> driver->xmlopt)) < 0)
> goto endjob;
> @@ -8029,7 +8029,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> /* virDomainDefCompatibleDevice call is delayed until we know the
> * device we're going to update. */
> - if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0)
> + if ((ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force)) < 0)
> goto endjob;
>
> qemuDomainSaveStatus(vm);
> @@ -8100,7 +8100,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
> if (!vmdef)
> goto cleanup;
>
> - if (qemuDomainDetachDeviceConfig(vmdef, dev, priv->qemuCaps,
> + if (qemuDomainDetachDeviceConfig(vmdef, dev_copy, priv->qemuCaps,
> parse_flags,
> driver->xmlopt) < 0)
> goto cleanup;
> @@ -8109,7 +8109,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> int rc;
>
> - if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false)) < 0)
> + if ((rc = qemuDomainDetachDeviceLive(vm, dev, driver, false)) < 0)
> goto cleanup;
>
> if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
More information about the libvir-list
mailing list