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

[Laine Stump <laine@xxxxxxxxxx>]

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


Reviewed-by: Laine Stump <laine@xxxxxxxxxx>


> ---
>   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)