src/qemu/qemu_driver.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
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
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 >
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
© 2016 - 2024 Red Hat, Inc.