On Mon, May 4, 2015 at 5:10 PM, Rasmus Villemoes
<li...@rasmusvillemoes.dk> wrote:
> On Sat, May 02 2015, Alexey Dobriyan <adobri...@gmail.com> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
