[PATCH] node_device: pacify grumpy coverity due to addr override

[Boris Fiuczynski]

On 12/11/20 3:55 PM, Michal Privoznik wrote:
> On 12/11/20 3:46 PM, Boris Fiuczynski wrote:
>> On 12/11/20 3:33 PM, Michal Privoznik wrote:
>>> On 12/10/20 6:32 PM, Boris Fiuczynski wrote:
>>>> With commit 09364608b4 node_device: refactor address retrieval of 
>>>> node device
>>>> "if-else if" was replaced by "switch".
>>>> The contained break statement now is no longer in context of the for 
>>>> loop
>>>> but instead of the switch causing the legitimate grumpiness of 
>>>> coverity.
>>>>
>>>> Signed-off-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>>>> Suggested-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>   src/node_device/node_device_driver.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/node_device/node_device_driver.c 
>>>> b/src/node_device/node_device_driver.c
>>>> index e254b49244..da1bc8a545 100644
>>>> --- a/src/node_device/node_device_driver.c
>>>> +++ b/src/node_device/node_device_driver.c
>>>> @@ -637,7 +637,7 @@ nodeDeviceFindAddressByName(const char *name)
>>>>       }
>>>>       def = virNodeDeviceObjGetDef(dev);
>>>> -    for (caps = def->caps; caps != NULL; caps = caps->next) {
>>>> +    for (caps = def->caps; caps != NULL && addr == NULL; caps = 
>>>> caps->next) {
>>>>           switch (caps->data.type) {
>>>>           case VIR_NODE_DEV_CAP_PCI_DEV: {
>>>>               virPCIDeviceAddress pci_addr = {
>>>>
>>>
>>> Yep, this is a genuine bug. Because those 'break' statements will end 
>>> the switch() and not the for() loop. But what I worry is that this 
>>> compound check is easy to overlook. So how about explicit:
>>>
>>>    if (addr)
>>>        break;
>>>
>>> at the end of the loop, after the switch()?
>>>
>>> Either way:
>>>
>>> Reviewed-by: Michal Privoznik <mprivozn at redhat.com>
>>>
>>> Michal
>>>
>>
>> Yes, agree that would make it more obvious.
>> Like this...
>>
>> diff --git a/src/node_device/node_device_driver.c 
>> b/src/node_device/node_device_driver.c
>> index e254b49244..51b20848ce 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
>> @@ -687,6 +687,9 @@ nodeDeviceFindAddressByName(const char *name)
>>           case VIR_NODE_DEV_CAP_LAST:
>>               break;
>>           }
>> +
>> +        if (addr)
>> +            break;
>>       }
>>
>>       virNodeDeviceObjEndAPI(&dev);
> 
> Exactly, this is exactly what I meant.
> 
>>
>>
>> If you want me to resend the patch please let me know.
>>
>>
> 
> No need. The change is trivial and since we have an agreement I'd say do 
> the change locally, append my Reviewed-by line and push.
> 
> Michal
> 
Michal,
since I do not have commit rights I am fine with you doing it.
Thanks.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294