Message-ID: <55D2A23D.2080209@atmel.com> (sfid-20150818_051139_724676_059F8BC4)
Date: Tue, 18 Aug 2015 12:10:53 +0900
From: Johnny Kim <johnny.kim@atmel.com>
MIME-Version: 1.0
To: Dan Carpenter <dan.carpenter@oracle.com>,
	Tony Cho <tony.cho@atmel.com>
CC: <gregkh@linuxfoundation.org>, <devel@driverdev.osuosl.org>,
	<rachel.kim@atmel.com>, <chris.park@atmel.com>,
	<linux-wireless@vger.kernel.org>, <jude.lee@atmel.com>,
	<leo.kim@atmel.com>
Subject: Re: [PATCH 5/5] staging: wilc1000: use id value as argument
References: <1439440883-16061-1-git-send-email-tony.cho@atmel.com> <1439440883-16061-6-git-send-email-tony.cho@atmel.com> <20150813144907.GA4484@mwanda>
In-Reply-To: <20150813144907.GA4484@mwanda>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Sender: linux-wireless-owner@vger.kernel.org

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.