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@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)?
>>
>
> 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. Here's what I have for the difference (attached with any
luck):
John
>From 41861fe512b5d7d71e50601f8d090e55f0293f26 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
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@xxxxxxxxxx>
---
src/storage/storage_backend_disk.c | 51 ++++++++++++++----------------
1 file changed, 23 insertions(+), 28 deletions(-)
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
