1. Patchew
  2. Libvirt
  3. View series

[PATCH v2] qemu: Rewrite code to the pattern

Kristina Hanicova posted 1 patch 4 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f1550fdbeb341604e680196d891563d2a5b6e4c2.1637753073.git.khanicov@redhat.com
src/qemu/qemu_driver.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
[PATCH v2] qemu: Rewrite code to the pattern
Posted by Kristina Hanicova 4 years, 2 months ago 
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>
---

This is v2 of: https://listman.redhat.com/archives/libvir-list/2021-November/msg00747.html

Diff to v1:
* adding variable 'rc' to fix buggy code and keep the code
  equivalent (suggested by Jano)

 src/qemu/qemu_driver.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1959b639da..5c4b493f64 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15975,6 +15975,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
     g_autofree char *drivealias = NULL;
     const char *qdevid = NULL;
     int ret = -1;
+    int rc = 0;
     size_t i;
     virDomainDiskDef *conf_disk = NULL;
     virDomainDiskDef *disk;
@@ -16229,13 +16230,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
             !virStorageSourceIsEmpty(disk->src)) {
 
             qemuDomainObjEnterMonitor(driver, vm);
-            ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
-                                                &info);
-            if (qemuDomainObjExitMonitor(driver, vm) < 0)
-                ret = -1;
-            if (ret < 0)
+            rc = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid, &info);
+
+            if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
                 goto endjob;
-            ret = -1;
         }
 
         virDomainDiskSetBlockIOTune(disk, &info);
@@ -16310,6 +16308,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
     g_autofree char *drivealias = NULL;
     const char *qdevid = NULL;
     int ret = -1;
+    int rc = 0;
     int maxparams;
 
     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -16361,10 +16360,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
                 goto endjob;
         }
         qemuDomainObjEnterMonitor(driver, vm);
-        ret = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply);
-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
-            goto endjob;
-        if (ret < 0)
+        rc = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply);
+
+        if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
             goto endjob;
     }
 
@@ -17375,10 +17373,8 @@ 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
Re: [PATCH v2] qemu: Rewrite code to the pattern
Posted by Martin Kletzander 4 years, 2 months ago 
On Wed, Nov 24, 2021 at 12:25:35PM +0100, 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>
>---
>
>This is v2 of: https://listman.redhat.com/archives/libvir-list/2021-November/msg00747.html
>
>Diff to v1:
>* adding variable 'rc' to fix buggy code and keep the code
>  equivalent (suggested by Jano)
>
> src/qemu/qemu_driver.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 1959b639da..5c4b493f64 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -15975,6 +15975,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>     g_autofree char *drivealias = NULL;
>     const char *qdevid = NULL;
>     int ret = -1;
>+    int rc = 0;

Since this variable is not used in the function except ...

>     size_t i;
>     virDomainDiskDef *conf_disk = NULL;
>     virDomainDiskDef *disk;
>@@ -16229,13 +16230,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>             !virStorageSourceIsEmpty(disk->src)) {
>

... in this block, you can safely introduce it here ;)

>             qemuDomainObjEnterMonitor(driver, vm);
>-            ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
>-                                                &info);
>-            if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-                ret = -1;
>-            if (ret < 0)
>+            rc = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid, &info);
>+
>+            if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
>                 goto endjob;
>-            ret = -1;
>         }
>
>         virDomainDiskSetBlockIOTune(disk, &info);
>@@ -16310,6 +16308,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>     g_autofree char *drivealias = NULL;
>     const char *qdevid = NULL;
>     int ret = -1;
>+    int rc = 0;
>     int maxparams;
>
>     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>@@ -16361,10 +16360,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>                 goto endjob;
>         }
>         qemuDomainObjEnterMonitor(driver, vm);
>-        ret = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply);
>-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-            goto endjob;
>-        if (ret < 0)
>+        rc = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply);
>+
>+        if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)

Same here.

Anyway, since this is just a nitpick I can say this is

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>             goto endjob;
>     }
>
>@@ -17375,10 +17373,8 @@ 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
>
Re: [PATCH v2] qemu: Rewrite code to the pattern
Posted by Michal Prívozník 4 years, 2 months ago 
On 11/24/21 13:01, Martin Kletzander wrote:
> On Wed, Nov 24, 2021 at 12:25:35PM +0100, 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>
>> ---
>>
>> This is v2 of:
>> https://listman.redhat.com/archives/libvir-list/2021-November/msg00747.html
>>
>>
>> Diff to v1:
>> * adding variable 'rc' to fix buggy code and keep the code
>>  equivalent (suggested by Jano)
>>
>> src/qemu/qemu_driver.c | 22 +++++++++-------------
>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 1959b639da..5c4b493f64 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15975,6 +15975,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>     g_autofree char *drivealias = NULL;
>>     const char *qdevid = NULL;
>>     int ret = -1;
>> +    int rc = 0;
> 
> Since this variable is not used in the function except ...
> 
>>     size_t i;
>>     virDomainDiskDef *conf_disk = NULL;
>>     virDomainDiskDef *disk;
>> @@ -16229,13 +16230,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>             !virStorageSourceIsEmpty(disk->src)) {
>>
> 
> ... in this block, you can safely introduce it here ;)
> 
>>             qemuDomainObjEnterMonitor(driver, vm);
>> -            ret = qemuMonitorSetBlockIoThrottle(priv->mon,
>> drivealias, qdevid,
>> -                                                &info);
>> -            if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> -                ret = -1;
>> -            if (ret < 0)
>> +            rc = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias,
>> qdevid, &info);
>> +
>> +            if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
>>                 goto endjob;
>> -            ret = -1;
>>         }
>>
>>         virDomainDiskSetBlockIOTune(disk, &info);
>> @@ -16310,6 +16308,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>>     g_autofree char *drivealias = NULL;
>>     const char *qdevid = NULL;
>>     int ret = -1;
>> +    int rc = 0;
>>     int maxparams;
>>
>>     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>> @@ -16361,10 +16360,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>>                 goto endjob;
>>         }
>>         qemuDomainObjEnterMonitor(driver, vm);
>> -        ret = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias,
>> qdevid, &reply);
>> -        if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> -            goto endjob;
>> -        if (ret < 0)
>> +        rc = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias,
>> qdevid, &reply);
>> +
>> +        if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
> 
> Same here.
> 
> Anyway, since this is just a nitpick I can say this is
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Fixed and pushed.

Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.