Re: [libvirt] [PATCH 07/15] storage: Use VIR_AUTOFREE for storage backends

7 Feb 2019

On Wed, Feb 06, 2019 at 08:41:39AM -0500, John Ferlan wrote:
...
Let's make use of the auto __cleanup capabilities. This also allows
for the cleanup of some goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/storage/storage_backend.c              |  9 +--
 src/storage/storage_backend_disk.c         | 62 +++++++-----------
 src/storage/storage_backend_fs.c           | 17 ++---
 src/storage/storage_backend_gluster.c      | 30 ++++-----
 src/storage/storage_backend_iscsi.c        | 73 +++++++---------------
 src/storage/storage_backend_iscsi_direct.c | 37 ++++-------
 src/storage/storage_backend_logical.c      | 35 ++++-------
 src/storage/storage_backend_mpath.c        | 18 +++---
 src/storage/storage_backend_rbd.c          | 35 ++++-------
 src/storage/storage_backend_scsi.c         | 63 +++++++------------
 src/storage/storage_backend_sheepdog.c     | 27 +++-----
 src/storage/storage_backend_vstorage.c     | 25 +++-----
 src/storage/storage_backend_zfs.c          | 15 ++---
 src/storage/storage_file_gluster.c         | 16 ++---
 14 files changed, 153 insertions(+), 309 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index a54c338cf0..5c8275e978 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -87,8 +87,7 @@ virStorageDriverLoadBackendModule(const char *name,
                                   const char *regfunc,
                                   bool forceload)
 {
-    char *modfile = NULL;
-    int ret;
+    VIR_AUTOFREE(char *) modfile = NULL;
if (!(modfile = virFileFindResourceFull(name,
                                             "libvirt_storage_backend_",
@@ -98,11 +97,7 @@ virStorageDriverLoadBackendModule(const char *name,
                                             "LIBVIRT_STORAGE_BACKEND_DIR")))
         return -1;
-    ret = virModuleLoad(modfile, regfunc, forceload);
-
-    VIR_FREE(modfile);
-
-    return ret;
+    return virModuleLoad(modfile, regfunc, forceload);
 }
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 230cf44b97..f2f56ee3de 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -56,7 +56,8 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
                                  virStorageVolDefPtr vol)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *tmp, *devpath, *partname;
+    char *tmp, *partname;
+    VIR_AUTOFREE(char *) devpath = NULL;
     bool addVol = false;
/* Prepended path will be same for all partitions, so we can
@@ -89,7 +90,6 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
          * way of doing this...
          */
         vol->target.path = virStorageBackendStablePath(pool, devpath, true);
-        VIR_FREE(devpath);
         if (vol->target.path == NULL)
             goto error;
     }
@@ -355,13 +355,12 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
      */
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *parthelper_path;
+    VIR_AUTOFREE(char *) parthelper_path = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
     struct virStorageBackendDiskPoolVolData cbdata = {
         .pool = pool,
         .vol = vol,
     };
-    int ret;
if (!(parthelper_path = virFileFindResource("libvirt_parthelper",
                                                 abs_topbuilddir "/src",
@@ -388,12 +387,7 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
         def->allocation = 0;
     def->capacity = def->available = 0;
-    ret = virCommandRunNul(cmd,
-                           6,
-                           virStorageBackendDiskMakeVol,
-                           &cbdata);
-    VIR_FREE(parthelper_path);
-    return ret;
+    return virCommandRunNul(cmd, 6, virStorageBackendDiskMakeVol, &cbdata);
 }
static int
@@ -419,9 +413,8 @@ static int
 virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *parthelper_path;
+    VIR_AUTOFREE(char *) parthelper_path = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
-    int ret;
if (!(parthelper_path = virFileFindResource("libvirt_parthelper",
                                                 abs_topbuilddir "/src",
@@ -433,12 +426,8 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
                                "-g",
                                NULL);
-    ret = virCommandRunNul(cmd,
-                           3,
-                           virStorageBackendDiskMakePoolGeometry,
-                           pool);
-    VIR_FREE(parthelper_path);
-    return ret;
+    return virCommandRunNul(cmd, 3, virStorageBackendDiskMakePoolGeometry,
+                            pool);
 }
static int
@@ -769,14 +758,13 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
                                unsigned int flags)
 {
     char *part_num = NULL;
-    char *devpath = NULL;
     char *dev_name;
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     char *src_path = def->source.devices[0].path;
     char *srcname = last_component(src_path);
+    VIR_AUTOFREE(char *) devpath = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
     bool isDevMapperDevice;
-    int rc = -1;
virCheckFlags(0, -1);
@@ -799,7 +787,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
             virReportSystemError(errno,
                                  _("Couldn't read volume target path '%s'"),
                                  vol->target.path);
-            goto cleanup;
+            return -1;
         }
         dev_name = last_component(devpath);
     }
@@ -810,7 +798,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Volume path '%s' did not start with parent "
                          "pool source device name."), dev_name);
