Re: [PATCH libvirt v4 09/12] node_device: refactor address retrieval of node device

2020-12-10 Thread Boris Fiuczynski 

On 12/10/20 1:01 PM, John Ferlan wrote:

Coverity got pretty grumpy this morning with RESOURCE_LEAKS due to
override of @addr as changing from a break; inside a switch/case
statement sent us back to the for statement rather than outside the loop
which would occur on the break; inside the if {} else if {}

Suggestion... add "&& addr != NULL" as an additional for loop exit.

John


Thanks John.
I sent a patch with your suggested change slightly modified.

--
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 libvirt v4 09/12] node_device: refactor address retrieval of node device

2020-12-10 Thread John Ferlan 



On 12/3/20 12:59 PM, Shalini Chellathurai Saroja wrote:
> Use switch statements instead of if-else condition in the method
> nodeDeviceFindAddressByName to retrieve address of a node device.
> 
> Signed-off-by: Shalini Chellathurai Saroja 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/node_device/node_device_driver.c | 30 ++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/src/node_device/node_device_driver.c 
> b/src/node_device/node_device_driver.c
> index f5ea973c..65c647f5 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -638,7 +638,8 @@ nodeDeviceFindAddressByName(const char *name)
>  
>  def = virNodeDeviceObjGetDef(dev);
>  for (caps = def->caps; caps != NULL; caps = caps->next) {
> -if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> +switch (caps->data.type) {
> +case VIR_NODE_DEV_CAP_PCI_DEV: {

Coverity got pretty grumpy this morning with RESOURCE_LEAKS due to
override of @addr as changing from a break; inside a switch/case
statement sent us back to the for statement rather than outside the loop
which would occur on the break; inside the if {} else if {}

Suggestion... add "&& addr != NULL" as an additional for loop exit.

John

>  virPCIDeviceAddress pci_addr = {
>  .domain = caps->data.pci_dev.domain,
>  .bus = caps->data.pci_dev.bus,
> @@ -648,7 +649,9 @@ nodeDeviceFindAddressByName(const char *name)
>  
>  addr = virPCIDeviceAddressAsString(_addr);
>  break;
> -} else if (caps->data.type == VIR_NODE_DEV_CAP_CSS_DEV) {
> +}
> +
> +case VIR_NODE_DEV_CAP_CSS_DEV: {
>  virDomainDeviceCCWAddress ccw_addr = {
>  .cssid = caps->data.ccw_dev.cssid,
>  .ssid = caps->data.ccw_dev.ssid,
> @@ -657,6 +660,29 @@ nodeDeviceFindAddressByName(const char *name)
>  
>  addr = virDomainCCWAddressAsString(_addr);
>  break;
> +}
> +
> +case VIR_NODE_DEV_CAP_SYSTEM:
> +case VIR_NODE_DEV_CAP_USB_DEV:
> +case VIR_NODE_DEV_CAP_USB_INTERFACE:
> +case VIR_NODE_DEV_CAP_NET:
> +case VIR_NODE_DEV_CAP_SCSI_HOST:
> +case VIR_NODE_DEV_CAP_SCSI_TARGET:
> +case VIR_NODE_DEV_CAP_SCSI:
> +case VIR_NODE_DEV_CAP_STORAGE:
> +case VIR_NODE_DEV_CAP_FC_HOST:
> +case VIR_NODE_DEV_CAP_VPORTS:
> +case VIR_NODE_DEV_CAP_SCSI_GENERIC:
> +case VIR_NODE_DEV_CAP_DRM:
> +case VIR_NODE_DEV_CAP_MDEV_TYPES:
> +case VIR_NODE_DEV_CAP_MDEV:
> +case VIR_NODE_DEV_CAP_CCW_DEV:
> +case VIR_NODE_DEV_CAP_VDPA:
> +case VIR_NODE_DEV_CAP_AP_CARD:
> +case VIR_NODE_DEV_CAP_AP_QUEUE:
> +case VIR_NODE_DEV_CAP_AP_MATRIX:
> +case VIR_NODE_DEV_CAP_LAST:
> +break;
>  }
>  }
>  
>

[PATCH libvirt v4 09/12] node_device: refactor address retrieval of node device

2020-12-03 Thread Shalini Chellathurai Saroja 
Use switch statements instead of if-else condition in the method
nodeDeviceFindAddressByName to retrieve address of a node device.

Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
 src/node_device/node_device_driver.c | 30 ++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index f5ea973c..65c647f5 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -638,7 +638,8 @@ nodeDeviceFindAddressByName(const char *name)
 
 def = virNodeDeviceObjGetDef(dev);
 for (caps = def->caps; caps != NULL; caps = caps->next) {
-if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+switch (caps->data.type) {
+case VIR_NODE_DEV_CAP_PCI_DEV: {
 virPCIDeviceAddress pci_addr = {
 .domain = caps->data.pci_dev.domain,
 .bus = caps->data.pci_dev.bus,
@@ -648,7 +649,9 @@ nodeDeviceFindAddressByName(const char *name)
 
 addr = virPCIDeviceAddressAsString(_addr);
 break;
-} else if (caps->data.type == VIR_NODE_DEV_CAP_CSS_DEV) {
+}
+
+case VIR_NODE_DEV_CAP_CSS_DEV: {
 virDomainDeviceCCWAddress ccw_addr = {
 .cssid = caps->data.ccw_dev.cssid,
 .ssid = caps->data.ccw_dev.ssid,
@@ -657,6 +660,29 @@ nodeDeviceFindAddressByName(const char *name)
 
 addr = virDomainCCWAddressAsString(_addr);
 break;
+}
+
+case VIR_NODE_DEV_CAP_SYSTEM:
+case VIR_NODE_DEV_CAP_USB_DEV:
+case VIR_NODE_DEV_CAP_USB_INTERFACE:
+case VIR_NODE_DEV_CAP_NET:
+case VIR_NODE_DEV_CAP_SCSI_HOST:
+case VIR_NODE_DEV_CAP_SCSI_TARGET:
+case VIR_NODE_DEV_CAP_SCSI:
+case VIR_NODE_DEV_CAP_STORAGE:
+case VIR_NODE_DEV_CAP_FC_HOST:
+case VIR_NODE_DEV_CAP_VPORTS:
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+case VIR_NODE_DEV_CAP_DRM:
+case VIR_NODE_DEV_CAP_MDEV_TYPES:
+case VIR_NODE_DEV_CAP_MDEV:
+case VIR_NODE_DEV_CAP_CCW_DEV:
+case VIR_NODE_DEV_CAP_VDPA:
+case VIR_NODE_DEV_CAP_AP_CARD:
+case VIR_NODE_DEV_CAP_AP_QUEUE:
+case VIR_NODE_DEV_CAP_AP_MATRIX:
+case VIR_NODE_DEV_CAP_LAST:
+break;
 }
 }
 
-- 
2.26.2