[PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[samuel kihahu]

Replacing obsolete simple_strtoul with kstrtoul.

Signed-off-by: samuel kihahu <skihahu@gmail.com>
---
 drivers/staging/speakup/varhandlers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c
index d758284..b526c8e 100644
--- a/drivers/staging/speakup/varhandlers.c
+++ b/drivers/staging/speakup/varhandlers.c
@@ -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;
-- 
1.8.3.1

Re: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[Dan Carpenter]

On Wed, Dec 17, 2014 at 02:56:02PM +0300, samuel kihahu wrote:
> Replacing obsolete simple_strtoul with kstrtoul.
> 

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

regards,
dan carpenter

Re: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[samuel kihahu]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On Wed, Dec 17, 2014 at 03:11:19PM +0300, Dan Carpenter wrote:
> On Wed, Dec 17, 2014 at 02:56:02PM +0300, samuel kihahu wrote:
> > 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.

[-- Attachment #2: 0001-staging-speakup-replace-simple_strtoul-with-kstrtoul.patch --]
[-- Type: text/plain, Size: 992 bytes --]

>From 71d2f7123e697b95371585b4af9a0b35262307ea Mon Sep 17 00:00:00 2001
From: samuel kihahu <skihahu@gmail.com>
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 <skihahu@gmail.com>
---
 drivers/staging/speakup/varhandlers.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c
index d758284..8ac9ad7 100644
--- a/drivers/staging/speakup/varhandlers.c
+++ b/drivers/staging/speakup/varhandlers.c
@@ -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;
-- 
1.8.3.1

Re: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[Dan Carpenter]

On Wed, Dec 17, 2014 at 04:43:54PM +0300, samuel kihahu wrote:
> On Wed, Dec 17, 2014 at 03:11:19PM +0300, Dan Carpenter wrote:
> > On Wed, Dec 17, 2014 at 02:56:02PM +0300, samuel kihahu wrote:
> > > 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

Re: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[samuel kihahu]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Wed, Dec 17, 2014 at 05:03:22PM +0300, Dan Carpenter wrote:
> On Wed, Dec 17, 2014 at 04:43:54PM +0300, samuel kihahu wrote:
> > On Wed, Dec 17, 2014 at 03:11:19PM +0300, Dan Carpenter wrote:
> > > On Wed, Dec 17, 2014 at 02:56:02PM +0300, samuel kihahu wrote:
> > > > 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
> 

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

[-- Attachment #2: 0001-staging-speakup-replace-simple_strtoul-with-kstrtoul.patch --]
[-- Type: text/plain, Size: 1050 bytes --]

>From 3d39fd533b4a75e567f76f07f5bc978adddb0721 Mon Sep 17 00:00:00 2001
From: samuel kihahu <skihahu@gmail.com>
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 <skihahu@gmail.com>
---
 drivers/staging/speakup/varhandlers.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c
index d758284..716ccc1 100644
--- a/drivers/staging/speakup/varhandlers.c
+++ b/drivers/staging/speakup/varhandlers.c
@@ -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;
-- 
1.8.3.1

Re: [PATCH] staging: speakup: replace simple_strtoul with kstrtoul

[Dan Carpenter]

On Thu, Dec 18, 2014 at 05:48:10PM +0300, samuel kihahu wrote:
> On Wed, Dec 17, 2014 at 05:03:22PM +0300, Dan Carpenter wrote:
> > On Wed, Dec 17, 2014 at 04:43:54PM +0300, samuel kihahu wrote:
> > > On Wed, Dec 17, 2014 at 03:11:19PM +0300, Dan Carpenter wrote:
> > > > On Wed, Dec 17, 2014 at 02:56:02PM +0300, samuel kihahu wrote:
> > > > > 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
> > 
> 
> 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

[PATCH] staging: speakup: Replace simple_strtoul with kstrtoul

[Bharath Vedartham]

simple_strtoul is obsolete. Change it to kstrtoul. Kernel is building
and booting successfully.

Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com>
---
 drivers/staging/speakup/kobjects.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 2e36d87..ad5a2b2 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -787,7 +787,9 @@ static ssize_t message_store_helper(const char *buf, size_t count,
 			continue;
 		}
 
-		index = simple_strtoul(cp, &temp, 10);
+		retval = kstrtoul(cp, 0, &index);
+		if (retval)
+			return retval;
 
 		while ((temp < linefeed) && (*temp == ' ' || *temp == '\t'))
 			temp++;
-- 
2.7.4

Re: [PATCH] staging: speakup: Replace simple_strtoul with kstrtoul

[Samuel Thibault]

Bharath Vedartham, le dim. 24 févr. 2019 01:01:21 +0530, a ecrit:
> simple_strtoul is obsolete. Change it to kstrtoul. Kernel is building
> and booting successfully.

Please recheck your patch, temp is used after the simple_strtoul call.

Samuel

Re: [PATCH] staging: speakup: Replace simple_strtoul with kstrtoul

[Bharath Vedartham]

On Sat, Feb 23, 2019 at 08:37:50PM +0100, Samuel Thibault wrote:
> Bharath Vedartham, le dim. 24 févr. 2019 01:01:21 +0530, a ecrit:
> > simple_strtoul is obsolete. Change it to kstrtoul. Kernel is building
> > and booting successfully.
> 
> Please recheck your patch, temp is used after the simple_strtoul call.
> 
> Samuel

Sorry about that. After some thought, I feel this is a special case in
replacing simple_strtoul to kstrtoul. simple_strtoul can give back a
pointer to the end of the parsed string, whereas kstrtoul does not
give any thing of that kind. In our case, temp is the end of the
parsed cp string. 

To replace simple_strtoul to kstrtoul, we need to know the length of the
unsigned long int returned and shift the cp pointer by that length to
get temp. This might involve a bit of code refactoring.

Bharath