Re: [PATCH v2 11/32] storage: Use VIR_AUTOPTR(virCommand)

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
> Let's make use of the auto __cleanup capabilities cleaning up any
> now unnecessary goto paths.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
> src/storage/storage_backend_disk.c     |  85 +++++++++------------
> src/storage/storage_backend_fs.c       |  39 +++-------
> src/storage/storage_backend_logical.c  | 101 +++++++------------------
> src/storage/storage_backend_sheepdog.c |  59 ++++++---------
> src/storage/storage_backend_vstorage.c |  14 +---
> src/storage/storage_backend_zfs.c      |  47 +++---------
> src/storage/storage_driver.c           |   3 +-
> src/storage/storage_util.c             |  34 +++------
> src/util/virstoragefile.c              |  67 +++++++---------
> tests/storagepoolxml2argvtest.c        |   7 +-
> tests/storagevolxml2argvtest.c         |   6 +-
> tests/virstoragetest.c                 |   6 +-
> 12 files changed, 156 insertions(+), 312 deletions(-)
>
> @@ -502,51 +500,40 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
>     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>     int format = def->source.format;
>     const char *fmt;
> -    bool ok_to_mklabel = false;
> -    int ret = -1;
> -    virCommandPtr cmd = NULL;
> +    VIR_AUTOPTR(virCommand) cmd = NULL;
>
>     virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
> -                  VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
> +                  VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>
> -    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
> -                             VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
> -                             error);
> +    VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
> +                            VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
> +                            -1);
>
>     fmt = virStoragePoolFormatDiskTypeToString(format);
> -    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
> -        ok_to_mklabel = true;
> -    } else {
> -        if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
> -                                           fmt, true))
> -            ok_to_mklabel = true;
> -    }
> +    if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
> +        !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
> +                                         fmt, true)))
> +        return -1;
>
> -    if (ok_to_mklabel) {
> -        if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
> -                                                1024 * 1024) < 0)
> -            goto error;
> +    if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
> +                                            1024 * 1024) < 0)
> +        return -1;
>
> -        /* eg parted /dev/sda mklabel --script msdos */
> -        if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
> -            format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
> -        if (format == VIR_STORAGE_POOL_DISK_DOS)
> -            fmt = "msdos";
> -        else
> -            fmt = virStoragePoolFormatDiskTypeToString(format);
> -
> -        cmd = virCommandNewArgList(PARTED,
> -                                   def->source.devices[0].path,
> -                                   "mklabel",
> -                                   "--script",
> -                                   fmt,
> -                                   NULL);
> -        ret = virCommandRun(cmd, NULL);
> -    }
> +    /* eg parted /dev/sda mklabel --script msdos */
> +    if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
> +        format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
> +    if (format == VIR_STORAGE_POOL_DISK_DOS)
> +        fmt = "msdos";
> +    else
> +        fmt = virStoragePoolFormatDiskTypeToString(format);
>
> - error:
> -    virCommandFree(cmd);
> -    return ret;
> +    cmd = virCommandNewArgList(PARTED,
> +                               def->source.devices[0].path,
> +                               "mklabel",
> +                               "--script",
> +                               fmt,
> +                               NULL);
> +    return virCommandRun(cmd, NULL);
> }

Apart from the usual mixing of the ret->goto changes with adding
AUTOFREE, this also removes the 'ok_to_mklabel' bool.
Those changes really should be separated.

> @@ -341,33 +332,30 @@ static int
> virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool,
>                                     virStorageVolDefPtr vol)
> {
> -    int ret;
>     char *output = NULL;
>     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> -    virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
> +    VIR_AUTOPTR(virCommand) cmd = NULL;
>
> +    cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
>     virStorageBackendSheepdogAddHostArg(cmd, pool);
>     virCommandSetOutputBuffer(cmd, &output);
> -    ret = virCommandRun(cmd, NULL);
> -
> -    if (ret < 0)
> -        goto cleanup;
> +    if (virCommandRun(cmd, NULL) < 0)
> +        return -1;
>
> -    if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
> -        goto cleanup;
> +    if (virStorageBackendSheepdogParseVdiList(vol, output) < 0)
> +        return -1;
>
>     vol->type = VIR_STORAGE_VOL_NETWORK;
>
>     VIR_FREE(vol->key);
>     if (virAsprintf(&vol->key, "%s/%s",
>                     def->source.name, vol->name) < 0)
> -        goto cleanup;
> +        return -1;

Before, '0' from the virStorageBackendSheepdogParseVdiList would be
returned. While correct, it would look better in a separate patch.

>     VIR_FREE(vol->target.path);
>     ignore_value(VIR_STRDUP(vol->target.path, vol->name));
> - cleanup:
> -    virCommandFree(cmd);
> -    return ret;
> +
> +    return 0;
> }

To everything apart from virStorageBackendDiskBuildPool:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature