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@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