Patchwork [2/3] ext4: Use kstrtoul() instead of deprecated simple_strtoul()

login
register
mail settings
Submitter Lukas Czerner
Date March 8, 2013, 8:23 a.m.
Message ID <1362730998-3263-2-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/226038/
State Superseded
Headers show

Comments

Lukas Czerner - March 8, 2013, 8:23 a.m.
In parse_strtoul() we're still using deprecated simple_strtoul(). Change
that in favour of kstrtoul().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/super.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)
Theodore Ts'o - March 11, 2013, 2:26 a.m.
On Fri, Mar 08, 2013 at 09:23:17AM +0100, Lukas Czerner wrote:
> In parse_strtoul() we're still using deprecated simple_strtoul(). Change
> that in favour of kstrtoul().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Once you make this change, wouldn't it be better and even more
simplifying to replace the two places where we call parse_strtoul()
with kstrtoul()?

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - March 11, 2013, 7:36 a.m.
On Sun, 10 Mar 2013, Theodore Ts'o wrote:

> Date: Sun, 10 Mar 2013 22:26:35 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 2/3] ext4: Use kstrtoul() instead of deprecated
>     simple_strtoul()
> 
> On Fri, Mar 08, 2013 at 09:23:17AM +0100, Lukas Czerner wrote:
> > In parse_strtoul() we're still using deprecated simple_strtoul(). Change
> > that in favour of kstrtoul().
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> Once you make this change, wouldn't it be better and even more
> simplifying to replace the two places where we call parse_strtoul()
> with kstrtoul()?
> 
> 					- Ted

I think that parse_strtoul() is still useful to have because we
check the "max" value as well and return -EINVAL if it is exceeded.
Removing it we would have to add the check to the callers
where we're using is now, which seems unnecessary, especially since
we might expect more users of the helper.

-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - March 11, 2013, 3:38 p.m.
On Mon, Mar 11, 2013 at 08:36:16AM +0100, Lukáš Czerner wrote:
> I think that parse_strtoul() is still useful to have because we
> check the "max" value as well and return -EINVAL if it is exceeded.
> Removing it we would have to add the check to the callers
> where we're using is now, which seems unnecessary, especially since
> we might expect more users of the helper.

Well, at the moment only one of the two users of parse_strtoul() is
using the "max" value check feature.  I see one other potential user
of parse_strtoul (there's one use of simple_strtoul in get_sb_block()
which wasn't converted in your patch), but it wouldn't need the max
value check feature either.

So I don't have a super strong feeling about this, since this isn't
performance critical code, and it's probably not going to cost a large
amount of object code or stack space, but sometimes extra levels of
abstraction end up hurting more than they help.

Regards,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 620cf56..7bcdef1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2359,14 +2359,12 @@  struct ext4_attr {
 static int parse_strtoul(const char *buf,
 		unsigned long max, unsigned long *value)
 {
-	char *endp;
-
-	*value = simple_strtoul(skip_spaces(buf), &endp, 0);
-	endp = skip_spaces(endp);
-	if (*endp || *value > max)
-		return -EINVAL;
+	int ret;
 
-	return 0;
+	ret = kstrtoul(skip_spaces(buf), 0, value);
+	if (!ret && *value > max)
+		ret = -EINVAL;
+	return ret;
 }
 
 static ssize_t delayed_allocation_blocks_show(struct ext4_attr *a,