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@xxxxxxxxxx>
[..]
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
