Re: [PATCH -mm] Introduce strtol_check_range()

[Satyam Sharma <satyam@infradead.org>]

Hi Alexey,


On Wed, 1 Aug 2007, Alexey Dobriyan wrote:

> On Tue, Jul 31, 2007 at 10:04:10PM +0530, Satyam Sharma wrote:
> > Callers (especially "store" functions for sysfs or configfs attributes)
> > that want to convert an input string to a number may often also want to
> > check for simple input sanity or allowable range. strtol10_check_range()
> > of netconsole does this, so extract it out into lib/vsprintf.c, make it
> > generic w.r.t. base, and export it to the rest of the kernel and modules.
> 
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -335,9 +307,11 @@ static ssize_t store_enabled(struct netconsole_target *nt,
> >  	int err;
> >  	long enabled;
> >  
> > -	enabled = strtol10_check_range(buf, 0, 1);
> > -	if (enabled < 0)
> > +	enabled = strtol_check_range(buf, 0, 1, 10);
> > +	if (enabled < 0) {
> > +		printk(KERN_ERR "netconsole: invalid input\n");
> >  		return enabled;
> > +	}
> 
> Please, copy strtonum() from BSD instead. Nobody needs another
> home-grown converter.

BSD's strtonum(3) is a detestful, horrible shame.

The strtol_check_range() I implemented here does _all_ that strtonum()
does, plus is generic w.r.t. base, and minus the tasteless "errstr"
argument.

Tell me, how does that "errstr" ever make sense? We _anyway_ return
errors (-EINVAL or -ERANGE) if any of those cases show up. And
_because_ we use negative numbers to return errors, we can't use this
function to convert negative inputs anyway ... an appropriate error
message can always be outputted by the caller itself. [ hence the
two WARN_ON's I added here ]

But yeah, considering this implementation is so similar to strtonum(3)
(minus the shortcomings, that is :-) we can probably rename it to
something like kstrtonum() ... and we should probably be returning
different errors for the two invalid conditions, yes.


Thanks,
Satyam