Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/12/18 23:29, David Disseldorp wrote:
> Hi Qu,
>
> On Fri, 15 Dec 2023 19:09:23 +1030, Qu Wenruo wrote:
>
[...]
>> +/**
>> + * kstrtoull_suffix - convert a string to ull with suffixes support
>> + * @s: The start of the string. The string must be null-terminated, and may also
>> + *  include a single newline before its terminating null.
>> + * @base: The number base to use. The maximum supported base is 16. If base is
>> + *  given as 0, then the base of the string is automatically detected with the
>> + *  conventional semantics - If it begins with 0x the number will be parsed as a
>> + *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
>> + *  parsed as an octal number. Otherwise it will be parsed as a decimal.
>> + * @res: Where to write the result of the conversion on success.
>> + * @suffixes: A string of acceptable suffixes, must be provided. Or caller
>> + *  should use kstrtoull() directly.
>
> The suffixes parameter seems a bit cumbersome; callers need to provide
> both upper and lower cases, and unsupported characters aren't checked
> for. However, I can't think of any better suggestions at this stage.
>

Initially I went bitmap for the prefixes, but it's not any better.

Firstly where the bitmap should start. If we go bit 0 for "K", then the
code would introduce some difference between the bit number and left
shift (bit 0, left shift 10), which may be a little confusing.

If we go bit 1 for "K", the bit and left shift it much better, but bit 0
behavior would be left untouched.

Finally the bitmap itself is not that straightforward.

The limitation of providing both upper and lower case is due to the fact
that we don't have a case insensitive version of strchr().
But I think it's not that to fix, just convert them all to lower or
upper case, then do the strchr().

Would accepting both cases for the suffixes be good enough?

>> + *
>> + *
>> + * Return 0 on success.
>> + *
>> + * Return -ERANGE on overflow or -EINVAL if invalid chars found.
>> + * Return value must be checked.
>> + */
>> +int kstrtoull_suffix(const char *s, unsigned int base, unsigned long long *res,
>> +		     const char *suffixes)
>> +{
>> +	unsigned long long init_value;
>> +	unsigned long long final_value;
>> +	char *endptr;
>> +	int ret;
>> +
>> +	ret = _kstrtoull(s, base, &init_value, &endptr);
>> +	/* Either already overflow or no number string at all. */
>> +	if (ret < 0)
>> +		return ret;
>> +	final_value = init_value;
>> +	/* No suffixes. */
>> +	if (!*endptr)
>> +		goto done;
>> +
>> +	switch (*endptr) {
>> +	case 'K':
>> +	case 'k':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 10;
>> +		endptr++;
>> +		break;
>> +	case 'M':
>> +	case 'm':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 20;
>> +		endptr++;
>> +		break;
>> +	case 'G':
>> +	case 'g':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 30;
>> +		endptr++;
>> +		break;
>> +	case 'T':
>> +	case 't':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 40;
>> +		endptr++;
>> +		break;
>> +	case 'P':
>> +	case 'p':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 50;
>> +		endptr++;
>> +		break;
>> +	case 'E':
>> +	case 'e':
>> +		if (!strchr(suffixes, *endptr))
>> +			return -EINVAL;
>> +		final_value <<= 60;
>> +		endptr++;
>> +		break;
>> +	}
>> +	if (*endptr == '\n')
>
> Nit: the per-case logic could be simplified to a single "shift_val = X"
> if you initialise and handle !shift_val.

Indeed, thanks for the hint!

Thanks,
Qu
>
>> +		endptr++;
>> +	if (*endptr)
>> +		return -EINVAL;
>> +
>> +	/* Overflow check. */
>> +	if (final_value < init_value)
>> +		return -EOVERFLOW;
>> +done:
>> +	*res = final_value;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(kstrtoull_suffix);
>> +
>>   /**
>>    * kstrtoll - convert a string to a long long
>>    * @s: The start of the string. The string must be null-terminated, and may also
>> @@ -159,7 +262,7 @@ int kstrtoll(const char *s, unsigned int base, long long *res)
>>   	int rv;
>>
>>   	if (s[0] == '-') {
>> -		rv = _kstrtoull(s + 1, base, &tmp);
>> +		rv = _kstrtoull(s + 1, base, &tmp, NULL);
>>   		if (rv < 0)
>>   			return rv;
>>   		if ((long long)-tmp > 0)
>
>
>