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(a)linux.ibm.com>
>>> Suggested-by: John Ferlan <jferlan(a)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(a)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