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

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.

> @@ -341,33 +332,30 @@ static int > virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool, >                                     virStorageVolDefPtr vol) > { > -    int ret; >     char *output = NULL; >     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); > -    virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", > "list", vol->name, "-r", NULL); > +    VIR_AUTOPTR(virCommand) cmd = NULL; > > +    cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, > "-r", NULL); >     virStorageBackendSheepdogAddHostArg(cmd, pool); >     virCommandSetOutputBuffer(cmd, &output); > -    ret = virCommandRun(cmd, NULL); > - > -    if (ret < 0) > -        goto cleanup; > +    if (virCommandRun(cmd, NULL) < 0) > +        return -1; > > -    if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0) > -        goto cleanup; > +    if (virStorageBackendSheepdogParseVdiList(vol, output) < 0) > +        return -1; > >     vol->type = VIR_STORAGE_VOL_NETWORK; > >     VIR_FREE(vol->key); >     if (virAsprintf(&vol->key, "%s/%s", >                     def->source.name, vol->name) < 0) > -        goto cleanup; > +        return -1; Before, '0' from the virStorageBackendSheepdogParseVdiList would be returned. While correct, it would look better in a separate patch. > >     VIR_FREE(vol->target.path); >     ignore_value(VIR_STRDUP(vol->target.path, vol->name)); > - cleanup: > -    virCommandFree(cmd); > -    return ret; > + > +    return 0; > } > > To everything apart from virStorageBackendDiskBuildPool: Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano