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

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Aug 18, 2015 at 12:10:53PM +0900, Johnny Kim wrote:
> 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.

I thought we had created "id" here in this patch so we don't have to
pass function pointers through a u32 value (which can't fit a 64 bit
pointer).  What do you mean it is a "real ID value"?  Is it there in
the hardware spec?

Anyway, this code is buggy and messy.  Please find a different way to
write it.

regards,
dan carpenter