Re: [libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs
On Sat, Jun 03, 2017 at 09:11:52AM -0400, John Ferlan wrote:
- 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(a)redhat.com>
[..]
event = virNodeDeviceEventLifecycleNew("scsi_host12",
@@ -4534,13 +4533,8 @@ testDestroyVport(testDriverPtr privconn,
virNodeDeviceObjFree(obj);
obj = NULL;
You can drop ^this then since you don't need to clear it anymore.
Just a minor nit, while I was checking where testDestroyVport gets called from
- testStoragePoolDestroy, there's a spot that could be fixed the same way.
Would you mind squashing this bit in as well?
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6422ba8fb..a397364c7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4547,7 +4547,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,
[...]
@@ -5424,27 +5408,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++;
How about placing the increment to the iteration_expression segment of the
loop, aka at the end where the usual increment happens before condition
evaluation. Wondering whether there's a general technical term for the syntax
part of the loop between the parentheses.
}
- ret = ncaps;
- cleanup:
- if (obj)
- virNodeDeviceObjUnlock(obj);
- if (ret == -1) {
- --ncaps;
- while (--ncaps >= 0)
- VIR_FREE(names[ncaps]);
Hmm, this was indeed an interesting bit of code.
- }
- return ret;
+ virNodeDeviceObjUnlock(obj);
+ return ncaps;
+
+ error:
+ while (--ncaps >= 0)
+ VIR_FREE(names[ncaps]);
+ virNodeDeviceObjUnlock(obj);
+ return -1;
}
ACK with adjustments.
Erik