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&gt; [..] > 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