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.
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
On Thu, 2 Aug 2007, 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.
> > [...]
> >
> > 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 ]
Glargh, lemme take that back :-p
Wait, it takes in an const char ** argument, and then the whole error
return checking is based on the (const char *) *arg, and not the
return value (-EINVAL, -ERANGE) itself. And it appears, we set the
const char **errstr from inside the function only if the caller didn't
explicitly give us a NULL itself ... hmm, it all does make sense, plus
allows to convert negative inputs as well -- now that the error return
checking in the callsite won't be based on the return value anyway
(but strtonum(3) is still a bit daft, in that it "pretends" to return
error values in ret even though the caller can't use that to test if
an error actually occurred).
[ Thankfully there won't be any BSD lovers around here, so probably I'll
be able to get out of this clean :-) So okay, I'll port it over ... ]
Thanks,
Satyam
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.
On 8/2/07, Alexey Dobriyan <[email protected]> wrote:
> > > 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.
Embedding base in function name (func10, func8, func16 [, func2])
will eliminate that possibility and also save one argument
push on stack.
You can always multiplex them locally:
static int func_generic(base...) {...}
int func10(...) { return func_generic(10, ....); }
int func8(...) { return func_generic(8, ....); }
You also can have a faster "static int func_power_of_2(base...)" for
2,8,16, etc.
--
vda
On Aug 2 2007 05:16, Satyam Sharma wrote:
>
>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.
errstr (well, at least for strtol) are useful to find the first character that
does not make up a number (and then do whatever the user wants to, including,
continuing to parse). For example "chown 0:1337", strtol on "0:1337" should
give errstr=pointer to the ":", then check for it being a ':', then you know
the next char is the GID. :)
Jan
--
Hi Jan,
On Fri, 3 Aug 2007, Jan Engelhardt wrote:
>
> On Aug 2 2007 05:16, Satyam Sharma wrote:
> >
> >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.
>
> errstr (well, at least for strtol) are useful to find the first character that
> does not make up a number (and then do whatever the user wants to, including,
> continuing to parse). For example "chown 0:1337", strtol on "0:1337" should
> give errstr=pointer to the ":", then check for it being a ':', then you know
> the next char is the GID. :)
We were actually discussing the "errstr" that's the fourth argument of
BSD's strtonum(3) which has quite radically different semantics [1]
from the "endptr" argument of strtol(3). Anyway, I originally
misunderstood the interface / the correct way to be using that function
as I later mentioned in the other mail -- also see the answer to point
#3 in [2]. Porting it over to here does sound like a good idea.
I'm off on vacation with little or intermittent (possibly none) access
to email for the next 10 days, so please feel free to beat me to it :-)
Satyam
[1] http://www.freebsd.org/cgi/man.cgi?query=strtonum&apropos=0&sektion=3&manpath=FreeBSD+7-current&format=html
[2] http://lists.freebsd.org/pipermail/freebsd-current/2005-April/048744.html
On Fri, Aug 03, 2007 at 02:29:06PM +0530, Satyam Sharma wrote:
> On Fri, 3 Aug 2007, Jan Engelhardt wrote:
>
> >
> > On Aug 2 2007 05:16, Satyam Sharma wrote:
> > >
> > >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.
> >
> > errstr (well, at least for strtol) are useful to find the first character that
> > does not make up a number (and then do whatever the user wants to, including,
> > continuing to parse). For example "chown 0:1337", strtol on "0:1337" should
> > give errstr=pointer to the ":", then check for it being a ':', then you know
> > the next char is the GID. :)
>
> We were actually discussing the "errstr" that's the fourth argument of
> BSD's strtonum(3) which has quite radically different semantics [1]
> from the "endptr" argument of strtol(3).
Glad you've noticed.
> Anyway, I originally
> misunderstood the interface / the correct way to be using that function
> as I later mentioned in the other mail -- also see the answer to point
> #3 in [2]. Porting it over to here does sound like a good idea.
> [1] http://www.freebsd.org/cgi/man.cgi?query=strtonum&apropos=0&sektion=3&manpath=FreeBSD+7-current&format=html
> [2] http://lists.freebsd.org/pipermail/freebsd-current/2005-April/048744.html
Actually, strtonum() rejects "42\n" which means we can't massively use
it. :-(