[PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Andy Shevchenko]

The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
functions") updated a comment regard to simple_strto<foo>() functions, but
missed similar change in the vsprintf.c module.

Update comments in vsprintf.c as well for simple_strto<foo>() functions.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..d5641a217685 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -58,7 +58,7 @@
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  *
- * This function is obsolete. Please use kstrtoull instead.
+ * This function has caveats. Please use kstrtoull instead.
  */
 unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
 {
@@ -83,7 +83,7 @@ EXPORT_SYMBOL(simple_strtoull);
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  *
- * This function is obsolete. Please use kstrtoul instead.
+ * This function has caveats. Please use kstrtoul instead.
  */
 unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
 {
@@ -97,7 +97,7 @@ EXPORT_SYMBOL(simple_strtoul);
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  *
- * This function is obsolete. Please use kstrtol instead.
+ * This function has caveats. Please use kstrtol instead.
  */
 long simple_strtol(const char *cp, char **endp, unsigned int base)
 {
@@ -114,7 +114,7 @@ EXPORT_SYMBOL(simple_strtol);
  * @endp: A pointer to the end of the parsed string will be placed here
  * @base: The number base to use
  *
- * This function is obsolete. Please use kstrtoll instead.
+ * This function has caveats. Please use kstrtoll instead.
  */
 long long simple_strtoll(const char *cp, char **endp, unsigned int base)
 {
-- 
2.25.0

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Petr Mladek]

On Fri 2020-02-21 10:57:23, Andy Shevchenko wrote:
> The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
> functions") updated a comment regard to simple_strto<foo>() functions, but
> missed similar change in the vsprintf.c module.
> 
> Update comments in vsprintf.c as well for simple_strto<foo>() functions.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks good to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Uwe Kleine-König]

Hello,

On Fri, Feb 21, 2020 at 10:57:23AM +0200, Andy Shevchenko wrote:
> The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
> functions") updated a comment regard to simple_strto<foo>() functions, but
> missed similar change in the vsprintf.c module.
> 
> Update comments in vsprintf.c as well for simple_strto<foo>() functions.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1ce318..d5641a217685 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -58,7 +58,7 @@
>   * @endp: A pointer to the end of the parsed string will be placed here
>   * @base: The number base to use
>   *
> - * This function is obsolete. Please use kstrtoull instead.
> + * This function has caveats. Please use kstrtoull instead.
>   */

I wonder if we instead want to create a set of functions that is
versatile enough to cover kstrtoull and simple_strtoull. i.e. fix the
rounding problems (that are the caveats, right?) and as calling
convention use an errorvalued int return + an output-parameter of the
corresponding type.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Andy Shevchenko]

On Fri, Feb 21, 2020 at 4:54 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Feb 21, 2020 at 10:57:23AM +0200, Andy Shevchenko wrote:
> > The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
> > functions") updated a comment regard to simple_strto<foo>() functions, but
> > missed similar change in the vsprintf.c module.
> >
> > Update comments in vsprintf.c as well for simple_strto<foo>() functions.

...

> > - * This function is obsolete. Please use kstrtoull instead.
> > + * This function has caveats. Please use kstrtoull instead.

> I wonder if we instead want to create a set of functions that is
> versatile enough to cover kstrtoull and simple_strtoull. i.e. fix the
> rounding problems (that are the caveats, right?) and as calling
> convention use an errorvalued int return + an output-parameter of the
> corresponding type.

It wouldn't be possible to apply same rules to both. They both are
part of existing ABI.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Uwe Kleine-König]

On Fri, Feb 21, 2020 at 05:27:49PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 21, 2020 at 4:54 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Feb 21, 2020 at 10:57:23AM +0200, Andy Shevchenko wrote:
> > > The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
> > > functions") updated a comment regard to simple_strto<foo>() functions, but
> > > missed similar change in the vsprintf.c module.
> > >
> > > Update comments in vsprintf.c as well for simple_strto<foo>() functions.
> 
> ...
> 
> > > - * This function is obsolete. Please use kstrtoull instead.
> > > + * This function has caveats. Please use kstrtoull instead.
> 
> > I wonder if we instead want to create a set of functions that is
> > versatile enough to cover kstrtoull and simple_strtoull. i.e. fix the
> > rounding problems (that are the caveats, right?) and as calling
> > convention use an errorvalued int return + an output-parameter of the
> > corresponding type.
> 
> It wouldn't be possible to apply same rules to both. They both are
> part of existing ABI.

The idea is to creat a sane set of functions, then convert all users to
the sane one and only then strip the strange functions away. (Userspace)
ABI isn't affected, is it?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Rasmus Villemoes]

