[Open-FCoE] [PATCH] libhbalinux: fix error returns when scanning the fc_hosts dir.

Ma, Steve steve.ma at intel.com
Tue Jan 20 20:06:02 UTC 2009



>-----Original Message-----
>From: devel-bounces at open-fcoe.org [mailto:devel-bounces at open-fcoe.org] On
>Behalf Of Joe Eykholt
>Sent: Friday, January 16, 2009 5:48 PM
>To: devel at open-fcoe.org
>Subject: [Open-FCoE] [PATCH] libhbalinux: fix error returns when scanning
>the fc_hosts dir.
>
>libhbalinux: fix error returns when scanning the fc_hosts dir.
>
>sa_read_dir() expects zero on success, non-zero stops the directory read.
>We still want to look for other HBAs even if theres one
>we don't understand.
>
>Signed-off-by: Joe Eykholt <jeykholt at cisco.com>
>---
> src/lport.c |   22 ++++++++++++----------
> 1 files changed, 12 insertions(+), 10 deletions(-)
>
>
>diff --git a/src/lport.c b/src/lport.c
>index 9c47887..5701809 100644
>--- a/src/lport.c
>+++ b/src/lport.c
>@@ -142,7 +142,8 @@ sysfs_scan(struct dirent *dp, void *arg)
>                        "%s: malloc for local port %d failed,"
>                        " errno=0x%x\n", __func__,
>                        ap->ad_port_count - 1, errno);
>-               return HBA_STATUS_ERROR;
>+               free(ap);
>+               return 0;
I think "return HBA_STATUS_OK;" would be more consistent with the other part of the code.
>        }
>
>        memset(pp, 0, sizeof(*pp));
>@@ -260,7 +261,7 @@ sysfs_scan(struct dirent *dp, void *arg)
>                fprintf(stderr,
>                        "%s: insert of HBA %d port %d failed\n",
>                        __func__, ap->ad_kern_index, pp->ap_index);
>-               free(pp);
>+               goto skip;
Good goto! In skip: you have free(pp) again. That is dangerous.
I would say free(pp); pp=NULL;
	
>        }
>
>        /* Create adapter name */
>@@ -297,9 +298,7 @@ sysfs_scan(struct dirent *dp, void *arg)
>        sscanf(strrchr(buf, '/') + 1, "%x:%x:%x.%x",
>                &hba_info.domain, &hba_info.bus,
>                &hba_info.dev, &hba_info.func);
>-       rc = find_pci_device(&hba_info);
>-       if (rc != HBA_STATUS_OK)
>-               return rc;
>+       (void) find_pci_device(&hba_info);
>
>        /* Get Number of Ports */
>        atp->NumberOfPorts = hba_info.NumberOfPorts;
>@@ -354,10 +353,8 @@ sysfs_scan(struct dirent *dp, void *arg)
>        /* Get DriverName */
>        snprintf(drv_dir, sizeof(drv_dir), "%s" SYSFS_MODULE , hba_dir);
>        i = readlink(drv_dir, buf, sizeof(buf));
>-       if (i < 0) {
>-               printf("Fatal! readlink %s failed\n", drv_dir);
>-               return HBA_STATUS_ERROR;
>-       }
>+       if (i < 0)
>+               i = 0;
>        buf[i] = '\0';
>        if (!strstr(buf, "module")) {
>                /*
>@@ -381,7 +378,12 @@ sysfs_scan(struct dirent *dp, void *arg)
>                adapter_destroy(ap);      /* free adapter and ports */
>        }
>
>-       return HBA_STATUS_OK;
>+       return 0;
HBA_STATUS_OK is defined as 0 in hbaapi.h of hbaapi_src_2.2. We are using defines in hbaapi.h everywhere in the vendor library code. I wonder why you don't want to use those defines.
>+
>+skip:
>+       free(pp);
>+       free(ap);
Here, I would do
	if (pp)
		free(pp)
	if (ap)
		free (ap)
>+       return 0;
Again, I think "return HBA_STATUS_OK;" would be more consistent with the other part of the code.

> }
>
> void
>
>
>_______________________________________________
>devel mailing list
>devel at open-fcoe.org
>http://www.open-fcoe.org/mailman/listinfo/devel



More information about the devel mailing list