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@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). JanoNot sure what was meant about we seem to exit without logging an error. If virStorageBackendDeviceIsEmpty an error message is generated if falseis 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@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(-)
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
