On Mon, Aug 10, 2015 at 02:58:24PM +0900, Tony Cho wrote:
> +static u32 add_handler_in_list(tstrWILC_WFIDrv *handler)
I am suspicous of code which uses u32 for something where the hardware
doesn't specify unsigned 32 bits. Why not just return "int"?
> +{
> + u32 id;
It looks like we have a case of u32 disease. Prefer int over u32 unless
there is a reason for it.
> +
> + if (!handler)
> + return 0;
Don't add NULL checks unless it makes sense. How are we supposed to
recover from this if handler is NULL?
> +
> + for (id = 0; id < NUM_CONCURRENT_IFC; id++) {
> + if (!wfidrv_list[id]) {
> + wfidrv_list[id++] = handler;
Ugh... I don't like the ++ here. Just use a zero offset array or we
are going to have off by one bugs.
> + break;
> + }
> + }
> +
> + if (id > NUM_CONCURRENT_IFC)
> + return 0;
Well, that didn't take long, it's three lines later and we already have
hit our first off by one bug. This check can never be true. Of course,
no one checks the return value either...
> +
> + return id;
> +}
I guess I would be fine with this patch if you changed it to use a zero
offset array, ints instead of u32s and added some error handling.
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
