Re: [PATCH 2/5] serial: core: add sysfs attribute to suppress ready signalling on open

[Andy Shevchenko]

On Tue, Dec 1, 2020 at 1:04 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Tue, Dec 01, 2020 at 12:55:54PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 1, 2020 at 10:20 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > On Mon, Nov 30, 2020 at 08:27:54PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 30, 2020 at 5:42 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> >
> > > > > +       ret = kstrtouint(buf, 0, &val);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > >
> > > > > +       if (val > 1)
> > > > > +               return -EINVAL;
> > > >
> > > > Can't we utilise kstrtobool() instead?
> > >
> > > I chose not to as kstrtobool() results in a horrid interface. To many
> > > options to do the same thing and you end up with confusing things like
> > > "0x01" being accepted but treated as false (as only the first character
> > > is considered).
> >
> > And this is perfectly fine. 0x01 is not boolean.
>
> 0x01 is 1 and is generally treated as boolean true as you know.

Depends how you interpret this. kstrtobool() uses one character (and
in some cases two) of the input. Everything else is garbage.
Should we interpret garbage?

> So why should a sysfs-interface accept it as valid input and treat it as
> false? That's just bad design.

I can agree with this.

> > > Not sure how that ever made it into sysfs code...
> > >
> > > The attribute is read back as "0" or "1" and those are precisely the
> > > values that can be written back (well, modulo radix).
> >
> > So, how does it affect the kstrtobool() interface?
> > You read back 0 and 1 and they are pretty much accepted by it.
> >
> > > It's not relevant in this case, but tight control over the inputs also
> > > allows for extending the range later.
> >
> > And kstrtobool() does it. So I don't see any difference except a few
> > less lines of code and actually *stricter* rules than kstrtouint()
> > has.
>
> You miss the point; kstrobool accepts "12" today and treats it as true.
> You cannot extend such an interface to later accept a larger range than
> 0 and 1 as you didn't return an error for "12" from the start (as someone
> might now rely on "12" being treated as "1").

Somehow cifs uses kstrtobool() in conjunction with the wider ranges. Nobody
complained so far. But maybe they had it from day 1.

So, we have two issues here: kstrtobool() doesn't report an error of
input when it has garbage, the user may rely on garbage to be
discarded.

-- 
With Best Regards,
Andy Shevchenko