Re: [libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs
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(a)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