Re: [PATCH 0/3] sysctl: detect overflows when setting integers

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

Hello Alexey,

thank you for reviewing.

On 30.03.2015 14:34, Alexey Dobriyan wrote:
>> Unfortunately functions simple_strtoul and simple_strtoull cannot
>> be replaced by kstrtoul and kstrtoull in some places, because they
>> expect a zero terminated string instead of returning a pointer to
>> the character after the last digit.
>>
>> This patch introduces two new functions kstrtoul_e and kstrtoull_e
>> which fill this gap.
> 
> Well, there were two ideas:
> a) to convert first, see what's left and generalize it,
> b) kstrtox() should be used only in one place --
>    parsing integers in proc/sysfs files.

Neither a) nor b) are mentioned in your patch
https://lkml.org/lkml/2011/2/26/52.

How does a) apply to my patch series?

The patch series is about parsing integers inside the /proc mount so
what is the conflict with b)?

vsprintf.c has the following comment for simple_strtol:
* This function is obsolete. Please use kstrtol instead.

Could you, please, elaborate why kstrox.c should not be used in other
places? Should we duplicate these functions instead of reusing them?

> 
> The functions can probably be replaced by sscanf() (I didn't look closely).

No. sscanf does not return the end of the parsed string. It requires to
know beforehand if the string contains a octal, decimal or hexadecimal
number.

> 
> I hate "_e" suffix with passion.

Which names would you appreciate?

> 
> C in 2015 doesn't have this arcane concept known as optional
> parameters so I'd suggest to hack around with always returning
> number of OK characters

Which functions should return the number of OK characters?

> OR add one (just one) more separate
> interface
> 
>    unsigned int parse_integer(const char *s, unsigned int base, T *p);

Why would you call this function parse_integer and not kstrtox?

> 
> and dispatching with __builtin_choose_expr().

https://gcc.gnu.org/onlinedocs/gcc-3.4.5/gcc/Other-Builtins.html
describes how to use __builtin_types_compatible_p together with
__builtin_choose_expr().

I do not understand how to avoid exposing the interfaces of the
functions actually called by a parse_integer function multiplexer.

> 
> Of course if someone rewrites that abomination called
> sysctl parsing from scratch, maybe none of this will be needed!

Unfortunately you are very vague about what you dislike in sysctl.

When replying, please use the existing mail thread.

Best regards

Heinrich