On 05/26/2017 03:05 AM, Peter Krempa wrote:
On Thu, May 25, 2017 at 15:56:58 -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> --- src/test/test_driver.c | 75 +++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 46 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2db3f7d..3389edd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4512,7 +4512,6 @@ testDestroyVport(testDriverPtr privconn, const char *wwnn ATTRIBUTE_UNUSED, const char *wwpn ATTRIBUTE_UNUSED) { - int ret = -1; virNodeDeviceObjPtr obj = NULL; virObjectEventPtr event = NULL;
@@ -4526,7 +4525,7 @@ testDestroyVport(testDriverPtr privconn, if (!(obj = virNodeDeviceObjFindByName(&privconn->devs, "scsi_host12"))) { virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", _("no node device with matching name 'scsi_host12'")); - goto cleanup; + return -1; }
event = virNodeDeviceEventLifecycleNew("scsi_host12", @@ -4535,13 +4534,8 @@ testDestroyVport(testDriverPtr privconn,
virNodeDeviceObjRemove(&privconn->devs, &obj);
A static analyzer may argue that 'obj' may be non-null in some cases at this point ...
Not the one I've used... It seems it was smart enough to realize that virNodeDeviceObjRemove will unlock *obj and then lock/unlock *obj each time through it's loop and then set it to NULL if it matches something on list. So either we return with obj=NULL or an unlocked obj
- ret = 0; - - cleanup: - if (obj) - virNodeDeviceObjUnlock(obj);
So this would not unlock it.
Or did I miss something more subtle? When originally wrote the code I have to assume now I was paying less attention to what got called. Tks - John
testObjectEventQueue(privconn, event); - return ret; + return 0; }
ACK to the rest