Skip to contentSkip to conversation

kstrto*: converting strings to integers done (hopefully) right

PATCH In review Alexey Dobriyan 3 participants 5 messages Jun 9, 2010 – Dec 6, 2010

Subsystems (2)
Library code Vsprintf
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/2 kstrto*: converting strings to integers done (hopefully) right

1. simple_strto*() suck because they do not contain overflow checks and crufty, libc way to indicate failure.
2. strict_strto*() also do not have overflow checks but the name and comments pretend they do.
3. Both have only "long long" and "long" versions, but users want e. g. strtouint()
4. Both "simple" and "strict" prefixes are wrong:
Simple doesn't say what's so simple, strict should not exist because conversion should be strict by default.

The solution is to use "k" prefix and add convertors for more types.

Enter

kstrtoull(),

	kstrtoll(),
	kstrtoul(),
	kstrtol(),
	kstrtouint(),
	kstrtoint(),
	kstrtou64(),
	kstrtos64(),
	kstrtou32(),
	kstrtos32(),
	kstrtou16(),
	kstrtos16(),
	kstrtou8(),
	kstrtos8()

and, horrors, a runtime testsuite (although incomplete).

Use kstrto*() in your code today!

strict_strto*() become deprecated, stubbed to kstrto*() and eventually removed altogether.

I _hope_ kstrto*() will remain the only family of converters (along with scanf() of course).

P. S.: sizeof() and __alignof__() trick is done to shortcut a function call
when types are indistinguishable.
---

 include/linux/kernel.h |   49 ++++++-
 lib/Kconfig.debug      |    3 +
 lib/Makefile           |    2 +
 lib/kstrtox.c          |  225 ++++++++++++++++++++++++++++++
 lib/test-kstrtox.c     |  356 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c         |  147 --------------------
 6 files changed, 631 insertions(+), 151 deletions(-)
 create mode 100644 lib/kstrtox.c
 create mode 100644 lib/test-kstrtox.c
include/linux/kernel.h +45 -4
index 8317ec4..9006128 100644
@@ -182,14 +182,55 @@ NORET_TYPE void do_exit(long error_code)
 	ATTRIB_NORET;
 NORET_TYPE void complete_and_exit(struct completion *, long)
 	ATTRIB_NORET;
+
+int kstrtoull(const char *s, unsigned int base, unsigned long long *res);
+int kstrtoll(const char *s, unsigned int base, long long *res);
+int _kstrtoul(const char *s, unsigned int base, unsigned long *res);
+static inline int kstrtoul(const char *s, unsigned int base, unsigned long *res)
+{
+	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);
+}
+
+int _kstrtol(const char *s, unsigned int base, long *res);
+static inline int kstrtol(const char *s, unsigned int base, long *res)
+{
+	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 kstrtouint(const char *s, unsigned int base, unsigned int *res);
+int kstrtoint(const char *s, unsigned int base, int *res);
+
+static inline int kstrtou64(const char *s, unsigned int base, u64 *res)
+{
+	return kstrtoull(s, base, res);
+}
+
+static inline int kstrtou32(const char *s, unsigned int base, u32 *res)
+{
+	return kstrtouint(s, base, res);
+}
+
+int kstrtou16(const char *s, unsigned int base, u16 *res);
+int kstrtos16(const char *s, unsigned int base, s16 *res);
+int kstrtou8(const char *s, unsigned int base, u8 *res);
+int kstrtos8(const char *s, unsigned int base, s8 *res);
+
 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);
 extern long long simple_strtoll(const char *,char **,unsigned int);
