Re: [PATCH 5/5] staging: wilc1000: use id value as argument

[Johnny Kim <johnny.kim@atmel.com>]

Hello Dan.

On 2015년 08월 13일 23:49, Dan Carpenter wrote:
> On Thu, Aug 13, 2015 at 01:41:23PM +0900, Tony Cho wrote:
>> +static u32 get_id_from_handler(tstrWILC_WFIDrv *handler)
>> +{
>> +	u32 id;
>> +
>> +	if (!handler)
>> +		return 0;
>> +
>> +	for (id = 0; id < NUM_CONCURRENT_IFC; id++) {
>> +		if (wfidrv_list[id] == handler) {
>> +			id += 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (id > NUM_CONCURRENT_IFC)
>> +		return 0;
>> +	else
>> +		return id;
>> +}
>> +
> This still has an off by one bug.  Just use zero offset arrays
> throughout.
>
> static int get_id_from_handler(tstrWILC_WFIDrv *handler)
> {
> 	int id;
>
> 	if (!handler)
> 		return -ENOBUFS;
>
> 	for (id = 0; id < NUM_CONCURRENT_IFC; id++) {
> 		if (wfidrv_list[id] == handler)
> 			return id;
> 	}
>
> 	return -ENOBUFS;
> }
Thanks for your review. The return value of this function has from 0 till 2.
1 and 2 value is real ID value. only 0 value is reserved to remove a 
registered id.
But I also think that error handling should be added about the 
overflowed value
as your opinion.
>> +static tstrWILC_WFIDrv *get_handler_from_id(u32 id)
>> +{
>> +	if (id > 0 && id <= NUM_CONCURRENT_IFC)
>> +		return wfidrv_list[id - 1];
>> +	else
>> +		return NULL;
>> +}
> static tstrWILC_WFIDrv *get_handler_from_id(int id)
> {
> 	if (id < 0 || id >= NUM_CONCURRENT_IFC)
> 		return NULL;
> 	return wfidrv_list[id];
> }
>
> regards,
> dan carpenter
>

Regards.
Johnny Kim.