-        goto cleanup;
+        return -1;
     }
part_num = dev_name + strlen(srcname);
@@ -824,7 +812,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("cannot parse partition number from target "
                          "'%s'"), dev_name);
-        goto cleanup;
+        return -1;
     }
/* eg parted /dev/sda rm 2 or /dev/mapper/mpathc rm 2 */
@@ -835,7 +823,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
                                part_num,
                                NULL);
     if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
+        return -1;
/* Refreshing the pool is the easiest option as LOGICAL and EXTENDED
      * partition allocation/capacity management is handled within
@@ -844,12 +832,9 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
      */
     virStoragePoolObjClearVols(pool);
     if (virStorageBackendDiskRefreshPool(pool) < 0)
-        goto cleanup;
+        return -1;
-    rc = 0;
- cleanup:
-    VIR_FREE(devpath);
-    return rc;
+    return 0;
 }
@@ -857,11 +842,10 @@ static int
 virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
                                virStorageVolDefPtr vol)
 {
-    int res = -1;
-    char *partFormat = NULL;
     unsigned long long startOffset = 0, endOffset = 0;
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     virErrorPtr save_err;
+    VIR_AUTOFREE(char *)partFormat = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(PARTED,
@@ -874,11 +858,11 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
         vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("storage pool only supports LUKS encrypted volumes"));
-        goto cleanup;
+        return -1;
     }
if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
-        goto cleanup;
+        return -1;
     virCommandAddArg(cmd, partFormat);
/* If we're going to encrypt using LUKS, then we could need up to
@@ -888,13 +872,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset,
                                             vol->target.capacity) < 0)
-        goto cleanup;
+        return -1;
virCommandAddArgFormat(cmd, "%lluB", startOffset);
     virCommandAddArgFormat(cmd, "%lluB", endOffset);
if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
+        return -1;
/* wait for device node to show up */
     virWaitForDevices();
@@ -918,11 +902,7 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
             goto error;
     }
