kstrtojunk

kstrtojunk

[Alexey Dobriyan]

Could you please stop converting to kstrtobool()?

commit a81a5a17d44b26521fb1199f8ccf27f4af337a67
Author: Kees Cook <keescook@chromium.org>
Date:   Thu Mar 17 14:22:57 2016 -0700

    lib: add "on"/"off" support to kstrtobool

Especially after doing patches like this?

How on earth this was accepted?

Now the kernel is supposed to know about every pair of words
with the opposite meaning and accept them.

If kstrtobool() is ever going to be added it should accept
only '0' and '1' characters because kernel is not there
to second guess (same logic applies to whitespace trimming
for proc/sysfs files).

Another point is that in C/C++ any value other than 0
is true in for bool but kstrtobool() doesn't accept, say '2'
as true. This is why it wasn't added in the first place.

It is amazing to see how people think that every 2 common
lines of code should be generalized and pushed into lib/.

  Alexey

Re: kstrtojunk

[Kees Cook]

On Fri, Jul 1, 2016 at 5:53 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Could you please stop converting to kstrtobool()?
>
> commit a81a5a17d44b26521fb1199f8ccf27f4af337a67
> Author: Kees Cook <keescook@chromium.org>
> Date:   Thu Mar 17 14:22:57 2016 -0700
>
>     lib: add "on"/"off" support to kstrtobool
>
> Especially after doing patches like this?
>
> How on earth this was accepted?
>
> Now the kernel is supposed to know about every pair of words
> with the opposite meaning and accept them.
>
> If kstrtobool() is ever going to be added it should accept
> only '0' and '1' characters because kernel is not there
> to second guess (same logic applies to whitespace trimming
> for proc/sysfs files).
>
> Another point is that in C/C++ any value other than 0
> is true in for bool but kstrtobool() doesn't accept, say '2'
> as true. This is why it wasn't added in the first place.
>
> It is amazing to see how people think that every 2 common
> lines of code should be generalized and pushed into lib/.

There were plenty of common use-cases of the 0/1/no/yes/off/on usage.
I was specifically asked to use a common function, and this seemed
like the cleanest approach. kstrtobool already took y/n, and hasn't
ever handled "2" to mean "true", and is clearly documented. There's no
reason it couldn't be adjusted to do that, though.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: kstrtojunk

[Alexey Dobriyan]

On Fri, Jul 01, 2016 at 09:57:19AM -0400, Kees Cook wrote:
> On Fri, Jul 1, 2016 at 5:53 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Could you please stop converting to kstrtobool()?
> >
> > commit a81a5a17d44b26521fb1199f8ccf27f4af337a67
> > Author: Kees Cook <keescook@chromium.org>
> > Date:   Thu Mar 17 14:22:57 2016 -0700
> >
> >     lib: add "on"/"off" support to kstrtobool
> >
> > Especially after doing patches like this?
> >
> > How on earth this was accepted?
> >
> > Now the kernel is supposed to know about every pair of words
> > with the opposite meaning and accept them.
> >
> > If kstrtobool() is ever going to be added it should accept
> > only '0' and '1' characters because kernel is not there
> > to second guess (same logic applies to whitespace trimming
> > for proc/sysfs files).
> >
> > Another point is that in C/C++ any value other than 0
> > is true in for bool but kstrtobool() doesn't accept, say '2'
> > as true. This is why it wasn't added in the first place.
> >
> > It is amazing to see how people think that every 2 common
> > lines of code should be generalized and pushed into lib/.
> 
> There were plenty of common use-cases of the 0/1/no/yes/off/on usage.

The mistake was to allow y/n/Y/N/yes/no/on/off in the first place.
Supporting anything but 0/1 by core code legitimizes those usages.
But since y/n/Y/N/... can't be removed, it is a mistake to move them
to core code, let it rot.

> I was specifically asked to use a common function, and this seemed
> like the cleanest approach. kstrtobool already took y/n, and hasn't
> ever handled "2" to mean "true", and is clearly documented. There's no
> reason it couldn't be adjusted to do that, though.

Other things:
* "!s" check, other functions don't do that,
* doesn't handle \n,
* can't spell "off" right,
* no tests,