1. Patchew
  2. Libvirt
  3. View series

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

Boris Fiuczynski posted 1 patch 3 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201210173236.242991-1-fiuczy@linux.ibm.com
src/node_device/node_device_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Boris Fiuczynski 3 years, 4 months ago 
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@linux.ibm.com>
Suggested-by: John Ferlan <jferlan@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 = {
-- 
2.26.2
Re: [PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Michal Privoznik 3 years, 4 months ago 
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@linux.ibm.com>
> Suggested-by: John Ferlan <jferlan@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@redhat.com>

Michal
Re: [PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Boris Fiuczynski 3 years, 4 months ago 
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@linux.ibm.com>
>> Suggested-by: John Ferlan <jferlan@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@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);


If you want me to resend the patch please let me know.


-- 
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
Re: [PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Michal Privoznik 3 years, 4 months ago 
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@linux.ibm.com>
>>> Suggested-by: John Ferlan <jferlan@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@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
Re: [PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Boris Fiuczynski 3 years, 4 months ago 
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@linux.ibm.com>
>>>> Suggested-by: John Ferlan <jferlan@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@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
Re: [PATCH] node_device: pacify grumpy coverity due to addr override
Posted by Michal Privoznik 3 years, 4 months ago 
On 12/14/20 12:02 PM, Boris Fiuczynski wrote:

> Michal,
> since I do not have commit rights I am fine with you doing it.
> Thanks.
> 

Terribly sorry, for some reason I thought you have commit access.
Fixed and pushed.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.