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

[Rodrigo Alencar]

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@xxxxxxxxxx> 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.

> 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.

> 
> 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
> 
> 

-- 
Kind regards,

Rodrigo Alencar