[libvirt] [PATCH v2 11/32] storage: Use VIR_AUTOPTR(virCommand)
Ján Tomko
jtomko at redhat.com
Tue Feb 12 05:30:46 UTC 2019
On Mon, Feb 11, 2019 at 09:33:53PM -0500, John Ferlan wrote:
>
>
>On 2/11/19 9:52 AM, Ján Tomko wrote:
>> 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
>
>Not sure what was meant about we seem to exit without logging an error.
>If virStorageBackendDeviceIsEmpty an error message is generated if false
>is returned.
False alarm. Looking at the old code it seemed it was possible to fall
through with ok_to_mklabel = false, but the new code makes it more
readable.
>Here's what I have for the difference (attached with any
>luck):
Please use git send-email to post patches, they're easier to apply.
>
>John
>
>From 41861fe512b5d7d71e50601f8d090e55f0293f26 Mon Sep 17 00:00:00 2001
>From: John Ferlan <jferlan at redhat.com>
>Date: Mon, 11 Feb 2019 21:29:29 -0500
>Subject: [PATCH] storage: Rework logic in virStorageBackendDiskBuildPool
>
>Rework the logic to remove the need for the @ok_to_mklabel boolean.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
> src/storage/storage_backend_disk.c | 51 ++++++++++++++----------------
> 1 file changed, 23 insertions(+), 28 deletions(-)
>
Reviewed-by: Ján Tomko <jtomko at redhat.com>
Jano
>diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
>index 061c494b7d..abbe1c3e8b 100644
>--- a/src/storage/storage_backend_disk.c
>+++ b/src/storage/storage_backend_disk.c
>@@ -502,7 +502,6 @@ 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;
>
>@@ -514,35 +513,31 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
> error);
>
> 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 (ok_to_mklabel) {
>- if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>- 1024 * 1024) < 0)
>- goto error;
>+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>+ fmt, true)))
>+ goto error;
>
>- /* 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);
>- }
>+ if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>+ 1024 * 1024) < 0)
>+ goto error;
>+
>+ /* 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);
>
> error:
> virCommandFree(cmd);
>--
>2.20.1
>
-------------- 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/20190212/b68db928/attachment-0001.sig>
More information about the libvir-list
mailing list