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@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.
>
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@xxxxxxxxxx>
>
> Jano
