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

[Michal Privoznik]

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