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

Joe Eykholt jre at nuovasystems.com
Tue Jan 20 20:36:05 UTC 2009


Ma, Steve wrote:
> 
>> -----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.

That doesn't make sense to me, because this is called from a function, sa_dir_read(),
that is not part of the HBA-API.  It doesn't check the return against
HBA_STATUS_OK, it checks it against 0.  That they happen to be the
same value doesn't matter.  In addition, sa_dir_read() returns whatever
non-zero value it gets, or an errno, so the non-zero returns should be
errno values as well, not HBA_STATUS values.

So, do you agree that the patch is OK as is?

>>        }
>>
>>        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;

I've gotten yelled at for doing unnecessary pp = NULL, although
it's a recommended defensive code practice where I work,
in kernel code that would be taboo.
 	
>>        }
>>
>>        /* 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

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




More information about the devel mailing list