Re: [PATCH v12 02/11] lib: kstrtox: add kstrtoudec64() and kstrtodec64()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, May 12, 2026 at 03:12:24PM +0100, Rodrigo Alencar wrote:
> On 26/05/12 04:48PM, Andy Shevchenko wrote:
> > On Tue, May 12, 2026 at 02:21:14PM +0100, Rodrigo Alencar wrote:
> > > On 26/05/12 04:12PM, Andy Shevchenko wrote:
> > > > On Tue, May 12, 2026 at 12:39:53PM +0100, Jonathan Cameron wrote:
> > > > > On Sun, 10 May 2026 13:42:20 +0100
> > > > > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> > > > > 
> > > > > > Add helpers that parses decimal numbers into 64-bit number, i.e., decimal
> > > > > > point numbers with pre-defined scale are parsed into a 64-bit value (fixed
> > > > > > precision). After the decimal point, digits beyond the specified scale
> > > > > > are ignored.
> > > > > 
> > > > > Whilst Rodrigo has already replied to say there will be another version
> > > > > I'd like to request final feedback from those who were involved in the parser
> > > > > discussions.  
> > > > > 
> > > > > They got very involved and I'm far from an expert in the right way to do
> > > > > this stuff.  
> > > > > 
> > > > > I don't think David Laight was +CC so I've added that.
> > > > > David, Andy - I think you two were most involved in that discussion:
> > > > > Any objections to the end result? 
> > > > 
> > > > I already said a few times about the naming. I do not like the kstrto*()
> > > > be semantically different on how they treat the input. Second point is
> > > > to avoid code duplication, but this one is less of a concern since the
> > > > new code is in the library close to the other potentially duplicate code
> > > > piece and hence can be addressed later.
> > > 
> > > I suppose I reached into kstrtodec64() and kstrtoudec64() because it aligns
> > > with your expectations for kstrto*() semantics, no? Those include:
> > >  - overflow check;
> > >  - extensive input validation;
> > >  - optional '\n' in the end;
> > >  - mandatory nul-termination.
> > > 
> > > am I missing anything?
> > 
> > When we add scale we basically make that not true. Moreover the code in this
> > patch makes scale == number_of_characters which I think a bit fragile, however
> > it's about the fractional part when the amount of digits is equal to scale.
> 
> That is not really the case. It is being set as a limit, so it does check for
> truncation and zero-padding.

I do not see it happens in _parse_integer_limit(). It doesn't try to parse more
characters than it's requested in max_chars. It doesn't check if there are more
character nor their converted values.

> > To make this work as expected we need to add an additional call like
> > kstrtoull() (and perhaps drop that \n and NUL-terminator checks) and see
> > if that overflows or not. Since it's a fractional part it must have less
> > than 20 (decimal) digits there, so we check the rv (or how many digits
> > were parsed successfully) and compare to 20. If it's more, we got too many
> > decimal digits.
> 
> For overflow it checks the KSTRTOX_OVERFLOW flag and leverages check_mul_overflow()
> and check_add_overflow() when combining fractional and integer parts. The amount
> of characters is not really important there. The scale cannot be bigger than 19 and
> that makes sure that int_pow() does not overflow. The code uses _parse_integer_limit()
> due to the nature of input and to avoid 64-bit division, kstrtoull() at any point
> (parsing integer or fractional parts) does not make much sense.

Under 'like kstrotoull()' I meant something that repeats needed functionality.
I believe it's parse_integer() (without limit).

> > Maybe I'm missing these checks already performed?
> > 
> > > > Having the test cases is a big benefit, and that part I like the most.

-- 
With Best Regards,
Andy Shevchenko