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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
