[PATCH] qemu: Rewrite code to the pattern

[Ján Tomko]

On a Tuesday in 2021, Kristina Hanicova wrote:
>I have seen this pattern a lot in the project, so I decided to
>rewrite code I stumbled upon to the same pattern as well.
>
>Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
>---
> src/qemu/qemu_driver.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 1959b639da..b938687189 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -16231,10 +16231,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>             qemuDomainObjEnterMonitor(driver, vm);
>             ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
>                                                 &info);
>-            if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-                ret = -1;
>-            if (ret < 0)
>+            if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)
>                 goto endjob;

This is not equivalent code.

Before, if qemuDomainObjExitMonitor failed, we would set ret to -1
before goto endjob.

Now, if qemuDomainObjExitMonitor fails, we will return whatever
qemuMonitorSetBlockIoThrottle returned.

>+
>             ret = -1;

This is an odd assignment. Introduced by 87ee705183241a42ffd36d9f5d3934cacf91c343
it resets 'ret' to its original state after we've used it for the return
value of 'qemuMonitorSetBlockIoThrottle'.

Introducing a separate 'rc' or 'rv' variable for that purpose would
solve the oddness and remove the need to reset ret back to -1 if qemuDomainObjExitMonitor
failed.

>         }
>
>@@ -16362,9 +16361,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>         }
>         qemuDomainObjEnterMonitor(driver, vm);
>         ret = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply);
>-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-            goto endjob;
>-        if (ret < 0)
>+        if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)

Here, the existing code is buggy. It can return 0 from qemuMonitorGetBlockIoThrottle
even if qemuDomainObjExitMonitor failed.

>             goto endjob;
>     }
>
>@@ -17375,10 +17372,7 @@ qemuDomainSetTime(virDomainPtr dom,
>     if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) {
>         qemuDomainObjEnterMonitor(driver, vm);
>         rv = qemuMonitorRTCResetReinjection(priv->mon);
>-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-            goto endjob;
>-
>-        if (rv < 0)
>+        if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
>             goto endjob;
>     }

This one looks good.

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211124/ac1bf498/attachment-0001.sig>