Re: [PATCH v3 2/4] kstrtox: introduce a safer version of memparse()

Qu Wenruo
2024-01-04T06:43:00Z

On 2024/1/4 17:00, Randy Dunlap wrote:
> Hi,
>
[...]
>> + * parameter.
>> + *
>> + * @suffixes: The suffixes which should be parsed. Use logical ORed
>> + * memparse_suffix enum to indicate the supported suffixes.
>> + * The suffixes are case-insensive, all 2 ^ 10 based.
>
> case-insensitive
>
>> + * Supported ones are "KMGPTE".
>> + * NOTE: If one suffix out of the supported one is hit, it would
>
> ones
>
>> + * end the parse normally, with @retptr pointed to the unsupported
>> + * suffix.
>
> Could you explain (or give an example) of "to the unsupported suffix"?
> This isn't clear IMO.

Oh, my bad, that sentence itself is not correct.

What I really want to say is:

If one suffix (one of "KMGPTE") is hit but that suffix is not
specified in the @suffxies parameter, it would end the parse normally,
with @retptr pointed to the (unsupported) suffix.

The example would be the "68k " case in the ok cases in the next patch.
We have two different cases for the same "68k" string, with different
@suffixes and different results:

"68k ", KMGPTE -> 68 * 1024, @retptr at " ".
"68k ", M -> 68, @retptr at 'k'.

I don't have a better expression here unfortunately, maybe the special
case is not even worthy explaining?

>
>> + *
>> + * @res: Where to write the result.
>> + *
>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>> + *
>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>> + * Return -INVALID if no valid number string can be found.
>> + * Return -ERANGE if the number overflows.
>> + * For minus return values, @retptr would not be updated.
>
> * Returns:
> * * %0 if any valid numeric string can be parsed, and @retptr is updated.
> * * %-EINVAL if no valid number string can be found.
> * * %-ERANGE if the number overflows.
> * * For negative return values, @retptr is not updated.
>
>
> For *ALL* of the comments, I request/suggest that you change the "would be" or
> "would not be" to "is" or "is not" or whatever present tense words make the
> most sense.

No problem.

Thanks,
Qu

>
>
>> + */
>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>> + unsigned long long *res, char **retptr)
>> +{
>> + unsigned long long value;
>> + unsigned int rv;
>> + int shift = 0;
>> + int base = 0;
>> +
>> + s = _parse_integer_fixup_radix(s, &base);
>> + rv = _parse_integer(s, base, &value);
>> + if (rv & KSTRTOX_OVERFLOW)
>> + return -ERANGE;
>> + if (rv == 0)
>> + return -EINVAL;
>> +
>> + s += rv;
>> + switch (*s) {
>> + case 'K':
>> + case 'k':
>> + if (!(suffixes & MEMPARSE_SUFFIX_K))
>> + break;
>> + shift = 10;
>> + break;
>> + case 'M':
>> + case 'm':
>> + if (!(suffixes & MEMPARSE_SUFFIX_M))
>> + break;
>> + shift = 20;
>> + break;
>> + case 'G':
>> + case 'g':
>> + if (!(suffixes & MEMPARSE_SUFFIX_G))
>> + break;
>> + shift = 30;
>> + break;
>> + case 'T':
>> + case 't':
>> + if (!(suffixes & MEMPARSE_SUFFIX_T))
>> + break;
>> + shift = 40;
>> + break;
>> + case 'P':
>> + case 'p':
>> + if (!(suffixes & MEMPARSE_SUFFIX_P))
>> + break;
>> + shift = 50;
>> + break;
>> + case 'E':
>> + case 'e':
>> + if (!(suffixes & MEMPARSE_SUFFIX_E))
>> + break;
>> + shift = 60;
>> + break;
>> + }
>> + if (shift) {
>> + s++;
>> + if (value >> (64 - shift))
>> + return -ERANGE;
>> + value <<= shift;
>> + }
>> + *res = value;
>> + if (retptr)
>> + *retptr = (char *)s;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(memparse_safe);
>> +
>> /**
>> * kstrtoull - convert a string to an unsigned long long
>> * @s: The start of the string. The string must be null-terminated, and may also
>
> Thanks.