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

[Rodrigo Alencar]

On 26/05/13 10:41AM, Rodrigo Alencar wrote:
> On 26/05/10 01:42PM, Rodrigo Alencar via B4 Relay wrote:
> > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > 
> > 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.
> 
> Hi Andy,
> 
> I am starting over here, the other conversation is getting hard to follow.
> This is my new proposal...

+cc David
 
> ...
> 
> > +static int _kstrtoudec64(const char *s, unsigned int scale, u64 *res)
> > +{
> > +	u64 _res = 0, _frac = 0;
> > +	unsigned int rv;
> > +
> > +	if (scale > 19) /* log10(2^64) = 19.26 */
> > +		return -EINVAL;
> > +
> > +	if (*s != '.') {
> > +		rv = _parse_integer(s, 10, &_res);
> > +		if (rv & KSTRTOX_OVERFLOW)
> > +			return -ERANGE;
> > +		if (rv == 0)
> > +			return -EINVAL;
> > +		s += rv;
> > +	}
> > +
> > +	if (*s == '.' && scale) {
> > +		s++; /* skip decimal point */
> > +		rv = _parse_integer_limit(s, 10, &_frac, scale);
> > +		if (rv & KSTRTOX_OVERFLOW)
> > +			return -ERANGE;
> > +		if (rv == 0)
> > +			return -EINVAL;
> > +		s += rv;
> > +		if (rv < scale)
> > +			_frac *= int_pow(10, scale - rv);
> > +		while (isdigit(*s)) /* truncate */
> > +			s++;
> > +	}
> > +
> > +	if (*s == '\n')
> > +		s++;
> > +	if (*s)
> > +		return -EINVAL;
> > +
> > +	if (check_mul_overflow(_res, int_pow(10, scale), &_res) ||
> > +	    check_add_overflow(_res, _frac, &_res))
> > +		return -ERANGE;
> > +
> > +	*res = _res;
> > +	return 0;
> > +}
> 
> This function now becomes:
> 
> 	static int _kstrtoudec64(const char *s, unsigned int scale, u64 *res)
> 	{
> 		u64 _res = 0;
> 		unsigned int rv_int, rv_frac;
> 
> 		rv_int = _parse_integer(s, 10, &_res);
> 		if (rv_int & KSTRTOX_OVERFLOW)
> 			return -ERANGE;
> 		s += rv_int;
> 
> 		if (*s == '.')
> 			s++; /* skip decimal point */
> 
> 		rv_frac = _parse_integer_limit_init(s, 10, _res, &_res, scale);
> 		if (rv_frac & KSTRTOX_OVERFLOW)
> 			return -ERANGE;
> 		s += rv_frac;
> 
> 		if (!rv_int && !rv_frac && !isdigit(*s))
> 			return -EINVAL; /* no digits at all */
> 
> 		while (isdigit(*s)) /* truncate digits */
> 			s++;
> 
> 		if (*s == '\n')
> 			s++;
> 		if (*s)
> 			return -EINVAL;
> 
> 		if (_res && (scale > (19 + rv_frac) || /* log10(2^64) = 19.26 */
> 		    check_mul_overflow(_res, int_pow(10, scale - rv_frac), &_res)))
> 			return -ERANGE;
> 
> 		*res = _res;
> 		return 0;
> 	}
> 
> The new thing here is _parse_integer_limit_init(), which is a local modified
> helper that accepts an init value, so _parse_integer_limit() becomes:
> 
> 	unsigned int _parse_integer_limit(const char *s, unsigned int base,
> 					  unsigned long long *p, size_t max_chars)
> 	{
> 		return _parse_integer_limit_init(s, base, 0, p, max_chars);
> 	}
> 
> with init = 0:
> 
> 	static unsigned int _parse_integer_limit_init(const char *s, unsigned int base,
> 						      unsigned long long init,
> 						      unsigned long long *p,
> 						      size_t max_chars)
> 	{
> 		unsigned long long res;
> 		unsigned int rv;
> 
> 		res = init;
> 		/* ...
> 		 * the rest is the same implementation as _parse_integer_limit()
> 		 * ...
> 		 */
> 		return rv;
> 	}
> 
> That allows to accumulate the final value into the same variable, which makes
> things simpler and decreases the amount of overflow checks.
> 
> The scale can now be a bigger value, like 0.00000000000000000000000000000000423
> can be parsed with scale = 35, resulting into 423.
> 
> The truncation loop is still there... I think this implementation is better,
> and I am not sure what is the input limit that you would consider ok to allow
> non-zero digits to be truncated once the scale can now be something bigger than 19.
> As long as the output fits into a u64 variable, the parser still works.

The truncation loop is at least stricting the input on digits!
Any comments on that?

> 
> I am also adding new test cases for that!

I have a v13 ready with this. I'll give it a go soon...

-- 
Kind regards,

Rodrigo Alencar