1. Patchew
  2. Libvirt
  3. View series

[PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy

Michal Privoznik posted 1 patch 2 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ec6108e58e44c0d9544331a3cee45f2e9a5ddf19.1641315547.git.mprivozn@redhat.com
src/lxc/lxc_driver.c   | 10 +++++-----
src/qemu/qemu_driver.c |  8 ++++----
2 files changed, 9 insertions(+), 9 deletions(-)
[PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy
Posted by Michal Privoznik 2 years, 3 months ago 
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.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2036895
Signed-off-by: Michal Privoznik <mprivozn@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)
-- 
2.34.1
Re: [PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy
Posted by Laine Stump 2 years, 3 months ago 
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@redhat.com>


Reviewed-by: Laine Stump <laine@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)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.