Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1435226 > unrolled thread

kstrtojunk

Back to article view | Back to linux.kernel

Contents

  kstrtojunk Alexey Dobriyan <adobriyan@gmail.com> - 2016-07-01 12:00 +0200
    Re: kstrtojunk Kees Cook <keescook@chromium.org> - 2016-07-01 16:00 +0200
      Re: kstrtojunk Alexey Dobriyan <adobriyan@gmail.com> - 2016-07-04 23:30 +0200

#1435226 — kstrtojunk

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

[toc] | [next] | [standalone]

#1435352

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

[toc] | [prev] | [next] | [standalone]

#1436608

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,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web