[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