Re: [libvirt] [PATCH v2 11/32] storage: Use VIR_AUTOPTR(virCommand)

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&gt; >>>> Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; >>>> --- >>>> 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(a)redhat.com&gt; 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(a)redhat.com&gt; --- 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