From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751822AbbEDOdA (ORCPT ); Mon, 4 May 2015 10:33:00 -0400 Received: from mail-lb0-f180.google.com ([209.85.217.180]:35638 "EHLO mail-lb0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751162AbbEDOcw (ORCPT ); Mon, 4 May 2015 10:32:52 -0400 MIME-Version: 1.0 In-Reply-To: <87h9rsiigj.fsf@rasmusvillemoes.dk> References: <20150502004714.GA21655@p183.telecom.by> <87h9rsiigj.fsf@rasmusvillemoes.dk> Date: Mon, 4 May 2015 17:32:50 +0300 Message-ID: Subject: Re: [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) 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 4:24 PM, Rasmus Villemoes wrote: > On Sat, May 02 2015, Alexey Dobriyan wrote: >> Enter parse_integer(). >> >> int parse_integer(const char *s, unsigned int base, T *val); >> > > I like the general idea. Few nits below (and in reply to other patches). > > First: Could you tell me what tree I can commit this on top of, to see > what gcc makes of it. Any recent kernel should be OK, code it quite self contained. I've just applied first two patches on top 4.1-rc2. BTW the correct order is 1) [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) *** 2) [PATCH CORRECT 03/10] parse_integer: convert sscanf() 3) [PATCH 03/10] parse_integer: convert sscanf() 4) [PATCH 04/10] sscanf: fix overflow ... 10) [PATCH 10/10] ext2, ext3, ext4: convert to parse_integer()/kstrto*() I've copied patch #2 twice, so it won't apply and resent it with subject from patch #3 to confuse everyone even more. >> +#define parse_integer(s, base, val) \ >> +({ \ >> + const char *_s = (s); \ >> + unsigned int _base = (base); \ >> + typeof(val) _val = (val); \ >> + \ >> + __builtin_choose_expr( \ >> + __builtin_types_compatible_p(typeof(_val), signed char *), \ >> + _parse_integer_sc(_s, _base, (void *)_val), >> \ > > Why the (void*) cast? Isn't _val supposed to have precisely the type > expected by _parse_integer_sc at this point? > >> + __builtin_choose_expr( \ >> + __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 4,\ >> + _parse_integer_i(_s, _base, (void *)_val), \ >> + __builtin_choose_expr( \ >> + __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 8,\ >> + _parse_integer_ll(_s, _base, (void *)_val), \ >> + __builtin_choose_expr( \ >> + __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 4,\ >> + _parse_integer_u(_s, _base, (void *)_val), \ >> + __builtin_choose_expr( \ >> + __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 8,\ >> + _parse_integer_ull(_s, _base, (void *)_val), \ > > Ah, I see. In these cases, one probably has to do a cast to pass a > (long*) as either (int*) or (long long*) - but why not cast to the type > actually expected by _parse_integer_* instead of the launder-anything (void*)? First macro was written without casts at all naively thinking that gcc will only typecheck in chosen __builtin_choose_expr branch. But it doesn't do that, remove casts and observe million of warnings. So I shut it up with "void *". Branch is chosen base on __b_t_c_p expression and I don't think it is possible to sneak in incorrect pointer. > Another thing: It may be slightly confusing that this can't be used with > an array passed as val. This won't work: > > long x[1]; > rv = parse_integer(s, 0, x); > One could argue that one should pass &x[0] instead, but since this is a > macro, gcc doesn't really give a very helpful error (I just get "error: > invalid initializer"). I think it can be fixed simply by declaring _val > using typeof(&val[0]). I'd say &x[0] is way more clear that x in this case, but objection taken. kstrto*() works in exactly same situation after all. >> +int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val) >> +{ >> + int rv; >> + >> + if (*s == '-') { >> + return -EINVAL; >> + } else if (*s == '+') { >> + rv = __parse_integer(s + 1, base, val); >> + if (rv < 0) >> + return rv; >> + return rv + 1; >> + } else >> + return __parse_integer(s, base, val); >> +} >> +EXPORT_SYMBOL(_parse_integer_ull); >> + >> +int _parse_integer_ll(const char *s, unsigned int base, long long *val) >> +{ >> + unsigned long long tmp; >> + int rv; >> + >> + if (*s == '-') { >> + rv = __parse_integer(s + 1, base, &tmp); >> + if (rv < 0) >> + return rv; >> + if ((long long)-tmp >= 0) >> + return -ERANGE; > > Is there any reason to disallow "-0"? No! -0 is not accepted because code is copied from kstrtoll() which doesn't accept "-0". It is even in the testsuite: static void __init test_kstrtoll_fail(void) { ... /* negative zero isn't an integer in Linux */ {"-0", 0}, {"-0", 8}, {"-0", 10}, {"-0", 16}, Frankly I don't even remember why it does that, and no one noticed until now. libc functions accept "-0".