8:52 a.m.
On Mon, Feb 11, 2019 at 07:41:41AM -0500, John Ferlan wrote:
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(a)redhat.com>
>> Reviewed-by: Erik Skultety <eskultet(a)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)?
Yes. Also, in that case we seem to exit without logging an error (as
opposed to calling virCommandRun which should log one).
Jano
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(a)redhat.com>
>
> Jano