Messages in this thread Patch in this message | | | Date | Fri, 8 May 2015 13:46:46 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*()) |
| |
On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan <adobriyan@gmail.com> 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 <linux/compiler.h> > +#include <linux/types.h> > + > +/* > + * 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?
| |