Re: [PATCH v2 10/24] platform/x86: ideapad-laptop: misc. device attribute changes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 16, 2021 at 11:54 PM Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
> 2021. január 16., szombat 20:52 keltezéssel, Andy Shevchenko írta:
> > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote:

...

> > > -       if (sscanf(buf, "%i", &state) != 1)
> > > -               return -EINVAL;
> > > +       ret = kstrtouint(buf, 0, &state);
> > > +       if (ret)
> > > +               return ret;
> >
> > This seems to me a relaxing case, and should be 10 instead of 0. Am I
> > right about %i?
> > If it's true it's probably minor, but still an ABI breakage.
>
> According to the latest C99 draft[1] (7.19.6.2):
>
>   [The 'i' format specifier] Matches an optionally signed integer, whose format
>   is the same as expected for the subject sequence of the strtol function with
>   the value 0 for the base argument. The corresponding argument shall be a pointer
>   to signed integer.

> Skimming over `vsscanf()`, I'm fairly sure it implements the same behaviour.
> So '0' as the base is correct, I believe.

Ah, okay, good to know. I assumed that %i is decimal only.
Thanks!

>  But technically it's still an ABI
> breakage since negative numbers are no longer accepted. In the case of
> `store_ideapad_fan()` it changes nothing since there was bounds checking in place.
> In the case of `store_ideapad_cam()` negative numbers are now rejected. I'm not
> sure if this change should be of great concern, since the the documentation only
> mentions '0' and '1', and I would be surprised if anyone used this interface
> to send negative numbers to the embedded controller.

If it's only 0 / 1 perhaps you may go further and make it bool
(kstrtobool() API)?

> [1]: https://wg14.link/c99



-- 
With Best Regards,
Andy Shevchenko




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux