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

[John Ferlan]

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.  Here's what I have for the difference (attached with any
luck):

John

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-storage-Rework-logic-in-virStorageBackendDiskBuildPo.patch
Type: text/x-patch
Size: 3304 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190211/e3317a49/attachment-0001.bin>