[libvirt] [PATCH v2 11/32] storage: Use VIR_AUTOPTR(virCommand)
Ján Tomko
jtomko at redhat.com
Mon Feb 11 14:52:37 UTC 2019
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 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)?
>
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 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/22a0bb05/attachment-0001.sig>
More information about the libvir-list
mailing list