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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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

>             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

Attachment: signature.asc
Description: PGP signature