On 06/29/2017 08:57 AM, Erik Skultety wrote:
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.
True, I'll drop it especially since the reason it was there to avoid the virNodeDeviceObjUnlock call is no longer valid.
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;
I don't mind changing this as well - although I probably have that in another patch somewhere dealing with storage tests. Trying to keep nodedev with nodedev and storage with storage.
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.
Do you mean : (caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++) ?? If so, yuck. Since we're iterating on caps, I think what should be incremented is caps and not ncaps as well. Putting it after the VIR_STRDUP() just makes it clearer that we successfully added something. I'd prefer to keep as is. Tks - John
} - 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