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@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;
+
             ret = -1;
         }
 
@@ -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)
             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;
     }
 
-- 
2.31.1

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

This patch rewrites the pattern using early return where it is
not needed.

Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
 src/qemu/qemu_alias.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 5e35f43614..6f664fcd84 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -361,10 +361,9 @@ static int
 qemuAssignDeviceSoundAlias(virDomainSoundDef *sound,
                            int idx)
 {
-    if (sound->info.alias)
-        return 0;
+    if (!sound->info.alias)
+        sound->info.alias = g_strdup_printf("sound%d", idx);
 
-    sound->info.alias = g_strdup_printf("sound%d", idx);
     return 0;
 }
 
@@ -373,10 +372,9 @@ static int
 qemuAssignDeviceVideoAlias(virDomainVideoDef *video,
                            int idx)
 {
-    if (video->info.alias)
-        return 0;
+    if (!video->info.alias)
+        video->info.alias = g_strdup_printf("video%d", idx);
 
-    video->info.alias = g_strdup_printf("video%d", idx);
     return 0;
 }
 
@@ -385,10 +383,9 @@ static int
 qemuAssignDeviceHubAlias(virDomainHubDef *hub,
                          int idx)
 {
-    if (hub->info.alias)
-        return 0;
+    if (!hub->info.alias)
+        hub->info.alias = g_strdup_printf("hub%d", idx);
 
-    hub->info.alias = g_strdup_printf("hub%d", idx);
     return 0;
 }
 
@@ -397,10 +394,9 @@ static int
 qemuAssignDeviceSmartcardAlias(virDomainSmartcardDef *smartcard,
                                int idx)
 {
-    if (smartcard->info.alias)
-        return 0;
+    if (!smartcard->info.alias)
+        smartcard->info.alias = g_strdup_printf("smartcard%d", idx);
 
-    smartcard->info.alias = g_strdup_printf("smartcard%d", idx);
     return 0;
 }
 
@@ -409,10 +405,9 @@ static int
 qemuAssignDeviceMemballoonAlias(virDomainMemballoonDef *memballoon,
                                 int idx)
 {
-    if (memballoon->info.alias)
-        return 0;
+    if (!memballoon->info.alias)
+        memballoon->info.alias = g_strdup_printf("balloon%d", idx);
 
-    memballoon->info.alias = g_strdup_printf("balloon%d", idx);
     return 0;
 }
 
@@ -421,10 +416,9 @@ static int
 qemuAssignDeviceTPMAlias(virDomainTPMDef *tpm,
                          int idx)
 {
-    if (tpm->info.alias)
-        return 0;
+    if (!tpm->info.alias)
+        tpm->info.alias = g_strdup_printf("tpm%d", idx);
 
-    tpm->info.alias = g_strdup_printf("tpm%d", idx);
     return 0;
 }
 
@@ -586,10 +580,8 @@ qemuAssignDeviceWatchdogAlias(virDomainWatchdogDef *watchdog)
 {
     /* Currently, there's just one watchdog per domain */
 
-    if (watchdog->info.alias)
-        return 0;
-
-    watchdog->info.alias = g_strdup("watchdog0");
+    if (!watchdog->info.alias)
+        watchdog->info.alias = g_strdup("watchdog0");
 
     return 0;
 }
@@ -621,9 +613,8 @@ qemuAssignDeviceInputAlias(virDomainDef *def,
 int
 qemuAssignDeviceVsockAlias(virDomainVsockDef *vsock)
 {
-    if (vsock->info.alias)
-        return 0;
-    vsock->info.alias = g_strdup("vsock0");
+    if (!vsock->info.alias)
+        vsock->info.alias = g_strdup("vsock0");
 
     return 0;
 }
-- 
2.31.1

On Tue, Nov 23, 2021 at 07:35:08PM +0100, Kristina Hanicova wrote:
>This patch rewrites the pattern using early return where it is
>not needed.
>

At which point there is probably no need for the functions to return an
integer.  Changing that to void could remove even more code.