From c4ff1a1825b3f58b2020972d618c161a528085bf Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Fri, 12 May 2017 11:08:57 -0400 Subject: [PATCH] test: Adjust cleanup/error paths for nodedev test APIs - In testDestroyVport rather than use a cleanup label, just return -1 immediately since nothing else is needed. - In testStoragePoolDestroy, if !privpool, then just return -1 since nothing else will happen anyway. - 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 --- src/test/test_driver.c | 78 ++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e17c86a1e5..bd24e5ba9d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4550,7 +4550,6 @@ testDestroyVport(testDriverPtr privconn, const char *wwnn ATTRIBUTE_UNUSED, const char *wwpn ATTRIBUTE_UNUSED) { - int ret = -1; virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL; @@ -4564,7 +4563,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", @@ -4573,15 +4572,9 @@ testDestroyVport(testDriverPtr privconn, virNodeDeviceObjRemove(&privconn->devs, obj); virNodeDeviceObjFree(obj); - obj = NULL; - ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); testObjectEventQueue(privconn, event); - return ret; + return 0; } @@ -4594,7 +4587,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) virObjectEventPtr event = NULL; if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name))) - goto cleanup; + return -1; if (!virStoragePoolObjIsActive(privpool)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -5369,7 +5362,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) virNodeDevicePtr ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if ((ret = virGetNodeDevice(conn, name))) { @@ -5379,9 +5372,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name) } } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5396,13 +5387,11 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, virCheckFlags(0, NULL); if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5415,7 +5404,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) char *ret = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return NULL; def = virNodeDeviceObjGetDef(obj); if (def->parent) { @@ -5425,9 +5414,7 @@ testNodeDeviceGetParent(virNodeDevicePtr dev) "%s", _("no parent for this device")); } - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); + virNodeDeviceObjUnlock(obj); return ret; } @@ -5440,20 +5427,16 @@ testNodeDeviceNumOfCaps(virNodeDevicePtr dev) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = def->caps; caps; caps = caps->next) ++ncaps; - ret = ncaps; - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj); - return ret; + virNodeDeviceObjUnlock(obj); + return ncaps; } @@ -5465,27 +5448,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames) virNodeDeviceDefPtr def; virNodeDevCapsDefPtr caps; int ncaps = 0; - int ret = -1; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto cleanup; + return -1; def = virNodeDeviceObjGetDef(obj); for (caps = 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; } @@ -5639,14 +5621,14 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) - goto out; + return -1; def = virNodeDeviceObjGetDef(obj); if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) - goto out; + goto cleanup; if (VIR_STRDUP(parent_name, 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 @@ -5659,7 +5641,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceObjGetParentHost(&driver->devs, def, EXISTING_DEVICE) < 0) { obj = NULL; - goto out; + goto cleanup; } event = virNodeDeviceEventLifecycleNew(dev->name, @@ -5671,7 +5653,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjFree(obj); obj = NULL; - out: + cleanup: if (obj) virNodeDeviceObjUnlock(obj); testObjectEventQueue(driver, event);