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&gt; > Suggested-by: John Ferlan <jferlan(a)redhat.com&gt; > --- >   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&gt; Michal

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&gt; >>> Suggested-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>>   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&gt; >> >> 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