On 09/04/2008, Felipe Balbi <me@xxxxxxxxxxxxxxx> wrote:
> On Wed, Apr 09, 2008 at 07:26:31PM +0200, andrzej zaborowski wrote:
> [...]
>
> > > + time = strict_strtoul(buf, 10, &res);
> > > + /* Numbers only, please. */
> > > + if (buf && *buf != '\n' && *(buf + 1) != '\0')
> > > + return -EINVAL;
> >
> > The condition doesn't look correct, for example it rejects "55\n\0"
> > but accepts NULL.
>
>
> Well, this is not my driver and i just moved to strict_stroul cuz
> checkpatch asked me to. I'll take a closer look tomorrow but if you can
> come up with a better solution for this, I'll ack.
Turns out the result is returned in the buffer in third parameter, and
the checking is already "strict" (rejects trailing non-numbers), so
this should look something like:
ret = strict_strtoul(buf, 0, &time);
if (ret)
return ret;
>
> btw, good catch, i didn't had too much time to work on this but still
> the driver is working fine.
>
>
> > > + i2c_set_clientdata(client, lm);
> > > + lm->client = client;
> > > + lm8323_pdata = client->dev.platform_data;
> > > + if (!lm8323_pdata)
> > > + return -EINVAL; /* ? */
> >
> > Here lm remains allocated and is lost. Not that it bothers me, but
> > probably not what was intended.
>
>
> You mean the error handling?
Yep.
>
> if (!lm8323_pdata) {
> kfree(lm);
> return -EINVAL;
> }
>
> fixing tomorrow.
Thanks
--
Please do not print this email unless absolutely necessary. Spread
environmental awareness.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