-    res = 0;
-
- cleanup:
-    VIR_FREE(partFormat);
-    return res;
+    return 0;
error:
     /* Best effort to remove the partition. Ignore any errors
@@ -932,7 +912,7 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
     ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
     virSetError(save_err);
     virFreeError(save_err);
-    goto cleanup;
+    return -1;
 }
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 7d05ceeeb8..41d010dea0 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -246,7 +246,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool)
 {
     int ret = -1;
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *src = NULL;
+    VIR_AUTOFREE(char *) src = NULL;
     FILE *mtab;
     struct mntent ent;
     char buf[1024];
@@ -282,7 +282,6 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool)
cleanup:
     VIR_FORCE_FCLOSE(mtab);
-    VIR_FREE(src);
     return ret;
 }
@@ -299,9 +298,8 @@ static int
 virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *src = NULL;
+    VIR_AUTOFREE(char *) src = NULL;
     VIR_AUTOPTR(virCommand) cmd = NULL;
-    int ret = -1;
     int rc;
if (virStorageBackendFileSystemIsValid(pool) < 0)
@@ -320,13 +318,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
         return -1;
cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src);
-    if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
-
-    ret = 0;
- cleanup:
-    VIR_FREE(src);
-    return ret;
+    return virCommandRun(cmd, NULL);
 }
@@ -579,7 +571,7 @@ virStoragePoolDefFSNamespaceParse(xmlXPathContextPtr ctxt,
                                   void **data)
 {
     virStoragePoolFSMountOptionsDefPtr cmdopts = NULL;
-    xmlNodePtr *nodes = NULL;
+    VIR_AUTOFREE(xmlNodePtr *)nodes = NULL;
     int nnodes;
     size_t i;
     int ret = -1;
@@ -617,7 +609,6 @@ virStoragePoolDefFSNamespaceParse(xmlXPathContextPtr ctxt,
     ret = 0;
cleanup:
-    VIR_FREE(nodes);
     virStoragePoolDefFSNamespaceFree(cmdopts);
     return ret;
 }
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
index cb9f7e4735..0fe548f7e0 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -127,11 +127,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
     if (glfs_set_volfile_server(ret->vol, "tcp",
                                 ret->uri->server, ret->uri->port) < 0 ||
         glfs_init(ret->vol) < 0) {
-        char *uri = virURIFormat(ret->uri);
-
-        virReportSystemError(errno, _("failed to connect to %s"),
-                             NULLSTR(uri));
-        VIR_FREE(uri);
+        VIR_AUTOFREE(char *) uri = NULL;
+        uri = virURIFormat(ret->uri);
+        virReportSystemError(errno, _("failed to connect to %s"), NULLSTR(uri));
         goto error;
     }
@@ -187,8 +185,7 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state,
                                     virStorageVolDefPtr vol,
                                     const char *name)
 {
-    int ret = -1;
-    char *path = NULL;
+    VIR_AUTOFREE(char *) path = NULL;
     char *tmp;
VIR_FREE(vol->key);
@@ -200,35 +197,31 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state,
     if (name) {
         VIR_FREE(vol->name);
         if (VIR_STRDUP(vol->name, name) < 0)
-            goto cleanup;
+            return -1;
     }
if (virAsprintf(&path, "%s%s%s", state->volname, state->dir,
                     vol->name) < 0)
-        goto cleanup;
+        return -1;
tmp = state->uri->path;
     if (virAsprintf(&state->uri->path, "/%s", path) < 0) {
         state->uri->path = tmp;
-        goto cleanup;
+        return -1;
     }
     if (!(vol->target.path = virURIFormat(state->uri))) {
         VIR_FREE(state->uri->path);
         state->uri->path = tmp;
-        goto cleanup;
+        return -1;
     }
     VIR_FREE(state->uri->path);
     state->uri->path = tmp;
/* the path is unique enough to serve as a volume key */
     if (VIR_STRDUP(vol->key, vol->target.path) < 0)
-        goto cleanup;
-
-    ret = 0;
+        return -1;
- cleanup:
-    VIR_FREE(path);
-    return ret;
+    return 0;
 }
@@ -243,9 +236,9 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
 {
     int ret = -1;
     VIR_AUTOPTR(virStorageVolDef) vol = NULL;
+    VIR_AUTOFREE(char *) header = NULL;
     glfs_fd_t *fd = NULL;
     virStorageSourcePtr meta = NULL;
-    char *header = NULL;
     ssize_t len;
     int backingFormat;
@@ -333,7 +326,6 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
     virStorageSourceFree(meta);
     if (fd)
         glfs_close(fd);
-    VIR_FREE(header);
     return ret;
 }
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 483ba15102..41fa5e128d 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -130,27 +130,20 @@ static int
 virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool,
                               const char *session)
 {
-    char *sysfs_path;
-    int retval = -1;
+    VIR_AUTOFREE(char *) sysfs_path = NULL;
     uint32_t host;
if (virAsprintf(&sysfs_path,
                     "/sys/class/iscsi_session/session%s/device", session) < 0)
-        goto cleanup;
+        return -1;
if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0)
-        goto cleanup;
+        return -1;
if (virStorageBackendSCSIFindLUs(pool, host) < 0)
-        goto cleanup;
-
-    retval = 0;
-
- cleanup:
-
-    VIR_FREE(sysfs_path);
+        return -1;
-    return retval;
+    return 0;
 }
@@ -159,6 +152,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
                                       unsigned int flags)
 {
     VIR_AUTOPTR(virStoragePoolSource) source = NULL;
+    VIR_AUTOFREE(char *) portal = NULL;
     size_t ntargets = 0;
     char **targets = NULL;
     char *ret = NULL;
@@ -168,7 +162,6 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
         .nsources = 0,
         .sources = NULL
     };
-    char *portal = NULL;
virCheckFlags(0, NULL);
@@ -223,10 +216,10 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
         }
         VIR_FREE(list.sources);
     }
+    /* NB: Not virString -like, managed be VIR_APPEND_ELEMENT */
     for (i = 0; i < ntargets; i++)
         VIR_FREE(targets[i]);
     VIR_FREE(targets);
