From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753019AbbEHUqt (ORCPT ); Fri, 8 May 2015 16:46:49 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:33005 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752220AbbEHUqr (ORCPT ); Fri, 8 May 2015 16:46:47 -0400 Date: Fri, 8 May 2015 13:46:46 -0700 From: Andrew Morton To: Alexey Dobriyan Cc: linux-kernel@vger.kernel.org, linux@rasmusvillemoes.dk Subject: Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*()) Message-Id: <20150508134646.6b9bf4158d220b65c5a922f9@linux-foundation.org> In-Reply-To: <20150508183029.GB9044@p183.telecom.by> References: <20150508182911.GA9044@p183.telecom.by> <20150508183029.GB9044@p183.telecom.by> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan wrote: > kstrto*() and kstrto*_from_user() family of functions were added > to help with parsing one integer written as string to proc/sysfs/debugfs > files. But they have a limitation: string passed must end with \0 or \n\0. > There are enough places where kstrto*() functions can't be used because of > this limitation. Trivial example: major:minor "%u:%u". > > Currently the only way to parse everything is simple_strto*() functions. > But they are suboptimal: > * they do not detect overflow (can be fixed, but no one bothered since ~0.99.11), > * there are only 4 of them -- long and "long long" versions, > This leads to silent truncation in the most simple case: > > val = strtoul(s, NULL, 0); > > * half of the people think that "char **endp" argument is necessary and > add unnecessary variable. > > OpenBSD people, fed up with how complex correct integer parsing is, added > strtonum(3) to fixup for deficiencies of libc-style integer parsing: > http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strtonum.3?query=strtonum&arch=i386 > > It'd be OK to copy that but it relies on "errno" and fixed strings as > error reporting channel which I think is not OK for kernel. > strtonum() also doesn't report number of characted consumed. > > What to do? > > Enter parse_integer(). fs/binfmt_misc.c | 12 fs/cachefiles/daemon.c | 84 ++-- fs/dcache.c | 2 fs/ext2/super.c | 6 fs/ext3/super.c | 7 fs/ext4/super.c | 15 fs/inode.c | 2 fs/libfs.c | 26 - fs/namespace.c | 4 fs/ocfs2/cluster/heartbeat.c | 54 +- fs/ocfs2/cluster/nodemanager.c | 50 +- fs/ocfs2/stack_user.c | 52 +- include/linux/kernel.h | 129 ------- include/linux/parse-integer.h | 188 ++++++++++ lib/Kconfig.debug | 3 lib/Makefile | 2 lib/cmdline.c | 42 +- lib/kstrtox.c | 254 ------------- lib/kstrtox.h | 1 lib/parse-integer.c | 222 ++++++++++++ lib/parser.c | 33 - lib/swiotlb.c | 2 lib/test-kstrtox.c | 6 lib/test-parse-integer.c | 563 +++++++++++++++++++++++++++++++ lib/vsprintf.c | 81 ++-- mm/memcontrol.c | 19 - mm/memtest.c | 2 mm/page_alloc.c | 2 mm/shmem.c | 14 29 files changed, 1242 insertions(+), 635 deletions(-) So not counting lib/test-parse-integer.c, it's a net addition of 44 lines. That's OK. My overall reaction to this is "oh god, not again". Is it really worth it? > --- /dev/null > +++ b/include/linux/parse-integer.h > @@ -0,0 +1,79 @@ > +#ifndef _PARSE_INTEGER_H > +#define _PARSE_INTEGER_H > +#include > +#include > + > +/* > + * int parse_integer(const char *s, unsigned int base, T *val); > + * > + * Convert integer string representation to an integer. > + * Range of accepted values equals to that of type T. > + * > + * Conversion to unsigned integer accepts sign "+". > + * Conversion to signed integer accepts sign "+" and sign "-". > + * > + * Radix 0 means autodetection: leading "0x" implies radix 16, > + * leading "0" implies radix 8, otherwise radix is 10. > + * Autodetection hint works after optional sign, but not before. > + * > + * Return number of characters parsed or -E. > + * > + * "T=char" case is not supported because -f{un,}signed-char can silently > + * change range of accepted values. > + */ > +#define parse_integer(s, base, val) \ > +({ \ > + const char *_s = (s); \ > + unsigned int _base = (base); \ > + typeof(&(val)[0]) _val = (val); \ > + \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), signed char *), \ > + _parse_integer_sc(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), unsigned char *), \ > + _parse_integer_uc(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), short *), \ > + _parse_integer_s(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), unsigned short *), \ > + _parse_integer_us(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), int *), \ > + _parse_integer_i(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), unsigned int *), \ > + _parse_integer_u(_s, _base, (void *)_val), \ > + __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), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), long long *), \ > + _parse_integer_ll(_s, _base, (void *)_val), \ > + __builtin_choose_expr( \ > + __builtin_types_compatible_p(typeof(_val), unsigned long long *),\ > + _parse_integer_ull(_s, _base, (void *)_val), \ > + _parse_integer_link_time_error())))))))))))); \ > +}) Wow. > +/* internal, do not use */ > +int _parse_integer_sc(const char *s, unsigned int base, signed char *val); > +int _parse_integer_uc(const char *s, unsigned int base, unsigned char *val); > +int _parse_integer_s(const char *s, unsigned int base, short *val); > +int _parse_integer_us(const char *s, unsigned int base, unsigned short *val); > +int _parse_integer_i(const char *s, unsigned int base, int *val); > +int _parse_integer_u(const char *s, unsigned int base, unsigned int *val); > +int _parse_integer_ll(const char *s, unsigned int base, long long *val); > +int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val); These all have fairly lengthy implementations. Could it all be done with a single function? int __parse_integer(const char *s, unsigned int base, unsigned int size, void *val); Where "size" is 1,2,4,8 with the top bit set if signed?