From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751422AbbEDO52 (ORCPT ); Mon, 4 May 2015 10:57:28 -0400 Received: from mail-la0-f53.google.com ([209.85.215.53]:35820 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751007AbbEDO5S (ORCPT ); Mon, 4 May 2015 10:57:18 -0400 MIME-Version: 1.0 In-Reply-To: <87d22gigbt.fsf@rasmusvillemoes.dk> References: <20150502004714.GA21655@p183.telecom.by> <20150502005324.GE21655@p183.telecom.by> <87d22gigbt.fsf@rasmusvillemoes.dk> Date: Mon, 4 May 2015 17:57:16 +0300 Message-ID: Subject: Re: [PATCH 05/10] parse_integer: convert lib/ From: Alexey Dobriyan To: Rasmus Villemoes Cc: Andrew Morton , Linux Kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 4, 2015 at 5:10 PM, Rasmus Villemoes wrote: > On Sat, May 02 2015, Alexey Dobriyan 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 :-(