| Message ID | 20200626104442.GF117543@hirez.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | lib: Extend kstrtobool() to accept "true"/"false" | expand |
On Fri, 2020-06-26 at 12:44 +0200, Peter Zijlstra wrote: > On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote: > > > > This is too lax - it will be enabled for any !0 value. Please > > > accept > > > only 0 and 1. > > > > kstrtobool() ftw > > And looking at that, I find it really strange it does not in fact > accept > "true" / "false", so how about this? looks good. Thanks, Srinvas > > --- > Subject: lib: Extend kstrtobool() to accept "true"/"false" > > Extend the strings recognised by kstrtobool() to cover: > > - 1/0 > - y/n > - yes/no (new) > - t/f (new) > - true/false (new) > - on/off > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > lib/kstrtox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----- > ---------- > 1 file changed, 45 insertions(+), 15 deletions(-) > > diff --git a/lib/kstrtox.c b/lib/kstrtox.c > index 1006bf70bf74..b8b950325097 100644 > --- a/lib/kstrtox.c > +++ b/lib/kstrtox.c > @@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8); > * @s: input string > * @res: result > * > - * This routine returns 0 iff the first character is one of > 'Yy1Nn0', or > - * [oO][NnFf] for "on" and "off". Otherwise it will return > -EINVAL. Value > - * pointed to by res is updated upon finding a match. > + * This return return 0 on success, otherwise it will return > -EINVAL. > + * It will accept (case invariant): > + * > + * - 1/0 > + * - y/n > + * - yes/no > + * - t/f > + * - true/false > + * - on/off > + * > + * and set @*res to either true/false respectively. > */ > int kstrtobool(const char *s, bool *res) > { > @@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res) > return -EINVAL; > > switch (s[0]) { > + case 't': > + case 'T': > + if (!s[1] || !strcasecmp(s, "true")) > + goto have_true; > + > + break; > + > case 'y': > case 'Y': > + if (!s[1] || !strcasecmp(s, "yes")) > + goto have_true; > + > + break; > + > case '1': > +have_true: > *res = true; > return 0; > + > + case 'f': > + case 'F': > + if (!s[1] || !strcasecmp(s, "false")) > + goto have_false; > + > + break; > case 'n': > case 'N': > + if (!s[1] || !strcasecmp(s, "no")) > + goto have_false; > + > + break; > case '0': > +have_false: > *res = false; > return 0; > + > case 'o': > case 'O': > - switch (s[1]) { > - case 'n': > - case 'N': > - *res = true; > - return 0; > - case 'f': > - case 'F': > - *res = false; > - return 0; > - default: > - break; > - } > + if (!strcasecmp(s, "on")) > + goto have_true; > + > + if (!strcasecmp(s, "off")) > + goto have_false; > + > + break; > + > default: > break; > }
On Fri, Jun 26, 2020 at 12:45 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote: > > > > This is too lax - it will be enabled for any !0 value. Please accept > > > only 0 and 1. > > > > kstrtobool() ftw > > And looking at that, I find it really strange it does not in fact accept > "true" / "false", so how about this? > > --- > Subject: lib: Extend kstrtobool() to accept "true"/"false" > > Extend the strings recognised by kstrtobool() to cover: > > - 1/0 > - y/n > - yes/no (new) > - t/f (new) > - true/false (new) > - on/off > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > --- > lib/kstrtox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 45 insertions(+), 15 deletions(-) > > diff --git a/lib/kstrtox.c b/lib/kstrtox.c > index 1006bf70bf74..b8b950325097 100644 > --- a/lib/kstrtox.c > +++ b/lib/kstrtox.c > @@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8); > * @s: input string > * @res: result > * > - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or > - * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL. Value > - * pointed to by res is updated upon finding a match. > + * This return return 0 on success, otherwise it will return -EINVAL. > + * It will accept (case invariant): > + * > + * - 1/0 > + * - y/n > + * - yes/no > + * - t/f > + * - true/false > + * - on/off > + * > + * and set @*res to either true/false respectively. > */ > int kstrtobool(const char *s, bool *res) > { > @@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res) > return -EINVAL; > > switch (s[0]) { > + case 't': > + case 'T': > + if (!s[1] || !strcasecmp(s, "true")) > + goto have_true; > + > + break; > + > case 'y': > case 'Y': > + if (!s[1] || !strcasecmp(s, "yes")) > + goto have_true; > + > + break; > + > case '1': > +have_true: > *res = true; > return 0; > + > + case 'f': > + case 'F': > + if (!s[1] || !strcasecmp(s, "false")) > + goto have_false; > + > + break; > case 'n': > case 'N': > + if (!s[1] || !strcasecmp(s, "no")) > + goto have_false; > + > + break; > case '0': > +have_false: > *res = false; > return 0; > + > case 'o': > case 'O': > - switch (s[1]) { > - case 'n': > - case 'N': > - *res = true; > - return 0; > - case 'f': > - case 'F': > - *res = false; > - return 0; > - default: > - break; > - } > + if (!strcasecmp(s, "on")) > + goto have_true; > + > + if (!strcasecmp(s, "off")) > + goto have_false; > + > + break; > + > default: > break; > }
On Fri, Jun 26, 2020 at 12:44:42PM +0200, Peter Zijlstra wrote: > On Fri, Jun 26, 2020 at 12:22:55PM +0200, Peter Zijlstra wrote: > > > > This is too lax - it will be enabled for any !0 value. Please accept > > > only 0 and 1. > > > > kstrtobool() ftw > > And looking at that, I find it really strange it does not in fact accept > "true" / "false", so how about this? > > --- > Subject: lib: Extend kstrtobool() to accept "true"/"false" > > Extend the strings recognised by kstrtobool() to cover: > > - 1/0 > - y/n > - yes/no (new) > - t/f (new) > - true/false (new) > - on/off > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> There were some worries about dealing with unterminated strings when I did the original conversion[1], but I think those all got fixed. Reviewed-by: Kees Cook <keescook@chromium.org> [1] https://lore.kernel.org/lkml/CAGXu5jJrFv5Y8Q_i3yFYBDmT0+pO05dS3ijB0gOn-huasxZWmA@mail.gmail.com/
Hi! > > > This is too lax - it will be enabled for any !0 value. Please accept > > > only 0 and 1. > > > > kstrtobool() ftw > > And looking at that, I find it really strange it does not in fact accept > "true" / "false", so how about this? > > --- > Subject: lib: Extend kstrtobool() to accept "true"/"false" > > Extend the strings recognised by kstrtobool() to cover: > > - 1/0 > - y/n > - yes/no (new) > - t/f (new) > - true/false (new) > - on/off Is it good idea to add more values there? It is easy to do, but... we don't want people to use this by hand, and ideally everyone would just use 1/0... I also see potential for confusion... as in echo off > enable_off_mode (ok, this is with existing code, but...) Plus, if programs learn to do "echo true > ..." they will stop working on older kernels. Plus, this really should be documented somewhere, as it is kernel ABI. IMO this does not need changing. NAK. Best regards, Pavel (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 29 Jun 2020 14:09:38 +0200 Pavel Machek <pavel@ucw.cz> wrote: > > Extend the strings recognised by kstrtobool() to cover: > > > > - 1/0 > > - y/n > > - yes/no (new) > > - t/f (new) > > - true/false (new) > > - on/off > > Is it good idea to add more values there? It is easy to do, but... we don't want > people to use this by hand, and ideally everyone would just use 1/0... > > I also see potential for confusion... as in echo off > enable_off_mode (ok, this is > with existing code, but...) > > Plus, if programs learn to do "echo true > ..." they will stop working on older kernels. I'm inclined to agree with this, It is indeed an invitation to write non-back-compatible userspace and it simply makes the kernel interface more complex.
diff --git a/lib/kstrtox.c b/lib/kstrtox.c index 1006bf70bf74..b8b950325097 100644 --- a/lib/kstrtox.c +++ b/lib/kstrtox.c @@ -325,9 +325,17 @@ EXPORT_SYMBOL(kstrtos8); * @s: input string * @res: result * - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or - * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL. Value - * pointed to by res is updated upon finding a match. + * This return return 0 on success, otherwise it will return -EINVAL. + * It will accept (case invariant): + * + * - 1/0 + * - y/n + * - yes/no + * - t/f + * - true/false + * - on/off + * + * and set @*res to either true/false respectively. */ int kstrtobool(const char *s, bool *res) { @@ -335,30 +343,52 @@ int kstrtobool(const char *s, bool *res) return -EINVAL; switch (s[0]) { + case 't': + case 'T': + if (!s[1] || !strcasecmp(s, "true")) + goto have_true; + + break; + case 'y': case 'Y': + if (!s[1] || !strcasecmp(s, "yes")) + goto have_true; + + break; + case '1': +have_true: *res = true; return 0; + + case 'f': + case 'F': + if (!s[1] || !strcasecmp(s, "false")) + goto have_false; + + break; case 'n': case 'N': + if (!s[1] || !strcasecmp(s, "no")) + goto have_false; + + break; case '0': +have_false: *res = false; return 0; + case 'o': case 'O': - switch (s[1]) { - case 'n': - case 'N': - *res = true; - return 0; - case 'f': - case 'F': - *res = false; - return 0; - default: - break; - } + if (!strcasecmp(s, "on")) + goto have_true; + + if (!strcasecmp(s, "off")) + goto have_false; + + break; + default: break; }