Re: [PATCH -mm] Introduce strtol_check_range()

[Alexey Dobriyan]

On Thu, Aug 02, 2007 at 05:16:59AM +0530, Satyam Sharma wrote:
> 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,

What you did with base argument is creating opportunity to fsckup,
namely, forgetting that base is last and putting it second.

> 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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/