Re: [PATCH 1/2] qemu: remove unnecessary else branches after return / goto

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Thursday in 2022, Kristina Hanicova wrote:
> I think the code looks cleaner without else branches.
>
> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> ---
> src/qemu/qemu_domainjob.c |  3 +--
> src/qemu/qemu_migration.c | 13 +++++++------
> src/qemu/qemu_process.c   |  4 ++--
> src/qemu/qemu_snapshot.c  | 13 ++++++-------
> 4 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index cf1e093e22..6bf3a0ab42 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -153,8 +153,7 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
>
>     if (STREQ(phase, "none"))
>         return 0;
> -    else
> -        return -1;
> +    return -1;

I think it would look even better with a blank line above the ending
return.

> }
>
>
> @@ -6189,8 +6191,7 @@ qemuMigrationDstErrorInit(virQEMUDriver *driver)
>     driver->migrationErrors = virHashAtomicNew(qemuMigrationDstErrorFree);
>     if (driver->migrationErrors)
>         return 0;
> -    else
> -        return -1;
> +    return -1;

Same here.

> }
>
> /**
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1ed60917ea..7a36f85d7e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8700,8 +8700,8 @@ qemuProcessRefreshBlockjobs(virQEMUDriver *driver,
>
>     if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
>         return qemuBlockJobRefreshJobs(driver, vm);
> -    else
> -        return qemuProcessRefreshLegacyBlockjobs(driver, vm);
> +
> +    return qemuProcessRefreshLegacyBlockjobs(driver, vm);
> }
>
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 5333730df1..212d37d3b4 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -583,13 +583,12 @@ qemuSnapshotPrepareDiskExternal(virDomainObj *vm,
>                                      _("missing existing file for disk %s: %s"),
>                                      snapdisk->name, snapdisk->src->path);
>                 return -1;
> -            } else {
> -                if (snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK) {
> -                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> -                                   _("block device snapshot target '%s' doesn't exist"),
> -                                   snapdisk->src->path);
> -                    return -1;
> -                }
> +            }

This also looks crowded without the empty line.

> +            if (snapdisk->src->type == VIR_STORAGE_TYPE_BLOCK) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                               _("block device snapshot target '%s' doesn't exist"),
> +                               snapdisk->src->path);
> +                return -1;
>             }
>         } else {
>             /* at this point VIR_STORAGE_TYPE_DIR was already rejected */

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature