Re: [PATCH] lib: Drop needless one line labels

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Friday in 2021, Michal Privoznik wrote:
> In some cases we have a label that contains nothing but a return
> statement. The amount of such labels rises as we use automagic
> cleanup. Anyway, such labels are pointless and can be dropped.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> examples/c/domain/dommigrate.c       |  6 +-
> src/conf/domain_conf.c               | 70 +++++++++-----------
> src/conf/netdev_vport_profile_conf.c | 33 +++++-----
> src/conf/storage_conf.c              | 12 ++--
> src/conf/storage_source_conf.c       | 15 ++---
> src/conf/virsavecookie.c             |  3 +-
> src/hypervisor/virhostdev.c          |  4 +-
> src/libxl/libxl_migration.c          |  5 +-
> src/libxl/xen_xl.c                   |  9 +--
> src/openvz/openvz_driver.c           |  5 +-
> src/qemu/qemu_capabilities.c         |  5 +-
> src/qemu/qemu_monitor_json.c         | 16 ++---
> src/qemu/qemu_snapshot.c             | 21 +++---
> src/qemu/qemu_virtiofs.c             | 22 +++----
> src/rpc/virnetlibsshsession.c        | 10 +--
> src/storage/storage_backend_iscsi.c  | 13 ++--
> src/test/test_driver.c               |  5 +-
> src/util/vircgroup.c                 |  4 +-
> src/util/vircgroupv1.c               | 24 +++----
> src/util/virdnsmasq.c                | 42 +++++-------
> src/util/virhostcpu.c                | 11 ++--
> tests/domainconftest.c               |  5 +-
> tests/qemuxml2argvtest.c             | 11 +---
> tests/virpcitest.c                   | 15 ++---
> tools/virsh-domain.c                 |  8 +--
> tools/virsh-host.c                   |  6 +-
> tools/virsh-nodedev.c                | 45 +++++--------
> tools/virsh-volume.c                 | 96 +++++++++++-----------------
> 28 files changed, 199 insertions(+), 322 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 9186d59ca2..731227b158 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2058,16 +2058,16 @@ qemuMonitorJSONGetMemoryStats(qemuMonitor *mon,

Context:

    int got = 0;

    ret = qemuMonitorJSONGetBalloonInfo(mon, &mem);
    if (ret == 1 && (got < nr_stats)) {
        stats[got].tag = VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON;
        stats[got].val = mem;
        got++;
>     }

In this function, 'got' holds the value this function, while 'ret'
is used for the value of the called function.

Returning 'ret' instead of 'got' (which is pre-existing) relies
on qemuMonitorJSONGetBalloonInfo being called first and its return value
matching the one qemuMonitorJSONGetMemoryStats will want to return.

I think it would be easier to follow if you returned 'got' everywhere
and 'ret' (which can be renamed to rc or rv per our conventions) would
only be used to store qemuMonitorJSONGetBalloonInfo's return value.

>     if (!balloonpath)
> -        goto cleanup;
> +        return ret;
>
>     if (!(cmd = qemuMonitorJSONMakeCommand("qom-get",
>                                            "s:path", balloonpath,
>                                            "s:property", "guest-stats",
>                                            NULL)))
> -        goto cleanup;
> +        return ret;
>
>     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> -        goto cleanup;
> +        return ret;
>
>     if ((data = virJSONValueObjectGetObject(reply, "error"))) {
>         const char *klass = virJSONValueObjectGetString(data, "class");
> diff --git a/tests/virpcitest.c b/tests/virpcitest.c
> index 4c0f0b91c3..ad2fcc6751 100644
> --- a/tests/virpcitest.c
> +++ b/tests/virpcitest.c
> @@ -81,7 +81,7 @@ testVirPCIDeviceNew(const void *opaque G_GNUC_UNUSED)
>         virReportError(VIR_ERR_INTERNAL_ERROR, \
>                        "Unexpected count of items in " #list ": %d, " \
>                        "expecting %zu", count, (size_t) cnt); \
> -        goto cleanup; \
> +        return -1; \

You cannot change this macro without adjusting all the callers.

Namely testVirPCIDeviceDetach looks like it would now leak memory on
error. Not sure about testVirPCIDeviceReset

>     }
>
> static int
> @@ -165,7 +165,6 @@ testVirPCIDeviceReset(const void *opaque G_GNUC_UNUSED)
> static int
> testVirPCIDeviceReattach(const void *opaque G_GNUC_UNUSED)
> {
> -    int ret = -1;
>     virPCIDevice *dev[] = {NULL, NULL, NULL};
>     size_t i, nDev = G_N_ELEMENTS(dev);
>     g_autoptr(virPCIDeviceList) activeDevs = NULL;
> @@ -174,17 +173,17 @@ testVirPCIDeviceReattach(const void *opaque G_GNUC_UNUSED)
>
>     if (!(activeDevs = virPCIDeviceListNew()) ||
>         !(inactiveDevs = virPCIDeviceListNew()))
> -        goto cleanup;
> +        return -1;
>
>     for (i = 0; i < nDev; i++) {
>         virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0,
>                                        .slot = i + 1, .function = 0};
>         if (!(dev[i] = virPCIDeviceNew(&devAddr)))
> -            goto cleanup;
> +            return -1;
>
>         if (virPCIDeviceListAdd(inactiveDevs, dev[i]) < 0) {
>             virPCIDeviceFree(dev[i]);
> -            goto cleanup;
> +            return -1;
>         }
>
>         CHECK_LIST_COUNT(activeDevs, 0);
> @@ -198,15 +197,13 @@ testVirPCIDeviceReattach(const void *opaque G_GNUC_UNUSED)
>
>     for (i = 0; i < nDev; i++) {
>         if (virPCIDeviceReattach(dev[i], activeDevs, inactiveDevs) < 0)
> -            goto cleanup;
> +            return -1;
>
>         CHECK_LIST_COUNT(activeDevs, 0);
>         CHECK_LIST_COUNT(inactiveDevs, nDev - i - 1);
>     }
>
> -    ret = 0;
> - cleanup:
> -    return ret;
> +    return 0;
> }
>
> struct testPCIDevData {
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index fc84415e7b..5ae6657347 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -1122,11 +1122,11 @@ vshExtractCPUDefXMLs(vshControl *ctl,
>         }
>     }
>
> - cleanup:
> -    return g_steal_pointer(&cpus);
> +    return cpus;

This will return autofree'd memory.

You can leave this return as-is, remove both labels and use 'return
NULL' instead of 'goto error'.

>  error:
> -    goto cleanup;
> +    g_strfreev(cpus);
> +    return NULL;
> }

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

Jano

Attachment: signature.asc
Description: PGP signature