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

[John Ferlan]

On 2/11/19 7:33 AM, Ján Tomko wrote:
> 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.
> 

Just so it's clear what's being requested, does this mean taking the
current code and adding the:

+    if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
+        !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
+                                         fmt, true)))
+        goto error;

and then reformatting the rest inline as is done here (more or less)?

John

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