staging: speakup: replace simple_strtoul with kstrtoul

PATCH In review samuel kihahu 2 participants 6 messages Dec 17, 2014 – Dec 18, 2014 Staging

lore

Share & tools

lore

samuel kihahu

author

2014-12-17 11:56 UTC #0 lore raw

PATCH 1/1 staging: speakup: replace simple_strtoul with kstrtoul

Replacing obsolete simple_strtoul with kstrtoul.

Signed-off-by samuel kihahu <[email protected]>

---
 drivers/staging/speakup/varhandlers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/speakup/varhandlers.c +1 -1

index d758284..b526c8e 100644
@@ -323,7 +323,7 @@ char *spk_s2uchar(char *start, char *dest)
 {
 	int val = 0;
 
-	val = simple_strtoul(skip_spaces(start), &start, 10);
+	val = kstrtoul(skip_spaces(start), &start, 10);
 	if (*start == ',')
 		start++;
 	*dest = (u_char)val;

Signature · 1 line

1.8.3.1

Dan Carpenter

2014-12-17 12:11 UTC #1 lore raw

Quoted 2 lines · samuel kihahu

Replacing obsolete simple_strtoul with kstrtoul.

Nope. That's wrong. Learn how the functions are different beyond just the name.

regards,
dan carpenter

samuel kihahu

author

2014-12-17 13:44 UTC #2 lore raw

Quoted 6 lines · Dan Carpenter

Quoted 2 lines · samuel kihahu
Replacing obsolete simple_strtoul with kstrtoul.
Nope. That's wrong. Learn how the functions are different beyond just the name.

Noted, have made corrections to fit the kstrtoul and handle the return value.

Quoted 1 line

From 71d2f7123e697b95371585b4af9a0b35262307ea Mon Sep 17 00:00:00 2001

From: samuel kihahu <[email protected]>
Date: Wed, 17 Dec 2014 16:31:05 +0300
Subject: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

Replacing obsolete simple_strtoul with kstrtoul and checking the return status.

Signed-off-by samuel kihahu <[email protected]>

---
 drivers/staging/speakup/varhandlers.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/staging/speakup/varhandlers.c +3 -2

index d758284..8ac9ad7 100644
@@ -321,9 +321,10 @@ char *spk_strlwr(char *s)
 
 char *spk_s2uchar(char *start, char *dest)
 {
-	int val = 0;
+	unsigned long val;
 
-	val = simple_strtoul(skip_spaces(start), &start, 10);
+	if (kstrtoul(skip_spaces(start), &start, val))
+		return -EINVAL;
 	if (*start == ',')
 		start++;
 	*dest = (u_char)val;

Signature · 1 line

1.8.3.1

Dan Carpenter

2014-12-17 14:03 UTC #3 lore raw

Quoted 9 lines · samuel kihahu

Quoted 6 lines · Dan Carpenter
Quoted 2 lines · samuel kihahu
Replacing obsolete simple_strtoul with kstrtoul.
Nope. That's wrong. Learn how the functions are different beyond just the name.
Noted, have made corrections to fit the kstrtoul and handle the return value.

You have to compile test these things. Really kernel programming is not a good way to learn how to program. :(

regards,
dan carpenter

samuel kihahu

author

2014-12-18 14:48 UTC #4 lore raw

Quoted 8 lines · Dan Carpenter

Quoted 7 lines · samuel kihahu
Quoted 6 lines · Dan Carpenter
Quoted 2 lines · samuel kihahu
Replacing obsolete simple_strtoul with kstrtoul.
Nope. That's wrong. Learn how the functions are different beyond just the name.

Quoted 9 lines

Quoted 2 lines
Noted, have made corrections to fit the kstrtoul and handle the return value.
You have to compile test these things. Really kernel programming is not a good way to learn how to program. :(
regards,
dan carpenter

Appreciate the feedback, have modified the patch, tested and confirmed it builds.

Quoted 1 line

From 3d39fd533b4a75e567f76f07f5bc978adddb0721 Mon Sep 17 00:00:00 2001

From: samuel kihahu <[email protected]>
Date: Thu, 18 Dec 2014 17:42:37 +0300
Subject: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

Replacing obsolete simple_strtoul with kstrtoul, checking and returning the correct error code.

Signed-off-by samuel kihahu <[email protected]>

---
 drivers/staging/speakup/varhandlers.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/staging/speakup/varhandlers.c +6 -2

index d758284..716ccc1 100644
@@ -321,9 +321,13 @@ char *spk_strlwr(char *s)
 
 char *spk_s2uchar(char *start, char *dest)
 {
-	int val = 0;
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(skip_spaces(start), 10, &val);
+	if (IS_ERR(&ret))
+		return ERR_CAST(&ret);
 
-	val = simple_strtoul(skip_spaces(start), &start, 10);
 	if (*start == ',')
 		start++;
 	*dest = (u_char)val;

Signature · 1 line

1.8.3.1

Dan Carpenter

2014-12-18 21:25 UTC #5 lore raw

Quoted 15 lines · samuel kihahu

Quoted 14 lines · Dan Carpenter
Quoted 9 lines · samuel kihahu
Quoted 6 lines · Dan Carpenter
Quoted 2 lines · samuel kihahu
Replacing obsolete simple_strtoul with kstrtoul.
Nope. That's wrong. Learn how the functions are different beyond just the name.
Noted, have made corrections to fit the kstrtoul and handle the return value.
You have to compile test these things. Really kernel programming is not a good way to learn how to program. :(

Quoted 6 lines

Quoted 3 lines
regards,
dan carpenter
Appreciate the feedback, have modified the patch, tested and confirmed it builds.

No. It's still really wrong. One of the key differences between kstrtoul() and simple_strtoul() is that simple_strtoul() gives you a pointer to the end of the string.

It's actually best to use simple_strtoul() here. Checkpatch.pl is wrong.

As well the new error handling doesn't work at all.

regards,
dan carpenter