-extern int strict_strtoul(const char *, unsigned int, unsigned long *);
-extern int strict_strtol(const char *, unsigned int, long *);
-extern int strict_strtoull(const char *, unsigned int, unsigned long long *);
-extern int strict_strtoll(const char *, unsigned int, long long *);
+#define strict_strtoul	kstrtoul
+#define strict_strtol	kstrtol
+#define strict_strtoull	kstrtoull
+#define strict_strtoll	kstrtoll
 extern int sprintf(char * buf, const char * fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern int vsprintf(char *buf, const char *, va_list)
lib/Kconfig.debug +3 -0
index e722e9d..226384f 100644
@@ -1122,3 +1122,6 @@ source "samples/Kconfig"
 source "lib/Kconfig.kgdb"
 
 source "lib/Kconfig.kmemcheck"
+
+config TEST_KSTRTOX
+	tristate "Test kstrto*() family of functions at runtime"
lib/Makefile +2 -0
index 3f1062c..115c917 100644
@@ -22,6 +22,8 @@ lib-y	+= kobject.o kref.o klist.o
 obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
 	 bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o \
 	 string_helpers.o gcd.o lcm.o list_sort.o uuid.o
+obj-y += kstrtox.o
+obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
lib/kstrtox.c new file +225 -0

225 lines not on this page · raw message

lib/test-kstrtox.c new file +356 -0

356 lines not on this page · raw message

lib/vsprintf.c +0 -147

153 lines not on this page · raw message

Signature · 1 line
1.6.4.4
maintainer
#1 lore raw
Quoted 20 lines · On Thu, 10 Jun 2010 00:46:17 +0300 Alexey Dobriyan <[email protected]> wrote:

1. simple_strto*() suck because they do not contain overflow checks and crufty, libc way to indicate failure.
2. strict_strto*() also do not have overflow checks but the name and comments pretend they do.
3. Both have only "long long" and "long" versions, but users want e. g. strtouint()
4. Both "simple" and "strict" prefixes are wrong:

   Simple doesn't say what's so simple, strict should not exist because
   conversion should be strict by default.

   The solution is to use "k" prefix and add convertors for more types.

   Enter

   	kstrtoull(),
	kstrtoll(),
	kstrtoul(),
	kstrtol(),
	kstrtouint(),
	kstrtoint(),
Quoted 8 lines
	kstrtou64(),
	kstrtos64(),
	kstrtou32(),
	kstrtos32(),
	kstrtou16(),
	kstrtos16(),
	kstrtou8(),
	kstrtos8()

aw man, you're killin me. That means 100000 stupid "convert to kstro*" patches and 10000 "please use kstrto*" comments from patch reviewers. Or at least, one patch reviewer :(

For new code I'd like to see a checkpatch "hey dummy you used the old API" rule, please. For old code I think I'd punt any cleanup patches in a Jiriwards direction, OK?

But right now I'm wondering if the problems you've identified are big enough to bother fixing.

#2 lore raw
In reply to Alexey Dobriyan

lib/test-kstrtox.c line 7

Quoted 13 lines · Alexey Dobriyan
lib/test-kstrtox.c new file +7 -0
new file mode 100644
index 0000000..56fda26
@@ -0,0 +1,356 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define for_each_test(i, test) \
+       for (i = 0; i < sizeof(test) / sizeof(test[0]); i++)
+

micronit: Can you use ARRAY_SIZE macro here? Ignore this if it's irrelevant.

author
#3 lore raw
Quoted 15 lines · Thiago Farina

lib/test-kstrtox.c line 7

Quoted 13 lines · Alexey Dobriyan
lib/test-kstrtox.c new file +7 -0
new file mode 100644
index 0000000..56fda26
@@ -0,0 +1,356 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define for_each_test(i, test) \
+       for (i = 0; i < sizeof(test) / sizeof(test[0]); i++)
+

micronit: Can you use ARRAY_SIZE macro here? Ignore this if it's irrelevant.

Dunno,
sizeof(x)/sizeof(x[0]) is really C idiom. And ARRAY_SIZE is misnamed, it doesn't reflect size.

#4 lore raw
Quoted 17 lines · Alexey Dobriyan
Quoted 15 lines · Thiago Farina

lib/test-kstrtox.c line 7

Quoted 13 lines · Alexey Dobriyan
lib/test-kstrtox.c new file +7 -0
new file mode 100644
index 0000000..56fda26
@@ -0,0 +1,356 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define for_each_test(i, test) \
+       for (i = 0; i < sizeof(test) / sizeof(test[0]); i++)
+

micronit: Can you use ARRAY_SIZE macro here? Ignore this if it's irrelevant.

Quoted 2 lines

Dunno,
sizeof(x)/sizeof(x[0]) is really C idiom.

I know.

Quoted 2 lines

And ARRAY_SIZE is misnamed, it doesn't reflect size.

What it reflects so? Isn't the number of elements in the array? What would be a better name?