[libvirt] [PATCH v2 01/14] test: Adjust cleanup/error paths for nodedev test APIs

[John Ferlan]

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 at 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
>