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