linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexey Dobriyan <adobriyan@gmail.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 05/10] parse_integer: convert lib/
Date: Mon, 4 May 2015 17:57:16 +0300	[thread overview]
Message-ID: <CACVxJT_6uxbHvq=KrfB9vP9==H+a480=Rf1C4cU8tkbPXSzspA@mail.gmail.com> (raw)
In-Reply-To: <87d22gigbt.fsf@rasmusvillemoes.dk>

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 :-(

  reply	other threads:[~2015-05-04 14:57 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-02  0:47 [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) Alexey Dobriyan
2015-05-02  0:48 ` [PATCH 02/10] parse_integer: rewrite kstrto*() Alexey Dobriyan
2015-05-02  0:50 ` [PATCH 03/10] parse_integer: convert sscanf() Alexey Dobriyan
2015-05-02  1:10   ` [PATCH CORRECT " Alexey Dobriyan
2015-05-02  0:51 ` [PATCH 04/10] sscanf: fix overflow Alexey Dobriyan
2015-05-05  9:51   ` Rasmus Villemoes
2015-05-05 11:10     ` Alexey Dobriyan
2015-05-06  7:49       ` Rasmus Villemoes
2015-05-02  0:53 ` [PATCH 05/10] parse_integer: convert lib/ Alexey Dobriyan
2015-05-04 14:10   ` Rasmus Villemoes
2015-05-04 14:57     ` Alexey Dobriyan [this message]
2015-05-02  0:55 ` [PATCH 06/10] parse_integer: convert mm/ Alexey Dobriyan
2015-05-04 14:33   ` Rasmus Villemoes
2015-05-04 15:09     ` Alexey Dobriyan
2015-05-02  0:56 ` [PATCH 07/10] parse_integer: convert misc fs/ code Alexey Dobriyan
2015-05-02  0:59 ` [PATCH 08/10] fs/cachefiles/: convert to parse_integer() Alexey Dobriyan
2015-05-02  1:01 ` [PATCH 09/10] ocfs2: convert to parse_integer()/kstrto*() Alexey Dobriyan
2015-05-02  1:03 ` [PATCH 10/10] ext2, ext3, ext4: " Alexey Dobriyan
2015-05-04 13:24 ` [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) Rasmus Villemoes
2015-05-04 14:32   ` Alexey Dobriyan
2015-05-04 16:44     ` Rasmus Villemoes
2015-05-04 19:54       ` Alexey Dobriyan
2015-05-04 21:48         ` Rasmus Villemoes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACVxJT_6uxbHvq=KrfB9vP9==H+a480=Rf1C4cU8tkbPXSzspA@mail.gmail.com' \
    --to=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --subject='Re: [PATCH 05/10] parse_integer: convert lib/' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).