8:33 p.m.
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(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
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