Messages in this thread | | | Date | Mon, 4 May 2015 17:57:16 +0300 | Subject | Re: [PATCH 05/10] parse_integer: convert lib/ | From | Alexey Dobriyan <> |
| |
On Mon, May 4, 2015 at 5:10 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On Sat, May 02 2015, Alexey Dobriyan <adobriyan@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 :-(
| |