[PATCH 1/2] sunrpc/cache.h: fix coding style

[PATCH 1/2] sunrpc/cache.h: fix coding style

[Eldad Zack]

Neaten code style in get_int()

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 include/linux/sunrpc/cache.h |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index f5fd616..0757887 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -220,10 +220,16 @@ static inline int get_int(char **bpp, int *anint)
 	char *ep;
 	int rv;
 	int len = qword_get(bpp, buf, 50);
-	if (len < 0) return -EINVAL;
-	if (len ==0) return -ENOENT;
+
+	if (len < 0)
+		return -EINVAL;
+	if (len == 0)
+		return -ENOENT;
+
 	rv = simple_strtol(buf, &ep, 0);
-	if (*ep) return -EINVAL;
+	if (*ep)
+		return -EINVAL;
+
 	*anint = rv;
 	return 0;
 }
-- 
1.7.10

[PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[Eldad Zack]

This patch replaces the usage of simple_strtoul with kstrtoint in
get_int().

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 include/linux/sunrpc/cache.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index 0757887..9a60bcc 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -217,7 +217,7 @@ extern int qword_get(char **bpp, char *dest, int bufsize);
 static inline int get_int(char **bpp, int *anint)
 {
 	char buf[50];
-	char *ep;
+	ssize_t ret;
 	int rv;
 	int len = qword_get(bpp, buf, 50);
 
@@ -226,8 +226,8 @@ static inline int get_int(char **bpp, int *anint)
 	if (len == 0)
 		return -ENOENT;
 
-	rv = simple_strtol(buf, &ep, 0);
-	if (*ep)
+	ret = kstrtoint(buf, 0, &rv);
+	if (ret)
 		return -EINVAL;
 
 	*anint = rv;
-- 
1.7.10

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[Joe Perches]

On Thu, 2012-07-05 at 01:00 +0200, Eldad Zack wrote:
> This patch replaces the usage of simple_strtoul with kstrtoint in
> get_int().

Just some trivia:

> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
[]
> @@ -217,7 +217,7 @@ extern int qword_get(char **bpp, char *dest, int bufsize);
>  static inline int get_int(char **bpp, int *anint)
>  {
>  	char buf[50];
> -	char *ep;
> +	ssize_t ret;
>  	int rv;
>  	int len = qword_get(bpp, buf, 50);

This would be nicer as
	int len = qword_get(bpp, buf, sizeof(buf));

> @@ -226,8 +226,8 @@ static inline int get_int(char **bpp, int *anint)
>  	if (len == 0)
>  		return -ENOENT;
>  
> -	rv = simple_strtol(buf, &ep, 0);
> -	if (*ep)
> +	ret = kstrtoint(buf, 0, &rv);
> +	if (ret)
>  		return -EINVAL;
>  
>  	*anint = rv;

ret and rv aren't useful anymore.

Perhaps:

	if (kstrtoint(buf, 0, anint))
		return -EINVAL;

	return 0;

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[Eldad Zack]

On Wed, 4 Jul 2012, Joe Perches wrote:
> On Thu, 2012-07-05 at 01:00 +0200, Eldad Zack wrote:
> > This patch replaces the usage of simple_strtoul with kstrtoint in
> > get_int().
> 
> >  	int len = qword_get(bpp, buf, 50);
> This would be nicer as
> 	int len = qword_get(bpp, buf, sizeof(buf));

I agree, thanks!

> > @@ -226,8 +226,8 @@ static inline int get_int(char **bpp, int *anint)
> > +	ret = kstrtoint(buf, 0, &rv);
> > +	if (ret)
> >  		return -EINVAL;
> >  
> >  	*anint = rv;
> ret and rv aren't useful anymore.
> 
> Perhaps:
> 
> 	if (kstrtoint(buf, 0, anint))
> 		return -EINVAL;
> 
> 	return 0;

Yes, that works better, since kstrtoint() will not write to anint 
unless it's successful, so the whole business with rv is 
indeed redundant.

Looks much neater now thanks to your comments - I will resend it 
a bit later. Should I add you as Signed-off-by?

Eldad

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[J. Bruce Fields]

On Thu, Jul 05, 2012 at 01:00:26AM +0200, Eldad Zack wrote:
> This patch replaces the usage of simple_strtoul with kstrtoint in
> get_int().

Why?

--b.

> 
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  include/linux/sunrpc/cache.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index 0757887..9a60bcc 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -217,7 +217,7 @@ extern int qword_get(char **bpp, char *dest, int bufsize);
>  static inline int get_int(char **bpp, int *anint)
>  {
>  	char buf[50];
> -	char *ep;
> +	ssize_t ret;
>  	int rv;
>  	int len = qword_get(bpp, buf, 50);
>  
> @@ -226,8 +226,8 @@ static inline int get_int(char **bpp, int *anint)
>  	if (len == 0)
>  		return -ENOENT;
>  
> -	rv = simple_strtol(buf, &ep, 0);
> -	if (*ep)
> +	ret = kstrtoint(buf, 0, &rv);
> +	if (ret)
>  		return -EINVAL;
>  
>  	*anint = rv;
> -- 
> 1.7.10
> 

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[Eldad Zack]

On Wed, 4 Jul 2012, J. Bruce Fields wrote:
> On Thu, Jul 05, 2012 at 01:00:26AM +0200, Eldad Zack wrote:
> > This patch replaces the usage of simple_strtoul with kstrtoint in
> > get_int().
> 
> Why?

Since the simple_str* family don't account for overflow and AFAIK
deprecated. Also, in this specific case, the long from strtol is 
silently converted to an int by the caller.

Eldad

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[J. Bruce Fields]

On Thu, Jul 05, 2012 at 09:04:03PM +0200, Eldad Zack wrote:
> 
> On Wed, 4 Jul 2012, J. Bruce Fields wrote:
> > On Thu, Jul 05, 2012 at 01:00:26AM +0200, Eldad Zack wrote:
> > > This patch replaces the usage of simple_strtoul with kstrtoint in
> > > get_int().
> > 
> > Why?
> 
> Since the simple_str* family don't account for overflow and AFAIK
> deprecated. Also, in this specific case, the long from strtol is 
> silently converted to an int by the caller.

OK, thanks.  Please use that as the changelog when you resend.  ("This
patch replaces..." tells me nothing I couldn't figure out from looking
at the patch.)

--b.

Re: [PATCH 2/2] sunrpc/cache.h: replace simple_strtoul

[Eldad Zack]

On Thu, 5 Jul 2012, J. Bruce Fields wrote:

> On Thu, Jul 05, 2012 at 09:04:03PM +0200, Eldad Zack wrote:
> > 
> > On Wed, 4 Jul 2012, J. Bruce Fields wrote:
> > > On Thu, Jul 05, 2012 at 01:00:26AM +0200, Eldad Zack wrote:
> > > > This patch replaces the usage of simple_strtoul with kstrtoint in
> > > > get_int().
> > > 
> > > Why?
> > 
> > Since the simple_str* family don't account for overflow and AFAIK
> > deprecated. Also, in this specific case, the long from strtol is 
> > silently converted to an int by the caller.
> 
> OK, thanks.  Please use that as the changelog when you resend.  ("This
> patch replaces..." tells me nothing I couldn't figure out from looking
> at the patch.)

Will do, sorry about that.

Eldad