Re: [PATCH 05/10] parse_integer: convert lib/

[Alexey Dobriyan]

On Mon, May 4, 2015 at 5:10 PM, Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, May 02 2015, Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
>
>> match_number() needlessly allocates/duplicates memory,
>> parsing can be done straight from original string.
>
> I suppose that's true, but the patch doesn't seem to do anything about
> it? It's probably better to do in a separate cleanup anyway, but then
> maybe this belongs as a comment below ---.

Sure, it is just "raise eyebrow" moment.


>> @@ -126,38 +127,37 @@ EXPORT_SYMBOL(get_options);
>>
>>  unsigned long long memparse(const char *ptr, char **retptr)
>>  {
>> -     char *endptr;   /* local pointer to end of parsed string */
>> +     unsigned long long val;
>>
>> -     unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
>> -
>> -     switch (*endptr) {
>> +     ptr += parse_integer(ptr, 0, &val);
>
> This seems wrong. simple_strtoull used to "sanitize" the return value
> from the (old) _parse_integer, so that endptr still points into the
> given string. Unconditionally adding the result from parse_integer may
> make ptr point far before the actual string, into who-knows-what.

When converting I tried to preserve the amount of error checking done.
simple_strtoull() either
a) return 0 and not advance pointer, or
b) return something and advance pointer.

Current code just ignores error case, so do I.

>> --- a/lib/parser.c
>> +++ b/lib/parser.c
>> @@ -44,7 +44,7 @@ static int match_one(char *s, const char *p, substring_t args[])
>>               p = meta + 1;
>>
>>               if (isdigit(*p))
>> -                     len = simple_strtoul(p, (char **) &p, 10);
>> +                     p += parse_integer(p, 10, (unsigned int *)&len);
>
> Hm, I think passing cast expressions to parse_integer should be actively
> discouraged. While it might work in this case, some day somebody will
> copy-paste this to a place where the len variable doesn't have
> sizeof==4.

This cast to turn on unsigned (or signed) parsing is the only nontrivial place.

All I can say is that C programmer should be diligent about types.
Equivalent strtol() code has silent truncation. Equivalent code with any other
real function which accepts pointer has the same problem -- direct cast.

We have to live with it, I guess.

>> @@ -68,19 +68,21 @@ static int match_one(char *s, const char *p, substring_t args[])
>>                       break;
>>               }
>>               case 'd':
>> -                     simple_strtol(s, &args[argc].to, 0);
>> +                     /* anonymous variable */
>> +                     len = parse_integer(s, 0, &(int []){0}[0]);
>>                       goto num;
>>               case 'u':
>> -                     simple_strtoul(s, &args[argc].to, 0);
>> +                     len = parse_integer(s, 0, &(unsigned int []){0}[0]);
>>                       goto num;
>>               case 'o':
>> -                     simple_strtoul(s, &args[argc].to, 8);
>> +                     len = parse_integer(s, 8, &(unsigned int []){0}[0]);
>>                       goto num;
>>               case 'x':
>> -                     simple_strtoul(s, &args[argc].to, 16);
>> +                     len = parse_integer(s, 16, &(unsigned int []){0}[0]);
>
> I see that the commit log says "don't be scared", and the first of these
> even has a comment. But is there any reason to be that clever here? I
> see a couple of downsides:
>
> * gcc has to initialize some stack memory to 0, since it cannot know it
>   is only an output parameter.

Yes.

> * things like this usually consume an excessive amount of stack. I
>   haven't been able to try your patches, but a simplified version of the
>   above shows that gcc doesn't use the same stack slots for the various
>   anonymous variables.

Yes.

> * It's unreadable, despite the comment and the commit log.

> I suggest using the more obvious approach of declaring a union on the
> stack and pass the address of the appropriate member.

Union will work.

No that it matters, it's a filesystem option parsing code
which is executed rarely near the top of sys_mount(),
not exactly perfomance bottleneck.

It's a shame to lose such clever hack :-(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/