[libvirt] [PATCH 01/13] test: Adjust cleanup/error paths for nodedev test APIs

[John Ferlan]

 - Rather than "goto cleanup;" on failure to virNodeDeviceObjFindByName
   an @obj, just return directly.  This then allows the cleanup: label code
   to not have to check "if (obj)" before calling virNodeDeviceObjUnlock.
   This also simplifies some exit logic...

 - In testNodeDeviceObjFindByName use an error: label to handle the failure
   and don't do the ncaps++ within the VIR_STRDUP() source target index.
   Only increment ncaps after success. Easier on eyes at error label too.

 - In testNodeDeviceDestroy use "cleanup" rather than "out" for the goto

Signed-off-by: John Ferlan <jferlan at redhat.com>
---
 src/test/test_driver.c | 75 +++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2db3f7d..3389edd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn,
                  const char *wwnn ATTRIBUTE_UNUSED,
                  const char *wwpn ATTRIBUTE_UNUSED)
 {
-    int ret = -1;
     virNodeDeviceObjPtr obj = NULL;
     virObjectEventPtr event = NULL;
 
@@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn,
     if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) {
         virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
                        _("no node device with matching name 'scsi_host12'"));
-        goto cleanup;
+        return -1;
     }
 
     event = virNodeDeviceEventLifecycleNew("scsi_host12",
@@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
 
     virNodeDeviceObjRemove(&privconn->devs, &obj);
 
-    ret = 0;
-
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
     testObjectEventQueue(privconn, event);
-    return ret;
+    return 0;
 }
 
 
@@ -5328,16 +5322,14 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
     virNodeDevicePtr ret = NULL;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, name)))
-        goto cleanup;
+        return NULL;
 
     if ((ret = virGetNodeDevice(conn, name))) {
         if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
             virObjectUnref(ret);
     }
 
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
+    virNodeDeviceObjUnlock(obj);
     return ret;
 }
 
@@ -5352,13 +5344,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
     virCheckFlags(0, NULL);
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-        goto cleanup;
+        return NULL;
 
     ret = virNodeDeviceDefFormat(obj->def);
 
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
+    virNodeDeviceObjUnlock(obj);
     return ret;
 }
 
@@ -5370,7 +5360,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
     char *ret = NULL;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-        goto cleanup;
+        return NULL;
 
     if (obj->def->parent) {
         ignore_value(VIR_STRDUP(ret, obj->def->parent));
@@ -5379,9 +5369,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev)
                        "%s", _("no parent for this device"));
     }
 
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
+    virNodeDeviceObjUnlock(obj);
     return ret;
 }
 
@@ -5393,19 +5381,15 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev)
     virNodeDeviceObjPtr obj;
     virNodeDevCapsDefPtr caps;
     int ncaps = 0;
-    int ret = -1;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-        goto cleanup;
+        return -1;
 
     for (caps = obj->def->caps; caps; caps = caps->next)
         ++ncaps;
-    ret = ncaps;
 
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
-    return ret;
+    virNodeDeviceObjUnlock(obj);
+    return ncaps;
 }
 
 
@@ -5416,26 +5400,25 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
     virNodeDeviceObjPtr obj;
     virNodeDevCapsDefPtr caps;
     int ncaps = 0;
-    int ret = -1;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-        goto cleanup;
+        return -1;
 
     for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) {
-        if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0)
-            goto cleanup;
+        if (VIR_STRDUP(names[ncaps],
+                       virNodeDevCapTypeToString(caps->data.type)) < 0)
+            goto error;
+        ncaps++;
     }
-    ret = ncaps;
 
- cleanup:
-    if (obj)
-        virNodeDeviceObjUnlock(obj);
-    if (ret == -1) {
-        --ncaps;
-        while (--ncaps >= 0)
-            VIR_FREE(names[ncaps]);
-    }
-    return ret;
+    virNodeDeviceObjUnlock(obj);
+    return ncaps;
+
+ error:
+    while (--ncaps >= 0)
+        VIR_FREE(names[ncaps]);
+    virNodeDeviceObjUnlock(obj);
+    return -1;
 }
 
 
@@ -5584,13 +5567,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     virObjectEventPtr event = NULL;
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
-        goto out;
+        return -1;
 
     if (virNodeDeviceGetWWNs(obj->def, &wwnn, &wwpn) == -1)
-        goto out;
+        goto cleanup;
 
     if (VIR_STRDUP(parent_name, obj->def->parent) < 0)
-        goto out;
+        goto cleanup;
 
     /* virNodeDeviceGetParentHost will cause the device object's lock to be
      * taken, so we have to dup the parent's name and drop the lock
@@ -5603,7 +5586,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     if (virNodeDeviceObjGetParentHost(&driver->devs, obj->def,
                                       EXISTING_DEVICE) < 0) {
         obj = NULL;
-        goto out;
+        goto cleanup;
     }
 
     event = virNodeDeviceEventLifecycleNew(dev->name,
@@ -5613,7 +5596,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
     virNodeDeviceObjLock(obj);
     virNodeDeviceObjRemove(&driver->devs, &obj);
 
- out:
+ cleanup:
     if (obj)
         virNodeDeviceObjUnlock(obj);
     testObjectEventQueue(driver, event);
-- 
2.9.3