On 21/02/2020 17.33, Uwe Kleine-König wrote:
> On Fri, Feb 21, 2020 at 05:27:49PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 21, 2020 at 4:54 PM Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>> On Fri, Feb 21, 2020 at 10:57:23AM +0200, Andy Shevchenko wrote:
>>>> The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
>>>> functions") updated a comment regard to simple_strto<foo>() functions, but
>>>> missed similar change in the vsprintf.c module.
>>>>
>>>> Update comments in vsprintf.c as well for simple_strto<foo>() functions.
>>
>> ...
>>
>>>> - * This function is obsolete. Please use kstrtoull instead.
>>>> + * This function has caveats. Please use kstrtoull instead.
>>
>>> I wonder if we instead want to create a set of functions that is
>>> versatile enough to cover kstrtoull and simple_strtoull. i.e. fix the
>>> rounding problems (that are the caveats, right?) and as calling
>>> convention use an errorvalued int return + an output-parameter of the
>>> corresponding type.
>>
>> It wouldn't be possible to apply same rules to both. They both are
>> part of existing ABI.
> 
> The idea is to creat a sane set of functions, then convert all users to
> the sane one and only then strip the strange functions away. (Userspace)
> ABI isn't affected, is it?

There are lots of in-tree users of all these interfaces, converting them
all is never going to happen. And yes, there are also kstrtox_user
variants which are definitely part of ABI (more or less the whole reason
kstrox accepts a single trailing newline but is otherwise rather strict
is so it can parse stuff that is echo'd to a sysfs/procfs/... file).

But, one can at least try to unify the underlying integer parsing, and
provide knobs for users (meaning other parts of the kernel) to decide
how lax or strict to be in a given situation. Based on an idea from
Alexey Dobriyan of creating a single type-generic parser, I did that a
couple of years ago. It rebased pretty cleanly, so if anyone wants to
take a look:

https://github.com/Villemoes/linux/tree/parse-integer

There's certainly more to do, but just doing all kstrtox() in terms of
parse_integer() seems to reduce vmlinux size. Next steps would be the
simple_strtox family, which more or less just need PARSE_INTEGER_WRAP
AFAICT.

And stuff like strtoul_lenient in kernel/sysctl.c is an example of a
caller that wants to specify the laxness of the parsing (no overflow
allowed, so simple_* doesn't cut it, but we're parsing a string where we
might want to continue after the current number, so kstrtox is too
strict in terms of what trailers are allowed).

Rasmus

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Petr Mladek]

On Sat 2020-02-22 01:28:25, Rasmus Villemoes wrote:
> On 21/02/2020 17.33, Uwe Kleine-König wrote:
> > On Fri, Feb 21, 2020 at 05:27:49PM +0200, Andy Shevchenko wrote:
> >> On Fri, Feb 21, 2020 at 4:54 PM Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >>> On Fri, Feb 21, 2020 at 10:57:23AM +0200, Andy Shevchenko wrote:
> >>>> The commit 885e68e8b7b1 ("kernel.h: update comment about simple_strto<foo>()
> >>>> functions") updated a comment regard to simple_strto<foo>() functions, but
> >>>> missed similar change in the vsprintf.c module.
> >>>>
> >>>> Update comments in vsprintf.c as well for simple_strto<foo>() functions.
> >>
> >> ...
> >>
> >>>> - * This function is obsolete. Please use kstrtoull instead.
> >>>> + * This function has caveats. Please use kstrtoull instead.
> >>
> >>> I wonder if we instead want to create a set of functions that is
> >>> versatile enough to cover kstrtoull and simple_strtoull. i.e. fix the
> >>> rounding problems (that are the caveats, right?) and as calling
> >>> convention use an errorvalued int return + an output-parameter of the
> >>> corresponding type.
> >>
> >> It wouldn't be possible to apply same rules to both. They both are
> >> part of existing ABI.
> > 
> > The idea is to creat a sane set of functions, then convert all users to
> > the sane one and only then strip the strange functions away. (Userspace)
> > ABI isn't affected, is it?
> 
> There are lots of in-tree users of all these interfaces, converting them
> all is never going to happen. And yes, there are also kstrtox_user
> variants which are definitely part of ABI (more or less the whole reason
> kstrox accepts a single trailing newline but is otherwise rather strict
> is so it can parse stuff that is echo'd to a sysfs/procfs/... file).

Thanks a lot for the detailed answer. It seems that there is no easy
solution to the problem.

Is still anyone against the original patch updating the comments in
vsprintf.c. Otherwise, I would queue it for 5.7.

Best Regards,
Petr

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Rasmus Villemoes]

On 27/02/2020 14.14, Petr Mladek wrote:
> 
> Is still anyone against the original patch updating the comments in
> vsprintf.c. Otherwise, I would queue it for 5.7.

Please do.

Rasmus

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Sergey Senozhatsky]

On (20/02/27 14:14), Petr Mladek wrote:
> 
> Is still anyone against the original patch updating the comments in
> vsprintf.c. Otherwise, I would queue it for 5.7.
> 

+1

	-ss

Re: [PATCH v1] lib/vsprintf: update comment about simple_strto<foo>() functions

[Petr Mladek]

On Thu 2020-02-27 17:01:15, Rasmus Villemoes wrote:
> On 27/02/2020 14.14, Petr Mladek wrote:
> > 
> > Is still anyone against the original patch updating the comments in
> > vsprintf.c. Otherwise, I would queue it for 5.7.
> 
> Please do.

The patch has been committed into printk.git, for-5.7 branch.

Best Regards,
Petr