Re: [libvirt] [PATCH 09/15] storage: Use VIR_AUTOFREE for storage util

7 Feb 2019


      On Wed, Feb 06, 2019 at 08:41:41AM -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@redhat.com>
---
[snip]
...
@@ -3804,7 +3713,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     VIR_AUTOPTR(virStorageVolDef) vol = NULL;
-    char *devpath = NULL;
+    VIR_AUTOFREE(char *) devpath = NULL;
     int retval = -1;
/* Check if the pool is using a stable target path. The call to
@@ -3820,11 +3729,11 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
         virReportError(VIR_ERR_INVALID_ARG,
                        _("unable to use target path '%s' for dev '%s'"),
                        NULLSTR(def->target.path), dev);
-        goto cleanup;
+        return -1;
     }
if (VIR_ALLOC(vol) < 0)
-        goto cleanup;
+        return -1;
vol->type = VIR_STORAGE_VOL_BLOCK;
@@ -3834,10 +3743,10 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
      * just leave 'host' out
      */
     if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0)
-        goto cleanup;
+        return -1;
if (virAsprintf(&devpath, "/dev/%s", dev) < 0)
-        goto cleanup;
+        return -1;
VIR_DEBUG("Trying to create volume for '%s'", devpath);
@@ -3850,7 +3759,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
     if ((vol->target.path = virStorageBackendStablePath(pool,
                                                         devpath,
                                                         true)) == NULL)
-        goto cleanup;
+        return -1;
if (STREQ(devpath, vol->target.path) &&
         !(STREQ(def->target.path, "/dev") ||
@@ -3859,34 +3768,29 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
         VIR_DEBUG("No stable path found for '%s' in '%s'",
                   devpath, def->target.path);
-        retval = -2;
-        goto cleanup;
+        return -2;
     }
/* Allow a volume read failure to ignore or skip this block file */
     if ((retval = virStorageBackendUpdateVolInfo(vol, true,
                                                  VIR_STORAGE_VOL_OPEN_DEFAULT,
                                                  VIR_STORAGE_VOL_READ_NOERROR)) < 0)
-        goto cleanup;
+        return retval;
vol->key = virStorageBackendSCSISerial(vol->target.path,
                                            (def->source.adapter.type ==
                                             VIR_STORAGE_ADAPTER_TYPE_FC_HOST));
     if (!vol->key)
-        goto cleanup;
We're changing the logic ^here - previously if virStorageBackendUpdateVolInfo
succeeded but virStorageBackendSCSISerial returned NULL, we'd still return
retval which would have been equal to 0. To me, your change feels right, but I
want to make sure no caller was relying on 0 even though
virStorageBackendSCSISerial might have failed.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

    