Re: [PATCH v2] qemu: Rewrite code to the pattern

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(a)redhat.com&gt; > --- > > 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(a)redhat.com&gt;