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

[Ján Tomko]

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 at redhat.com>
>Reviewed-by: Erik Skultety <eskultet at redhat.com>
>---
> 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 at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190211/0e8b04b9/attachment-0001.sig>