-    VIR_FREE(portal);
     return ret;
 }
@@ -235,7 +228,7 @@ virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool,
                                 bool *isActive)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *session = NULL;
+    VIR_AUTOFREE(char *) session = NULL;
     int ret = -1;
*isActive = false;
@@ -259,10 +252,8 @@ virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool,
         return -1;
     }
-    if ((session = virStorageBackendISCSISession(pool, true)) != NULL) {
+    if ((session = virStorageBackendISCSISession(pool, true)))
         *isActive = true;
-        VIR_FREE(session);
-    }
     ret = 0;
return ret;
@@ -330,9 +321,8 @@ static int
 virStorageBackendISCSIStartPool(virStoragePoolObjPtr pool)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *portal = NULL;
-    char *session = NULL;
-    int ret = -1;
+    VIR_AUTOFREE(char *) portal = NULL;
+    VIR_AUTOFREE(char *) session = NULL;
if (def->source.nhost != 1) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -355,50 +345,40 @@ virStorageBackendISCSIStartPool(virStoragePoolObjPtr pool)
if ((session = virStorageBackendISCSISession(pool, true)) == NULL) {
         if ((portal = virStorageBackendISCSIPortal(&def->source)) == NULL)
-            goto cleanup;
+            return -1;
/* Create a static node record for the IQN target. Must be done
          * in order for login to the target */
         if (virISCSINodeNew(portal, def->source.devices[0].path) < 0)
-            goto cleanup;
+            return -1;
if (virStorageBackendISCSISetAuth(portal, &def->source) < 0)
-            goto cleanup;
+            return -1;
if (virISCSIConnectionLogin(portal,
                                     def->source.initiator.iqn,
                                     def->source.devices[0].path) < 0)
-            goto cleanup;
+            return -1;
     }
-    ret = 0;
-
- cleanup:
-    VIR_FREE(portal);
-    VIR_FREE(session);
-    return ret;
+    return 0;
 }
static int
 virStorageBackendISCSIRefreshPool(virStoragePoolObjPtr pool)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *session = NULL;
+    VIR_AUTOFREE(char *) session = NULL;
def->allocation = def->capacity = def->available = 0;
if ((session = virStorageBackendISCSISession(pool, false)) == NULL)
-        goto cleanup;
+        return -1;
     if (virISCSIRescanLUNs(session) < 0)
-        goto cleanup;
+        return -1;
     if (virStorageBackendISCSIFindLUs(pool, session) < 0)
-        goto cleanup;
-    VIR_FREE(session);
+        return -1;
return 0;
-
- cleanup:
-    VIR_FREE(session);
-    return -1;
 }
@@ -406,13 +386,11 @@ static int
 virStorageBackendISCSIStopPool(virStoragePoolObjPtr pool)
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-    char *portal;
-    char *session;
-    int ret = -1;
+    VIR_AUTOFREE(char *) portal = NULL;
+    VIR_AUTOFREE(char *) session = NULL;
if ((session = virStorageBackendISCSISession(pool, true)) == NULL)
         return 0;
-    VIR_FREE(session);
if ((portal = virStorageBackendISCSIPortal(&def->source)) == NULL)
         return -1;
@@ -420,12 +398,9 @@ virStorageBackendISCSIStopPool(virStoragePoolObjPtr pool)
     if (virISCSIConnectionLogout(portal,
                                  def->source.initiator.iqn,
                                  def->source.devices[0].path) < 0)
-        goto cleanup;
-    ret = 0;
+        return -1;
- cleanup:
-    VIR_FREE(portal);
-    return ret;
+    return 0;
 }
virStorageBackend virStorageBackendISCSI = {
diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
index 82fa4d7a25..6458b0f835 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -421,15 +421,14 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
     }
for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
-        char *target = NULL;
+        VIR_AUTOFREE(char *) target = NULL;
if (VIR_STRDUP(target, tmp_addr->target_name) < 0)
             goto cleanup;
-        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) {
-            VIR_FREE(target);
+        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
             goto cleanup;
-        }
+        target = NULL;
VIR_APPEND_ELEMENT clears the source, so ^this should not be needed.

[snip]
...
-    dev->path = pvname;
+    VIR_STEAL_PTR(dev->path, pvname);
This VIR_STEAL_PTR stuff should come separe as mentioned in previous reviews.

The rest looks fine:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

    