kstrto*: accept "-0" for signed conversion
Subsystems (13)
PATCH 1/12 kstrto*: accept "-0" for signed conversion
strtol(3) et al accept "-0", so should we.
--- lib/kstrtox.c | 2 +- lib/test-kstrtox.c | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-)
lib/kstrtox.c +1 -1
@@ -152,7 +152,7 @@ int kstrtoll(const char *s, unsigned int base, long long *res) rv =_kstrtoull (s +1 , base, &tmp);if (rv <0 )return rv; -if ((long long )(-tmp) >=0 ) +if ((long long )-tmp >0 )return -ERANGE ; *res = -tmp; }else {
lib/test-kstrtox.c +1 -5
@@ -260,6 +260,7 @@ static void __init test_kstrtoll_ok(void) {"4294967297", 10, 4294967297LL}, {"9223372036854775807", 10, 9223372036854775807LL}, + {"-0", 10, 0LL}, {"-1", 10, -1LL}, {"-2", 10, -2LL}, {"-9223372036854775808", 10, LLONG_MIN},
@@ -277,11 +278,6 @@ static void __init test_kstrtoll_fail(void) {"-9223372036854775809", 10}, {"-18446744073709551614", 10}, {"-18446744073709551615", 10}, -/* negative zero isn't an integer in Linux */ - {"-0", 0}, - {"-0", 8}, - {"-0", 10}, - {"-0", 16},/* sign is first character if any */ {"-+1", 0}, {"-+1", 8},
PATCH 2/12 Add parse_integer() (replacement for simple_strto*())
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().
int parse_integer(const char *s, unsigned int base, T *val);
Rationale:
* parse_integer() is exactly 1 (one) interface not 4 or many, one for each type.
* parse_integer() reports -E errors reliably in a canonical kernel way:
rv = parse_integer(str, 10, &val); if (rv < 0) return rv;
* parse_integer() writes result only if there were no errors, at least one digit has to be consumed,
* parse_integer doesn't mix error reporting channel and value channel, it does mix error and number-of-character-consumed channel, though.
* parse_integer() reports number of characters consumed, makes parsing multiple values easy:
rv = parse_integer(str, 0, &val1); if (rv < 0) return rv; str += rv; if (*str++ != ':') return -EINVAL; rv = parse_integer(str, 0, &val2); if (rv < 0) return rv; if (str[rv] != '\0') return -EINVAL;
There are several deficiencies in parse_integer() compared to strto*():
* can't be used in initializers:
const T x = strtoul();
* can't be used with bitfields,
* can't be used in expressions:
x = strtol() * 1024; x = y = strtol(); x += strtol();
* currently there is no support for _Bool and at least one place where simple_strtoul() is directly assigned to _Bool variable. It is trivial to add, but not clear if it should only accept "0" and "1", because, say, module param code accepts 0, 1, y, n, Y and N.
In defense of parse_integer() all I can say, is that using strtol() in expression or initializer promotes no error checking and thus probably should not be encouraged in C, language with no built-in error checking anyway.
The amount of "x = y = strtol()" expressions in kernel is very small. The amount of direct usage in expression is not small, but can be counted as an acceptable loss.
Misc suggestions and ideas from Rasmus Villemoes.
v2:
* self-contained header
* trim code size a bit
* testsuite
--- include/linux/kernel.h | 7 +- include/linux/parse-integer.h | 79 +++++++++++++++++ lib/Makefile | 1 + lib/kstrtox.c | 76 +++++----------- lib/kstrtox.h | 1 - lib/parse-integer.c | 198 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 305 insertions(+), 57 deletions(-) create mode 100644 include/linux/parse-integer.h create mode 100644 lib/parse-integer.c
include/linux/kernel.h +5 -2
@@ -10,6 +10,7 @@#include <linux/bitops.h> #include <linux/log2.h> #include <linux/typecheck.h> +#include <linux/parse-integer.h> #include <linux/printk.h> #include <linux/dynamic_debug.h> #include <asm/byteorder.h>
@@ -387,8 +388,10 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_treturn kstrtoint_from_user (s, count, base, res); } -/* Obsolete, do not use. Use kstrto<foo> instead */ - +/* +* Obsolete, do not use. +* Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf(). +*/ extern unsigned long simple_strtoul (const char *,char **,unsigned int );extern long simple_strtol (const char *,char **,unsigned int );extern unsigned long long simple_strtoull (const char *,char **,unsigned int );
include/linux/parse-integer.h new file +79 -0
79 lines not on this page · raw message
lib/Makefile +1 -0
@@ -32,6 +32,7 @@ obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o obj-y += hexdump.o obj-$(CONFIG_TEST_HEXDUMP) += test-hexdump.o obj-y += kstrtox.o +obj-y += parse-integer.o obj-$(CONFIG_TEST_BPF) += test_bpf.o obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o obj-$(CONFIG_TEST_KASAN) += test_kasan.o
lib/kstrtox.c +22 -54
103 lines not on this page · raw message
lib/kstrtox.h +0 -1
@@ -2,7 +2,6 @@#define _LIB_KSTRTOX_H#define KSTRTOX_OVERFLOW (1U << 31) -const char *_parse_integer_fixup_radix (const char *s,unsigned int *base);unsigned int _parse_integer (const char *s,unsigned int base,unsigned long long *res);#endif
lib/parse-integer.c new file +198 -0
198 lines not on this page · raw message
Signature · 1 line
2.0.4
Andrew Morton
maintainerRe: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
Quoted 21 lines · On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan <[email protected]> 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
Quoted 7 lines
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?
Quoted 60 lines
include/linux/parse-integer.h new file +57
-0@@ -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( \
include/linux/parse-integer.h line 67
Quoted 10 lines
include/linux/parse-integer.h new file +10
-0+ __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.
include/linux/parse-integer.h line 76
Quoted 9 lines
include/linux/parse-integer.h new file +9
-0+/* 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?
Rasmus Villemoes
reviewerRe: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
Quoted 2 lines · Andrew Morton
My overall reaction to this is "oh god, not again". Is it really worth it?
I think it is, if it's done right. The problem is to get consensus on what right means, but I think Alexey's approach is ok. The huge macro may be ugly, but it puts the ugliness in one place.
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+/* 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?
Quoted 3 lines
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?
I suggested something like that in private. These two patches roughly correspond to 02/12 and 04/12 (they are just proof-of-concept).
Subject: [PATCH 1/2] lib: introduce parse_integer
This is an alternative implementation of parse_integer. It has a slightly smaller code footprint (both in #LOC and in .text). Another motivation was to expand on the idea of passing flags to the underlying function, easing implementation of other interfaces in terms of parse_integer.
In the other proposal, PARSE_INTEGER_NEWLINE means three things, which I split into separate flags:
* Accept (but don't require) a single trailing newline.
* Require that the entire string is consumed (possibly after eating the trailing newline), otherwise return -EINVAL.
* Change return semantics: 0 for ok instead of #characters consumed.
Besides the three flags doing the above, I also added flags allowing consuming leading and/or trailing whitespace. This may be used to remove some boilerplate from code elsewhere. But this may be over-engineering at this point, and it's easy enough to rip out (and maybe add later). Still, I think it's nice to keep the behaviour-changing flags separate.
Implementation-wise (and what saves ~500 bytes of .text), the main difference is that there's only a single underlying function, and the type of the destination is communicated to it with another few bits in the base parameter. In the vast majority of cases, the combined base_type_flags parameter will be a compile-time constant.
--- include/linux/parse-integer.h | 78 ++++++++++++++++++++ lib/Makefile | 1 + lib/kstrtox.c | 16 ---- lib/kstrtox.h | 3 +- lib/parse-integer.c | 166 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 247 insertions(+), 17 deletions(-) create mode 100644 include/linux/parse-integer.h create mode 100644 lib/parse-integer.c
include/linux/parse-integer.h new file +78 -0
@@ -0,0 +1,78 @@ +#ifndef LINUX_PARSE_INTEGER_H +#define LINUX_PARSE_INTEGER_H + +#include <linux/compiler.h> +#include <linux/types.h> + +#define _PARSE_INTEGER_TYPE_SHIFT 8 +enum { + _PARSE_INTEGER_U8, + _PARSE_INTEGER_U16, + _PARSE_INTEGER_U32, + _PARSE_INTEGER_U64, + _PARSE_INTEGER_S8, + _PARSE_INTEGER_S16, + _PARSE_INTEGER_S32, + _PARSE_INTEGER_S64, +}; + +/* +* Various flags that can be ORed with the base to request slightly +* different semantics, to facilitate implementing various old +* interfaces in terms of parse_integer. Not to be used directly. +*/ + +/* allow (and skip) leading whitespace */ +#define _PARSE_INTEGER_LEAD_WS 0x08000000 +/* consume trailing whitespace */ +#define _PARSE_INTEGER_TRAIL_WS 0x10000000 +/* consume a single trailing newline */ +#define _PARSE_INTEGER_NEWLINE 0x20000000 +/* return 0 for success instead of #characters consumed */ +#define _PARSE_INTEGER_ZERO_ON_OK 0x40000000 +/* return -EINVAL unless the entire string was consumed */ +#define _PARSE_INTEGER_ALL 0x80000000 + +/* +* This should just be BUILD_BUG(), but due to header dependency hell +* we can't use that. +*/ +void __parse_integer_build_bug (void ); + +#define parse_integer (s, base_flags, dest) ({ \ + const char *_s = (s); \ + unsigned _btf = (base_flags); \ + typeof(&dest[0]) _dest = (dest); \ + unsigned _t; \ + \ + if (__builtin_types_compatible_p(typeof(_dest), u8*)) \ + _t = _PARSE_INTEGER_U8; \ + else if (__builtin_types_compatible_p(typeof(_dest), u16*)) \ + _t = _PARSE_INTEGER_U16; \ + else if (__builtin_types_compatible_p(typeof(_dest), u32*)) \ + _t = _PARSE_INTEGER_U32; \ + else if (__builtin_types_compatible_p(typeof(_dest), u64*)) \ + _t = _PARSE_INTEGER_U64; \ + else if (__builtin_types_compatible_p(typeof(_dest), s8*)) \ + _t = _PARSE_INTEGER_S8; \ + else if (__builtin_types_compatible_p(typeof(_dest), s16*)) \ + _t = _PARSE_INTEGER_S16; \ + else if (__builtin_types_compatible_p(typeof(_dest), s32*)) \ + _t = _PARSE_INTEGER_S32; \ + else if (__builtin_types_compatible_p(typeof(_dest), s64*)) \ + _t = _PARSE_INTEGER_S64; \ + else if (__builtin_types_compatible_p(typeof(_dest), unsigned long*)) \ + _t = BITS_PER_LONG == 64 ? _PARSE_INTEGER_U64 : _PARSE_INTEGER_U32; \ + else if (__builtin_types_compatible_p(typeof(_dest), long*)) \ + _t = BITS_PER_LONG == 64 ? _PARSE_INTEGER_S64 : _PARSE_INTEGER_S32; \ + else \ + __parse_integer_build_bug(); \ + _btf |= _t << _PARSE_INTEGER_TYPE_SHIFT; \ + __parse_integer(_s, _btf, _dest); \ +}) + +/* internal, do not use directly */ +int __parse_integer (const char *s,unsigned base_type_flags,void *val); +const char *_parse_integer_fixup_radix (const char *s,unsigned int *base); + +#endif /* LINUX_PARSE_INTEGER_H */
lib/Makefile +1 -0
@@ -32,6 +32,7 @@ obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o obj-y += hexdump.o obj-$(CONFIG_TEST_HEXDUMP) += test-hexdump.o obj-y += kstrtox.o +obj-y += parse-integer.o obj-$(CONFIG_TEST_BPF) += test_bpf.o obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o obj-$(CONFIG_TEST_KASAN) += test_kasan.o
lib/kstrtox.c +0 -16
@@ -20,22 +20,6 @@#include <asm/uaccess.h> #include "kstrtox.h" -const char *_parse_integer_fixup_radix (const char *s,unsigned int *base) -{ -if (*base ==0 ) { -if (s[0 ] =='0' ) { -if (_tolower (s[1 ]) =='x' &&isxdigit (s[2 ])) - *base =16 ; -else - *base =8 ; - }else - *base =10 ; - } -if (*base ==16 && s[0 ] =='0' &&_tolower (s[1 ]) =='x' ) - s +=2 ; -return s; -} -/* * Convert non-negative integer string representation in explicitly given radix * to an integer.
lib/kstrtox.h +2 -1
@@ -1,8 +1,9 @@#ifndef _LIB_KSTRTOX_H#define _LIB_KSTRTOX_H +#include <linux/parse-integer.h> +#define KSTRTOX_OVERFLOW (1U << 31) -const char *_parse_integer_fixup_radix (const char *s,unsigned int *base);unsigned int _parse_integer (const char *s,unsigned int base,unsigned long long *res);#endif
lib/parse-integer.c new file +166 -0
166 lines not on this page · raw message
Signature · 12 lines
2.1.3 Subject: [PATCH 2/2] lib: implement kstrtox using parse_integer The new parse_integer() interface makes it easy to implement the kstrtox family of functions as simple one-line static inlines. --- include/linux/kernel.h | 125 +--------------------- include/linux/parse-integer.h | 112 ++++++++++++++++++++ lib/kstrtox.c | 238 ------------------------------------------ 3 files changed, 113 insertions(+), 362 deletions(-)
include/linux/kernel.h +1 -124
137 lines not on this page · raw message
include/linux/parse-integer.h +112 -0
122 lines not on this page · raw message
lib/kstrtox.c +0 -238
244 lines not on this page · raw message
Signature · 1 line
2.1.3
Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
In reply to Andrew MortonQuoted 62 lines · Andrew Morton
Quoted 21 lines · On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan <[email protected]> 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=i386Quoted 7 lines
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(-)
Quoted 5 lines
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?
I think giving good examples to people is always worth it :-)
Quoted 38 lines
Quoted 38 lines
include/linux/parse-integer.h new file +38
-0+#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 *), \
Quoted 8 lines
include/linux/parse-integer.h line 67
Quoted 6 lines
include/linux/parse-integer.h new file +6
-0+_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.
Yep, but blame the language not me.
Macro would be simpler with _Generic, but it is too early for that.
Quoted 13 lines
include/linux/parse-integer.h line 76
Quoted 9 lines
include/linux/parse-integer.h new file +9
-0+/* 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?
Quoted 3 lines
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?
The question is why bother.
With smallish VM config I was testing, .text size difference is very small, LOC-wise the difference doesn't exist as well (additional error checking adds lines, not code per se). In the end _parse_integer() and simple_strto*() functions will be removed as well.
With 4-arg dispatch function, every callsite will be more bloated:
simple_strto* (pointer, pointer, int)
parse_integer (pointer, int, pointer)
4-arg (pointer, int, int, pointer)
I think this code is way understandable than any mask-shift alternative:
int _parse_integer_sc(const char *s, unsigned int base, signed char *val)
{
long long tmp;
int rv;
rv = _parse_integer_ll(s, base, &tmp);
if (rv < 0)
return rv;
if (tmp != (signed char)tmp)
return -ERANGE;
*val = tmp;
return rv;
}Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
[Answering to Noel Grandin, gmane sent email to list only]
Quoted 9 lines
Quoted 2 lines
int
parse_integer(const char *s, unsigned int base, T *val);I suspect splitting this into
parse_signed_int
and
parse_unsigned_int
(or some other equivalent names)
will make the call-sites a lot easier to read.
parse_integer() is about two and only two points (everything else is immaterial and technical details):
1) unified interface,
2) automatically fixing overflow bug based on type T.
Two interfaces in this situation is no different from having four.
C doesn't have generic functions so programmers get hardwired to million of names all doing roughly the same but __b_c_e and _Generic allows to mimic generic functions which is at least something.
Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
In reply to Alexey DobriyanQuoted 2 lines
int parse_integer(const char *s, unsigned int base, T *val);
I suspect splitting this into
parse_signed_int
and
parse_unsigned_int
(or some other equivalent names)
will make the call-sites a lot easier to read.
Andrew Morton
maintainerRe: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
In reply to Alexey DobriyanQuoted 3 lines · On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan <[email protected]> wrote:
Enter
parse_integer().int parse_integer(const char *s, unsigned int base, T *val);
OK, I grabbed these. Let's see how it goes. Were these patches still up to date?
Presumably we'd benefit from a checkpatch rule to alert people to the new regime. Can you please suggest what such a rule should do?
The macros in lib/test-parse-integer.c hurt my brain.
Re: [PATCH 02/12] Add parse_integer() (replacement for simple_strto*())
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Fri, 8 May 2015 21:30:29 +0300 Alexey Dobriyan <[email protected]> wrote:
Enter
parse_integer().int parse_integer(const char *s, unsigned int base, T *val);OK, I grabbed these.
Thanks! Just to note, first patch (accept "-0") is independent and can be sent at any time.
Quoted 1 line
Let's see how it goes. Were these patches still up to date?
Yes, kstrto*(), scanf() didn't change since when this was sent.
Quoted 2 lines
Presumably we'd benefit from a checkpatch rule to alert people to the new regime. Can you please suggest what such a rule should do?
It should say to switch from simple_strto*() to one of the 4 officially sanctioned integer conversion routines: parse_integer(), kstrto*(), kstrto*_from_user(), sscanf(). I'll send a patch.
Quoted 1 line
The macros in
lib/test-parse-integer.churt my brain.
Well, yes, but the only interesting thing there are test tables.
Alexey
PATCH 3/12 parse_integer: add runtime testsuite
In reply to Alexey DobriyanUse with CONFIG_PARSE_INTEGER=y.
--- lib/Kconfig.debug | 3 + lib/Makefile | 1 + lib/test-parse-integer.c | 563 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 567 insertions(+) create mode 100644 lib/test-parse-integer.c
lib/Kconfig.debug +3 -0
@@ -1628,6 +1628,9 @@ config TEST_HEXDUMP config TEST_STRING_HELPERS tristate "Test functions located in the string_helpers module at runtime" +config TEST_PARSE_INTEGER + tristate "Test parse_integer() function at runtime" + config TEST_KSTRTOX tristate "Test kstrto*() family of functions at runtime"
lib/Makefile +1 -0
@@ -36,6 +36,7 @@ obj-y += parse-integer.o obj-$(CONFIG_TEST_BPF) += test_bpf.o obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o obj-$(CONFIG_TEST_KASAN) += test_kasan.o +obj-$(CONFIG_TEST_PARSE_INTEGER) += test-parse-integer.o obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o obj-$(CONFIG_TEST_LKM) += test_module.o obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
lib/test-parse-integer.c new file +563 -0
563 lines not on this page · raw message
Signature · 1 line
2.0.4
PATCH 4/12 parse-integer: rewrite kstrto*()
In reply to Alexey DobriyanRewrite kstrto*() functions through parse_integer().
_kstrtoul() and _kstrtol() are removed because parse_integer() can dispatch based on sizeof(long) saving function call.
Also move function definitions and comment one instance. Remove redundant boilerplate comments from elsewhere.
High bit base hack suggested by Andrew Morton.
--- include/linux/kernel.h | 124 ----------------------- include/linux/parse-integer.h | 109 +++++++++++++++++++++ lib/kstrtox.c | 222 ------------------------------------------ lib/parse-integer.c | 26 ++++- 4 files changed, 134 insertions(+), 347 deletions(-)
include/linux/kernel.h +0 -124
@@ -264,130 +264,6 @@ void do_exit(long error_code)void complete_and_exit (struct completion *,long )__noreturn ; -/* Internal, do not use. */ -int __must_check _kstrtoul (const char *s,unsigned int base,unsigned long *res); -int __must_check _kstrtol (const char *s,unsigned int base,long *res); - -int __must_check kstrtoull (const char *s,unsigned int base,unsigned long long *res); -int __must_check kstrtoll (const char *s,unsigned int base,long long *res); - -/** -* kstrtoul - convert a string to an unsigned long -* @s: The start of the string. The string must be null-terminated, and may also -* include a single newline before its terminating null. The first character -* may also be a plus sign, but not a minus sign. -* @base: The number base to use. The maximum supported base is 16. If base is -* given as 0, then the base of the string is automatically detected with the -* conventional semantics - If it begins with 0x the number will be parsed as a -* hexadecimal (case insensitive), if it otherwise begins with 0, it will be -* parsed as an octal number. Otherwise it will be parsed as a decimal. -* @res: Where to write the result of the conversion on success. -* -* Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. -* Used as a replacement for the obsolete simple_strtoull. Return code must -* be checked. -*/ -static inline int __must_check kstrtoul (const char *s,unsigned int base,unsigned long *res) -{ -/* -* We want to shortcut function call, but -* __builtin_types_compatible_p(unsigned long, unsigned long long) = 0. -*/ -if (sizeof (unsigned long ) ==sizeof (unsigned long long ) && - __alignof__(unsigned long ) == __alignof__(unsigned long long )) -return kstrtoull (s, base, (unsigned long long *)res); -else -return _kstrtoul (s, base, res); -} - -/** -* kstrtol - convert a string to a long -* @s: The start of the string. The string must be null-terminated, and may also -* include a single newline before its terminating null. The first character -* may also be a plus sign or a minus sign. -* @base: The number base to use. The maximum supported base is 16. If base is -* given as 0, then the base of the string is automatically detected with the -* conventional semantics - If it begins with 0x the number will be parsed as a -* hexadecimal (case insensitive), if it otherwise begins with 0, it will be -* parsed as an octal number. Otherwise it will be parsed as a decimal. -* @res: Where to write the result of the conversion on success. -* -* Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. -* Used as a replacement for the obsolete simple_strtoull. Return code must -* be checked. -*/ -static inline int __must_check kstrtol (const char *s,unsigned int base,long *res) -{ -/* -* We want to shortcut function call, but -* __builtin_types_compatible_p(long, long long) = 0. -*/ -if (sizeof (long ) ==sizeof (long long ) && - __alignof__(long ) == __alignof__(long long )) -return kstrtoll (s, base, (long long *)res); -else -return _kstrtol (s, base, res); -} - -int __must_check kstrtouint (const char *s,unsigned int base,unsigned int *res); -int __must_check kstrtoint (const char *s,unsigned int base,int *res); - -static inline int __must_check kstrtou64 (const char *s,unsigned int base,u64 *res) -{ -return kstrtoull (s, base, res); -} - -static inline int __must_check kstrtos64 (const char *s,unsigned int base,s64 *res) -{ -return kstrtoll (s, base, res); -} - -static inline int __must_check kstrtou32 (const char *s,unsigned int base,u32 *res) -{ -return kstrtouint (s, base, res); -} - -static inline int __must_check kstrtos32 (const char *s,unsigned int base,s32 *res) -{ -return kstrtoint (s, base, res); -} - -int __must_check kstrtou16 (const char *s,unsigned int base,u16 *res); -int __must_check kstrtos16 (const char *s,unsigned int base,s16 *res); -int __must_check kstrtou8 (const char *s,unsigned int base,u8 *res); -int __must_check kstrtos8 (const char *s,unsigned int base,s8 *res); - -int __must_check kstrtoull_from_user (const char __user *s,size_t count,unsigned int base,unsigned long long *res); -int __must_check kstrtoll_from_user (const char __user *s,size_t count,unsigned int base,long long *res); -int __must_check kstrtoul_from_user (const char __user *s,size_t count,unsigned int base,unsigned long *res); -int __must_check kstrtol_from_user (const char __user *s,size_t count,unsigned int base,long *res); -int __must_check kstrtouint_from_user (const char __user *s,size_t count,unsigned int base,unsigned int *res); -int __must_check kstrtoint_from_user (const char __user *s,size_t count,unsigned int base,int *res); -int __must_check kstrtou16_from_user (const char __user *s,size_t count,unsigned int base,u16 *res); -int __must_check kstrtos16_from_user (const char __user *s,size_t count,unsigned int base,s16 *res); -int __must_check kstrtou8_from_user (const char __user *s,size_t count,unsigned int base,u8 *res); -int __must_check kstrtos8_from_user (const char __user *s,size_t count,unsigned int base,s8 *res); - -static inline int __must_check kstrtou64_from_user (const char __user *s,size_t count,unsigned int base,u64 *res) -{ -return kstrtoull_from_user (s, count, base, res); -} - -static inline int __must_check kstrtos64_from_user (const char __user *s,size_t count,unsigned int base,s64 *res) -{ -return kstrtoll_from_user (s, count, base, res); -} - -static inline int __must_check kstrtou32_from_user (const char __user *s,size_t count,unsigned int base,u32 *res) -{ -return kstrtouint_from_user (s, count, base, res); -} - -static inline int __must_check kstrtos32_from_user (const char __user *s,size_t count,unsigned int base,s32 *res) -{ -return kstrtoint_from_user (s, count, base, res); -} -/* * Obsolete, do not use. * Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf().
include/linux/parse-integer.h +109 -0
113 lines not on this page · raw message
lib/kstrtox.c +0 -222
228 lines not on this page · raw message
lib/parse-integer.c +25 -1
@@ -31,7 +31,7 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)return s; } -static int __parse_integer (const char *s,unsigned int base,unsigned long long *val) +static int ___parse_integer (const char *s,unsigned int base,unsigned long long *val) {const char *s0 = s, *sd;unsigned long long acc;
@@ -63,6 +63,26 @@ static int __parse_integer(const char *s, unsigned int base, unsigned long longreturn s - s0; } +static int __parse_integer (const char *s,unsigned int base,unsigned long long *val) +{ +unsigned long long tmp; +int rv; + + rv =___parse_integer (s, base & ~PARSE_INTEGER_NEWLINE , &tmp); +if (rv <0 ) +return rv; +if (base &PARSE_INTEGER_NEWLINE ) { +/* Accept "integer\0" or "integer\n\0" */ + s += rv; +if (*s =='\n' ) + s++; +if (*s) +return -EINVAL ; + } + *val = tmp; +return rv; +} +int _parse_integer_ull (const char *s,unsigned int base,unsigned long long *val) {char sign;
@@ -77,6 +97,8 @@ int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val rv =__parse_integer (s, base, val);if (rv <0 )return rv; +if (base &PARSE_INTEGER_NEWLINE ) +return 0 ;return rv + !!sign; }EXPORT_SYMBOL (_parse_integer_ull);
@@ -103,6 +125,8 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)return -ERANGE ; *val = tmp; } +if (base &PARSE_INTEGER_NEWLINE ) +return 0 ;return rv + !!sign; }EXPORT_SYMBOL (_parse_integer_ll);
Signature · 1 line
2.0.4
PATCH 5/12 parse_integer: convert scanf()
In reply to Alexey DobriyanRemove base second guessing -- done by parse_integer() just fine.
Uniformly fix too liberal acceptance in %lu/%ld cases in the next patch.
--- lib/vsprintf.c | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-)
lib/vsprintf.c +10 -26
@@ -2470,8 +2470,6 @@ EXPORT_SYMBOL_GPL(bprintf);int vsscanf (const char *buf,const char *fmt,va_list args) {const char *str = buf; -char *next; -char digit;int num =0 ;u8 qualifier;unsigned int base;
@@ -2483,6 +2481,8 @@ int vsscanf(const char *buf, const char *fmt, va_list args)bool is_sign;while (*fmt) { +int len; +/* skip any white space in format */ /* white space in format matchs any amount of * white space, including none, in the input.
@@ -2611,35 +2611,22 @@ int vsscanf(const char *buf, const char *fmt, va_list args)*/ str =skip_spaces (str); - digit = *str; -if (is_sign && digit =='-' ) - digit = *(str +1 ); - -if (!digit - || (base ==16 && !isxdigit (digit)) - || (base ==10 && !isdigit (digit)) - || (base ==8 && (!isdigit (digit) || digit >'7' )) - || (base ==0 && !isdigit (digit))) -break ; -if (is_sign) - val.s = qualifier !='L' ? -simple_strtol (str, &next, base) : -simple_strtoll (str, &next, base); + len =parse_integer (str, base, &val.s);else - val.u = qualifier !='L' ? -simple_strtoul (str, &next, base) : -simple_strtoull (str, &next, base); + len =parse_integer (str, base, &val.u); +if (len <0 ) +break ; -if (field_width >0 && next - str > field_width) { +if (field_width >0 ) {if (base ==0 )_parse_integer_fixup_radix (str, &base); -while (next - str > field_width) { +while (len > field_width) {if (is_sign) val.s =div_s64 (val.s, base);else val.u =div_u64 (val.u, base); - --next; + len--; } }
@@ -2680,10 +2667,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)break ; } num++; - -if (!next) -break ; - str = next; + str += len; }return num;
Signature · 1 line
2.0.4
PATCH 6/12 scanf: fix type range overflow
In reply to Alexey DobriyanFun fact:
uint8_t val;
sscanf("256", "%hhu", &val);will return 1 and make val=0 (clearly bogus).
Userspace sscanf() reports 1 parsed value, returns incorrect value but sets errno to ERANGE only for "%u" conversion and higher. %hhu and %hu are left in the cold.
Having no way to report errno=ERANGE in kernel, don't report successful parsing.
Patch allows to remove checks and switch to proper types in several (most?) cases:
grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
Such checks can be incorrect too -- checking for 3 digits with %3u for parsing uint8_t is not enough.
--- lib/vsprintf.c | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-)
lib/vsprintf.c +34 -11
@@ -2632,44 +2632,67 @@ int vsscanf(const char *buf, const char *fmt, va_list args)switch (qualifier) {case 'H' :/* that's 'hh' in format */ -if (is_sign) +if (is_sign) { +if (val.s != (signed char )val.s) + goto out; *va_arg(args, signed char *) = val.s; -else + }else { +if (val.u != (unsigned char )val.u) + goto out; *va_arg(args, unsigned char *) = val.u; + }break ;case 'h' : -if (is_sign) +if (is_sign) { +if (val.s != (short )val.s) + goto out; *va_arg(args, short *) = val.s; -else + }else { +if (val.u != (unsigned short )val.u) + goto out; *va_arg(args, unsigned short *) = val.u; + }break ;case 'l' : -if (is_sign) +if (is_sign) { +if (val.s != (long )val.s) + goto out; *va_arg(args, long *) = val.s; -else + }else { +if (val.u != (unsigned long )val.u) + goto out; *va_arg(args, unsigned long *) = val.u; + }break ;case 'L' : -if (is_sign) +if (is_sign) { *va_arg(args, long long *) = val.s; -else + }else { *va_arg(args, unsigned long long *) = val.u; + }break ;case 'Z' :case 'z' : +if (val.u != (size_t )val.u) + goto out; *va_arg(args, size_t *) = val.u;break ;default : -if (is_sign) +if (is_sign) { +if (val.s != (int )val.s) + goto out; *va_arg(args, int *) = val.s; -else + }else { +if (val.u != (unsigned int )val.u) + goto out; *va_arg(args, unsigned int *) = val.u; + }break ; } num++; str += len; } - +out:return num; }EXPORT_SYMBOL (vsscanf);
Signature · 1 line
2.0.4
PATCH 7/12 parse_integer: convert lib/
In reply to Alexey DobriyanConvert away lib/ from deprecated simple_strto*() interfaces.
--- lib/cmdline.c | 44 ++++++++++++++++++++++++-------------------- lib/parser.c | 33 ++++++++++++++++----------------- lib/swiotlb.c | 2 +- 3 files changed, 41 insertions(+), 38 deletions(-)
lib/cmdline.c +24 -20
@@ -27,7 +27,7 @@ static int get_range(char **str, int *pint)int x, inc_counter, upper_range; (*str)++; - upper_range =simple_strtol ((*str),NULL ,0 ); +parse_integer (*str,0 , &upper_range); inc_counter = upper_range - *pint;for (x = *pint; x < upper_range; x++) *pint++ = x;
@@ -51,13 +51,14 @@ static int get_range(char **str, int *pint)int get_option (char **str,int *pint) { -char *cur = *str; +int len; -if (!cur || !(*cur)) +if (!str || !*str)return 0 ; - *pint =simple_strtol (cur, str,0 ); -if (cur == *str) + len =parse_integer (*str,0 , pint); +if (len <0 )return 0 ; + *str += len;if (**str ==',' ) { (*str)++;return 2 ;
@@ -126,38 +127,41 @@ 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 ret =simple_strtoull (ptr, &endptr,0 ); - -switch (*endptr) { +unsigned long long val =0 ; +int len; + + len =parse_integer (ptr,0 , &val); +if (len <0 ) + goto out; + ptr += len; +switch (*ptr) {case 'E' :case 'e' : - ret <<=10 ; + val <<=10 ;case 'P' :case 'p' : - ret <<=10 ; + val <<=10 ;case 'T' :case 't' : - ret <<=10 ; + val <<=10 ;case 'G' :case 'g' : - ret <<=10 ; + val <<=10 ;case 'M' :case 'm' : - ret <<=10 ; + val <<=10 ;case 'K' :case 'k' : - ret <<=10 ; - endptr++; + val <<=10 ; + ptr++;default :break ; } - +out:if (retptr) - *retptr = endptr; + *retptr = (char *)ptr; -return ret; +return val; }EXPORT_SYMBOL (memparse);
lib/parser.c +16 -17
@@ -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);else if (*p =='%' ) {if (*s++ !='%' )return 0 ;
@@ -57,6 +57,11 @@ static int match_one(char *s, const char *p, substring_t args[]) args[argc].from = s;switch (*p++) { +union { +int i; +unsigned int u; + } u; +case 's' : {size_t str_len =strlen (s);
@@ -68,19 +73,20 @@ static int match_one(char *s, const char *p, substring_t args[])break ; }case 'd' : -simple_strtol (s, &args[argc].to,0 ); + len =parse_integer (s,0 , &u.i); goto num;case 'u' : -simple_strtoul (s, &args[argc].to,0 ); + len =parse_integer (s,0 , &u.u); goto num;case 'o' : -simple_strtoul (s, &args[argc].to,8 ); + len =parse_integer (s,8 , &u.u); goto num;case 'x' : -simple_strtoul (s, &args[argc].to,16 ); + len =parse_integer (s,16 , &u.u); num: -if (args[argc].to == args[argc].from) +if (len <0 )return 0 ; + args[argc].to = args[argc].from + len;break ;default :return 0 ;
@@ -127,10 +133,8 @@ EXPORT_SYMBOL(match_token);*/ static int match_number (substring_t *s,int *result,int base) { -char *endp;char *buf;int ret; -long val;size_t len = s->to - s->from; buf =kmalloc (len +1 ,GFP_KERNEL );
@@ -139,16 +143,11 @@ static int match_number(substring_t *s, int *result, int base)memcpy (buf, s->from, len); buf[len] ='\0' ; - ret =0 ; - val =simple_strtol (buf, &endp, base); -if (endp == buf) - ret = -EINVAL ; -else if (val < (long )INT_MIN || val > (long )INT_MAX ) - ret = -ERANGE ; -else - *result = (int ) val; + ret =parse_integer (buf, base, result);kfree (buf); -return ret; +if (ret <0 ) +return ret; +return 0 ; }/**
lib/swiotlb.c +1 -1
@@ -100,7 +100,7 @@ static int __init setup_io_tlb_npages(char *str) {if (isdigit (*str)) { - io_tlb_nslabs =simple_strtoul (str, &str,0 ); + str +=parse_integer (str,0 , &io_tlb_nslabs);/* avoid tail segment of size < IO_TLB_SEGSIZE */ io_tlb_nslabs =ALIGN (io_tlb_nslabs,IO_TLB_SEGSIZE ); }
Signature · 1 line
2.0.4
PATCH 8/12 parse_integer: convert mm/
In reply to Alexey DobriyanConvert mm/ directory away from deprecated simple_strto*() interface.
One thing to note about parse_integer() and seemingly useless casts -- range of accepted values depends on result type.
int val; parse_integer(s, 0, &val);
will accept negative integers, while
int val; parse_integer(s, 0, (unsigned int *)&val);
will accept only 0 and positive integers.
Cast is needed when result variable has to be of different type for some reason.
This is very important and hopefully [knocks wood] obvious.
--- mm/memcontrol.c | 19 +++++++++++-------- mm/memtest.c | 2 +- mm/page_alloc.c | 2 +- mm/shmem.c | 14 ++++++++------ 4 files changed, 21 insertions(+), 16 deletions(-)
mm/memcontrol.c +11 -8
@@ -4096,20 +4096,23 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,struct fd efile;struct fd cfile;const char *name; -char *endp;int ret; buf =strstrip (buf); - efd =simple_strtoul (buf, &endp,10 ); -if (*endp !=' ' ) + ret =parse_integer (buf,10 , &efd); +if (ret <0 ) +return ret; + buf += ret; +if (*buf++ !=' ' )return -EINVAL ; - buf = endp +1 ; - - cfd =simple_strtoul (buf, &endp,10 ); -if ((*endp !=' ' ) && (*endp !='\0' )) + ret =parse_integer (buf,10 , &efd); +if (ret <0 ) +return ret; + buf += ret; +if (*buf !=' ' && *buf !='\0' )return -EINVAL ; - buf = endp +1 ; + buf++; event =kzalloc (sizeof (*event),GFP_KERNEL );if (!event)
mm/memtest.c +1 -1
@@ -93,7 +93,7 @@ static int memtest_pattern __initdata;static int __init parse_memtest (char *arg) {if (arg) - memtest_pattern =simple_strtoul (arg,NULL ,0 ); +parse_integer (arg,0 , (unsigned int *)&memtest_pattern);else memtest_pattern =ARRAY_SIZE (patterns);
mm/page_alloc.c +1 -1
@@ -6020,7 +6020,7 @@ static int __init set_hashdist(char *str) {if (!str)return 0 ; - hashdist =simple_strtoul (str, &str,0 ); +parse_integer (str,0 , (unsigned int *)&hashdist);return 1 ; }__setup ("hashdist=" , set_hashdist);
mm/shmem.c +8 -6
@@ -2742,6 +2742,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,struct mempolicy *mpol =NULL ;uid_t uid;gid_t gid; +int rv;while (options !=NULL ) { this_char = options;
@@ -2795,14 +2796,15 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo, }else if (!strcmp (this_char,"mode" )) {if (remount)continue ; - sbinfo->mode =simple_strtoul (value, &rest,8 ) &07777 ; -if (*rest) + rv =parse_integer (value,8 , &sbinfo->mode); +if (rv <0 || value[rv]) goto bad_val; + sbinfo->mode &=07777 ; }else if (!strcmp (this_char,"uid" )) {if (remount)continue ; - uid =simple_strtoul (value, &rest,0 ); -if (*rest) + rv =parse_integer (value,0 , &uid); +if (rv <0 || value[rv]) goto bad_val; sbinfo->uid =make_kuid (current_user_ns (), uid);if (!uid_valid (sbinfo->uid))
@@ -2810,8 +2812,8 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo, }else if (!strcmp (this_char,"gid" )) {if (remount)continue ; - gid =simple_strtoul (value, &rest,0 ); -if (*rest) + rv =parse_integer (value,0 , &gid); +if (rv <0 || value[rv]) goto bad_val; sbinfo->gid =make_kgid (current_user_ns (), gid);if (!gid_valid (sbinfo->gid))
Signature · 1 line
2.0.4
PATCH 9/12 parse_integer: convert fs/
In reply to Alexey DobriyanConvert random fs/ code away from simple_strto*() interfaces.
Note about "struct simple_attr" conversion:
->set_buf is unneeded because everything can be done from stack. ->get_buf is useless as well, but that's a separate patch. Mutex is not removed, as it may guard readers from writers, separate story as well.
(code has been copied to arch/powerpc/.../spufs/, don't forget!)
binfmt_misc: file offset can't really be negative, type changed.
--- fs/binfmt_misc.c | 12 +++++++++--- fs/dcache.c | 2 +- fs/inode.c | 2 +- fs/libfs.c | 26 ++++++++++---------------- fs/namespace.c | 4 ++-- 5 files changed, 23 insertions(+), 23 deletions(-)
fs/binfmt_misc.c +9 -3
@@ -47,7 +47,7 @@ enum {Enabled, Magic}; typedef struct { struct list_head list; unsigned long flags; /* type, status, etc. */ - int offset; /* offset of magic */ + unsigned int offset; /* offset of magic */ int size; /* size of magic/mask */ char *magic; /* magic or filename extension */ char *mask; /* mask, NULL for exact match */
@@ -370,7 +370,13 @@ static Node *create_entry(const char __user *buffer, size_t count)if (!s) goto einval; *s++ ='\0' ; - e->offset =simple_strtoul (p, &p,10 ); + err =parse_integer (p,10 , &e->offset); +if (err <0 ) { +kfree (e); + goto out; + + } + p += err;if (*p++) goto einval;pr_debug ("register: offset: %#x\n" , e->offset);
@@ -548,7 +554,7 @@ static void entry_status(Node *e, char *page)if (!test_bit (Magic, &e->flags)) {sprintf (dp,"extension .%s\n" , e->magic); }else { - dp +=sprintf (dp,"offset %i\nmagic " , e->offset); + dp +=sprintf (dp,"offset %u\nmagic " , e->offset); dp =bin2hex (dp, e->magic, e->size);if (e->mask) { dp +=sprintf (dp,"\nmask " );
fs/dcache.c +1 -1
@@ -3374,7 +3374,7 @@ static int __init set_dhash_entries(char *str) {if (!str)return 0 ; - dhash_entries =simple_strtoul (str, &str,0 ); +parse_integer (str,0 , &dhash_entries);return 1 ; }__setup ("dhash_entries=" , set_dhash_entries);
fs/inode.c +1 -1
@@ -1787,7 +1787,7 @@ static int __init set_ihash_entries(char *str) {if (!str)return 0 ; - ihash_entries =simple_strtoul (str, &str,0 ); +parse_integer (str,0 , &ihash_entries);return 1 ; }__setup ("ihash_entries=" , set_ihash_entries);
fs/libfs.c +10 -16
@@ -752,7 +752,6 @@ struct simple_attr {int (*get)(void *,u64 *);int (*set)(void *,u64 );char get_buf[24 ];/* enough to store a u64 and "\n\0" */ -char set_buf[24 ];void *data;const char *fmt;/* format for read operation */ struct mutex mutex;/* protects access to these buffers */
@@ -830,31 +829,26 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf, size_t len, loff_t *ppos) { struct simple_attr *attr; -u64 val; -size_t size; -ssize_t ret; +s64 val; +int ret; attr = file->private_data;if (!attr->set)return -EACCES ; + ret =kstrtos64_from_user (buf, len,0 , &val); +if (ret <0 ) +return ret; + ret =mutex_lock_interruptible (&attr->mutex);if (ret)return ret; - - ret = -EFAULT ; - size =min (sizeof (attr->set_buf) -1 , len); -if (copy_from_user (attr->set_buf, buf, size)) - goto out; - - attr->set_buf[size] ='\0' ; - val =simple_strtoll (attr->set_buf,NULL ,0 ); ret = attr->set (attr->data, val); -if (ret ==0 ) - ret = len;/* on success, claim we got the whole input */ -out:mutex_unlock (&attr->mutex); -return ret; +if (ret <0 ) +return ret; +/* on success, claim we got the whole input */ +return len; }EXPORT_SYMBOL_GPL (simple_attr_write);
fs/namespace.c +2 -2
@@ -37,7 +37,7 @@ static int __init set_mhash_entries(char *str) {if (!str)return 0 ; - mhash_entries =simple_strtoul (str, &str,0 ); +parse_integer (str,0 , &mhash_entries);return 1 ; }__setup ("mhash_entries=" , set_mhash_entries);
@@ -47,7 +47,7 @@ static int __init set_mphash_entries(char *str) {if (!str)return 0 ; - mphash_entries =simple_strtoul (str, &str,0 ); +parse_integer (str,0 , &mphash_entries);return 1 ; }__setup ("mphash_entries=" , set_mphash_entries);
Signature · 1 line
2.0.4
PATCH 10/37 parse_integer: convert fs/cachefiles/
In reply to Alexey DobriyanConvert away from deprecated simple_strto*() interfaces.
Switch "unsigned long" to "unsigned int" where possible. kstrto*() functions can't be used because of trailing "%" sign. :^)
--- fs/cachefiles/daemon.c | 84 +++++++++++++++++++++++++++----------------------- 1 file changed, 45 insertions(+), 39 deletions(-)
fs/cachefiles/daemon.c +45 -39
@@ -326,14 +326,15 @@ static int cachefiles_daemon_range_error(struct cachefiles_cache *cache,*/ static int cachefiles_daemon_frun (struct cachefiles_cache *cache,char *args) { -unsigned long frun; +unsigned int frun; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - frun =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &frun); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -350,14 +351,15 @@ static int cachefiles_daemon_frun(struct cachefiles_cache *cache, char *args)*/ static int cachefiles_daemon_fcull (struct cachefiles_cache *cache,char *args) { -unsigned long fcull; +unsigned int fcull; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - fcull =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &fcull); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -374,14 +376,15 @@ static int cachefiles_daemon_fcull(struct cachefiles_cache *cache, char *args)*/ static int cachefiles_daemon_fstop (struct cachefiles_cache *cache,char *args) { -unsigned long fstop; +unsigned int fstop; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - fstop =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &fstop); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -398,14 +401,15 @@ static int cachefiles_daemon_fstop(struct cachefiles_cache *cache, char *args)*/ static int cachefiles_daemon_brun (struct cachefiles_cache *cache,char *args) { -unsigned long brun; +unsigned int brun; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - brun =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &brun); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -422,14 +426,15 @@ static int cachefiles_daemon_brun(struct cachefiles_cache *cache, char *args)*/ static int cachefiles_daemon_bcull (struct cachefiles_cache *cache,char *args) { -unsigned long bcull; +unsigned int bcull; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - bcull =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &bcull); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -446,14 +451,15 @@ static int cachefiles_daemon_bcull(struct cachefiles_cache *cache, char *args)*/ static int cachefiles_daemon_bstop (struct cachefiles_cache *cache,char *args) { -unsigned long bstop; +unsigned int bstop; +int rv;_enter (",%s" , args); -if (!*args) -return -EINVAL ; - - bstop =simple_strtoul (args, &args,10 ); + rv =parse_integer (args,10 , &bstop); +if (rv <0 ) +return rv; + args += rv;if (args[0 ] !='%' || args[1 ] !='\0' )return -EINVAL ;
@@ -601,21 +607,21 @@ inval:*/ static int cachefiles_daemon_debug (struct cachefiles_cache *cache,char *args) { -unsigned long mask; +unsigned int mask; +int rv;_enter (",%s" , args); - mask =simple_strtoul (args, &args,0 ); -if (args[0 ] !='\0' ) - goto inval; - + rv =parse_integer (args,0 , &mask); +if (rv <0 ) +return rv; +if (args[rv] !='\0' ) { +pr_err ("debug command requires mask\n" ); +return -EINVAL ; + } cachefiles_debug = mask;_leave (" = 0" );return 0 ; - -inval: -pr_err ("debug command requires mask\n" ); -return -EINVAL ; }/*
Signature · 1 line
2.0.4
PATCH 11/12 parse_integer: convert ext2, ext3, ext4
In reply to Alexey DobriyanConvert away from deprecated simple_strto*() interfaces.
--- fs/ext2/super.c | 6 ++++-- fs/ext3/super.c | 7 ++++--- fs/ext4/super.c | 15 +++++++-------- 3 files changed, 15 insertions(+), 13 deletions(-)
fs/ext2/super.c +4 -2
@@ -383,16 +383,18 @@ static unsigned long get_sb_block(void **data) {unsigned long sb_block;char *options = (char *) *data; +int rv;if (!options ||strncmp (options,"sb=" ,3 ) !=0 )return 1 ;/* Default location */ options +=3 ; - sb_block =simple_strtoul (options, &options,0 ); -if (*options && *options !=',' ) { + rv =parse_integer (options,0 , &sb_block); +if (rv <0 || (options[rv] && options[rv] !=',' )) {printk ("EXT2-fs: Invalid sb specification: %s\n" , (char *) *data);return 1 ; } + options += rv;if (*options ==',' ) options++; *data = (void *) options;
fs/ext3/super.c +4 -3
@@ -902,17 +902,18 @@ static ext3_fsblk_t get_sb_block(void **data, struct super_block *sb) {ext3_fsblk_t sb_block;char *options = (char *) *data; +int rv;if (!options ||strncmp (options,"sb=" ,3 ) !=0 )return 1 ;/* Default location */ options +=3 ; -/*todo: use simple_strtoll with >32bit ext3 */ - sb_block =simple_strtoul (options, &options,0 ); -if (*options && *options !=',' ) { + rv =parse_integer (options,0 , &sb_block); +if (rv <0 || (options[rv] && options[rv] !=',' )) {ext3_msg (sb,KERN_ERR ,"error: invalid sb specification: %s" , (char *) *data);return 1 ; } + options += rv;if (*options ==',' ) options++; *data = (void *) options;
fs/ext4/super.c +7 -8
@@ -1210,18 +1210,19 @@ static ext4_fsblk_t get_sb_block(void **data) {ext4_fsblk_t sb_block;char *options = (char *) *data; +int rv;if (!options ||strncmp (options,"sb=" ,3 ) !=0 )return 1 ;/* Default location */ options +=3 ; -/* TODO: use simple_strtoll with >32bit ext4 */ - sb_block =simple_strtoul (options, &options,0 ); -if (*options && *options !=',' ) { + rv =parse_integer (options,0 , &sb_block); +if (rv <0 || (options[rv] && options[rv] !=',' )) {printk (KERN_ERR "EXT4-fs: Invalid sb specification: %s\n" , (char *) *data);return 1 ; } + options += rv;if (*options ==',' ) options++; *data = (void *) options;
@@ -2491,10 +2492,10 @@ static ssize_t inode_readahead_blks_store(struct ext4_attr *a,struct ext4_sb_info *sbi, const char *buf, size_t count) { - unsigned long t; + unsigned int t;int ret; - ret =kstrtoul (skip_spaces (buf),0 , &t); + ret =kstrtouint (skip_spaces (buf),0 , &t);if (ret)return ret;
@@ -2518,13 +2519,11 @@ static ssize_t sbi_ui_store(struct ext4_attr *a, const char *buf, size_t count) { unsigned int *ui = (unsigned int *) (((char *) sbi) + a->u.offset); -unsigned long t;int ret; - ret =kstrtoul (skip_spaces (buf),0 , &t); + ret =kstrtouint (skip_spaces (buf),0 , ui);if (ret)return ret; - *ui = t;return count; }
Signature · 1 line
2.0.4
PATCH 12/12 parse_integer: convert fs/ocfs2/
In reply to Alexey DobriyanConvert away from deprecated simple_strto*() interfaces.
Autodetection of value range from type allows to remove disgusting checks:
if ((major == LONG_MIN) || (major == LONG_MAX) || (major > (u8)-1) || (major < 1)) return -ERANGE; if ((minor == LONG_MIN) || (minor == LONG_MAX) || (minor > (u8)-1) || (minor < 0))
Poof, they're gone!
--- fs/ocfs2/cluster/heartbeat.c | 54 ++++++++++++++++++------------------------ fs/ocfs2/cluster/nodemanager.c | 50 +++++++++++++++++--------------------- fs/ocfs2/stack_user.c | 50 +++++++++++++++++++------------------- 3 files changed, 70 insertions(+), 84 deletions(-)
fs/ocfs2/cluster/heartbeat.c +23 -31
@@ -1484,13 +1484,12 @@ static int o2hb_read_block_input(struct o2hb_region *reg,unsigned long *ret_bytes, unsigned int *ret_bits) { - unsigned long bytes; -char *p = (char *)page; - - bytes =simple_strtoul (p, &p,0 ); -if (!p || (*p && (*p !='\n' ))) -return -EINVAL ; + unsigned int bytes; +int rv; + rv =kstrtouint (page,0 , &bytes); +if (rv <0 ) +return rv;/* Heartbeat and fs min / max block sizes are the same. */ if (bytes >4096 || bytes <512 )return -ERANGE ;
@@ -1543,18 +1542,14 @@ static ssize_t o2hb_region_start_block_write(struct o2hb_region *reg,const char *page, size_t count) { -unsigned long long tmp; -char *p = (char *)page; + int rv;if (reg->hr_bdev)return -EINVAL ; - tmp =simple_strtoull (p, &p,0 ); -if (!p || (*p && (*p !='\n' ))) -return -EINVAL ; - - reg->hr_start_block = tmp; - + rv =kstrtoull (page,0 , ®->hr_start_block); +if (rv <0 ) +return rv;return count; }
@@ -1568,20 +1563,19 @@ static ssize_t o2hb_region_blocks_write(struct o2hb_region *reg,const char *page, size_t count) { - unsigned long tmp; -char *p = (char *)page; + unsigned int tmp; +int rv;if (reg->hr_bdev)return -EINVAL ; - tmp =simple_strtoul (p, &p,0 ); -if (!p || (*p && (*p !='\n' ))) -return -EINVAL ; - + rv =kstrtouint (page,0 , &tmp); +if (rv <0 ) +return rv;if (tmp >O2NM_MAX_NODES || tmp ==0 )return -ERANGE ; - reg->hr_blocks = (unsigned int )tmp; + reg->hr_blocks = tmp;return count; }
@@ -1717,9 +1711,8 @@ static ssize_t o2hb_region_dev_write(struct o2hb_region *reg, size_t count) {struct task_struct *hb_task; -long fd; +int fd;int sectsize; -char *p = (char *)page;struct fd f;struct inode *inode;ssize_t ret = -EINVAL ;
@@ -1733,10 +1726,9 @@ static ssize_t o2hb_region_dev_write(struct o2hb_region *reg,if (o2nm_this_node () ==O2NM_MAX_NODES ) goto out; - fd =simple_strtol (p, &p,0 ); -if (!p || (*p && (*p !='\n' ))) + ret =kstrtoint (page,0 , &fd); +if (ret <0 ) goto out; -if (fd <0 || fd >=INT_MAX ) goto out;
@@ -2210,12 +2202,12 @@ static ssize_t o2hb_heartbeat_group_threshold_store(struct o2hb_heartbeat_groupconst char *page, size_t count) { - unsigned long tmp; -char *p = (char *)page; + unsigned int tmp; +int rv; - tmp =simple_strtoul (p, &p,10 ); -if (!p || (*p && (*p !='\n' ))) -return -EINVAL ; + rv =kstrtouint (page,10 , &tmp); +if (rv <0 ) +return rv;/* this will validate ranges for us. */ o2hb_dead_threshold_set ((unsigned int ) tmp);
fs/ocfs2/cluster/nodemanager.c +22 -28
93 lines not on this page · raw message
fs/ocfs2/stack_user.c +25 -25
99 lines not on this page · raw message
Signature · 1 line
2.0.4