[PATCH 0/5] Command line related cleanups

[PATCH 0/5] Command line related cleanups

[Felipe Contreras]

Hi,

These became apparent in the review process of a new command line parameter.

Felipe Contreras (5):
  kstrtox: remove redundant cleanup
  cmdline: fix style issues
  cmdline: declare exported symbols immediately
  kstrtox: remove redundant casts
  params: improve standard definitions

 kernel/params.c | 25 +++++++++----------------
 lib/cmdline.c   | 14 ++++++--------
 lib/kstrtox.c   | 17 ++++++++---------
 3 files changed, 23 insertions(+), 33 deletions(-)

-- 
1.8.4.2+fc1

[PATCH 1/5] kstrtox: remove redundant cleanup

[Felipe Contreras]

We can't reach the cleanup code unless the flag KSTRTOX_OVERFLOW is not
set, so there's not no point in clearing a bit that we know is not set.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 lib/kstrtox.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index f78ae0c..ec8da78 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -92,7 +92,6 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 	rv = _parse_integer(s, base, &_res);
 	if (rv & KSTRTOX_OVERFLOW)
 		return -ERANGE;
-	rv &= ~KSTRTOX_OVERFLOW;
 	if (rv == 0)
 		return -EINVAL;
 	s += rv;
-- 
1.8.4.2+fc1

Re: [PATCH 1/5] kstrtox: remove redundant cleanup

[Levente Kurusa]

2013-11-16 18:32 keltezéssel, Felipe Contreras írta:
> We can't reach the cleanup code unless the flag KSTRTOX_OVERFLOW is not
> set, so there's not no point in clearing a bit that we know is not set.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Acked-by: Levente Kurusa <levex@linux.com>

Legit one. To be honest, I don't know who will apply it, because of the stuff discussed
earlier.

-- 
Regards,
Levente Kurusa

[PATCH 2/5] cmdline: fix style issues

[Felipe Contreras]

WARNING: space prohibited between function name and open parenthesis '('
+int get_option (char **str, int *pint)

WARNING: space prohibited between function name and open parenthesis '('
+	*pint = simple_strtol (cur, str, 0);

ERROR: trailing whitespace
+ $

WARNING: please, no spaces at the start of a line
+ $

WARNING: space prohibited between function name and open parenthesis '('
+		res = get_option ((char **)&str, ints + i);

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 lib/cmdline.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index eb67911..5466333 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -49,13 +49,13 @@ static int get_range(char **str, int *pint)
  *	3 - hyphen found to denote a range
  */
 
-int get_option (char **str, int *pint)
+int get_option(char **str, int *pint)
 {
 	char *cur = *str;
 
 	if (!cur || !(*cur))
 		return 0;
-	*pint = simple_strtol (cur, str, 0);
+	*pint = simple_strtol(cur, str, 0);
 	if (cur == *str)
 		return 0;
 	if (**str == ',') {
@@ -84,13 +84,13 @@ int get_option (char **str, int *pint)
  *	the parse to end (typically a null terminator, if @str is
  *	completely parseable).
  */
- 
+
 char *get_options(const char *str, int nints, int *ints)
 {
 	int res, i = 1;
 
 	while (i < nints) {
-		res = get_option ((char **)&str, ints + i);
+		res = get_option((char **)&str, ints + i);
 		if (res == 0)
 			break;
 		if (res == 3) {
@@ -153,7 +153,6 @@ unsigned long long memparse(const char *ptr, char **retptr)
 	return ret;
 }
 
-
 EXPORT_SYMBOL(memparse);
 EXPORT_SYMBOL(get_option);
 EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

[PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(memparse);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_option);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_options);

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 lib/cmdline.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5466333..d4932f7 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
 
 	return 1;
 }
+EXPORT_SYMBOL(get_option);
 
 /**
  *	get_options - Parse a string into a list of integers
@@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
 	ints[0] = i - 1;
 	return (char *)str;
 }
+EXPORT_SYMBOL(get_options);
 
 /**
  *	memparse - parse a string with mem suffixes into a number
@@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char **retptr)
 
 	return ret;
 }
-
 EXPORT_SYMBOL(memparse);
-EXPORT_SYMBOL(get_option);
-EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

Re: [PATCH 3/5] cmdline: declare exported symbols immediately

[Levente Kurusa]

2013-11-16 18:32 keltezéssel, Felipe Contreras írta:
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> +EXPORT_SYMBOL(memparse);
> 
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> +EXPORT_SYMBOL(get_option);
> 
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> +EXPORT_SYMBOL(get_options);
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  lib/cmdline.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 5466333..d4932f7 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
>  
>  	return 1;
>  }
> +EXPORT_SYMBOL(get_option);
>  
>  /**
>   *	get_options - Parse a string into a list of integers
> @@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
>  	ints[0] = i - 1;
>  	return (char *)str;
>  }
> +EXPORT_SYMBOL(get_options);
>  
>  /**
>   *	memparse - parse a string with mem suffixes into a number
> @@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char **retptr)
>  
>  	return ret;
>  }
> -
>  EXPORT_SYMBOL(memparse);
> -EXPORT_SYMBOL(get_option);
> -EXPORT_SYMBOL(get_options);
> 

I don't know about this one, but I have seen lots of files where EXPORT_SYMBOLs were
listed at the end of the file. To avoid misunderstanding, I still think that having the
exports after the function is more appropriate.

-- 
Regards,
Levente Kurusa

Re: [PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

On Sat, Nov 16, 2013 at 2:21 PM, Levente Kurusa <levex@linux.com> wrote:
> 2013-11-16 18:32 keltezéssel, Felipe Contreras írta:
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(memparse);
>>
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(get_option);
>>
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(get_options);
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  lib/cmdline.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/cmdline.c b/lib/cmdline.c
>> index 5466333..d4932f7 100644
>> --- a/lib/cmdline.c
>> +++ b/lib/cmdline.c
>> @@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
>>
>>       return 1;
>>  }
>> +EXPORT_SYMBOL(get_option);
>>
>>  /**
>>   *   get_options - Parse a string into a list of integers
>> @@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
>>       ints[0] = i - 1;
>>       return (char *)str;
>>  }
>> +EXPORT_SYMBOL(get_options);
>>
>>  /**
>>   *   memparse - parse a string with mem suffixes into a number
>> @@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char **retptr)
>>
>>       return ret;
>>  }
>> -
>>  EXPORT_SYMBOL(memparse);
>> -EXPORT_SYMBOL(get_option);
>> -EXPORT_SYMBOL(get_options);
>>
>
> I don't know about this one, but I have seen lots of files where EXPORT_SYMBOLs were
> listed at the end of the file. To avoid misunderstanding, I still think that having the
> exports after the function is more appropriate.

If that was appropriate then checkpatch should be updated to remove
that warning, but presumably it's desirable to have them one next to
the other.

-- 
Felipe Contreras

[PATCH 4/5] kstrtox: remove redundant casts

[Felipe Contreras]

The temporary variable is of the same type as the cast, so it's
redundant.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 lib/kstrtox.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index ec8da78..649b74b 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -176,7 +176,7 @@ int _kstrtoul(const char *s, unsigned int base, unsigned long *res)
 	rv = kstrtoull(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (unsigned long long)(unsigned long)tmp)
+	if (tmp != (unsigned long)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -192,7 +192,7 @@ int _kstrtol(const char *s, unsigned int base, long *res)
 	rv = kstrtoll(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (long long)(long)tmp)
+	if (tmp != (long)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -223,7 +223,7 @@ int kstrtouint(const char *s, unsigned int base, unsigned int *res)
 	rv = kstrtoull(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (unsigned long long)(unsigned int)tmp)
+	if (tmp != (unsigned int)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -254,7 +254,7 @@ int kstrtoint(const char *s, unsigned int base, int *res)
 	rv = kstrtoll(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (long long)(int)tmp)
+	if (tmp != (int)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -269,7 +269,7 @@ int kstrtou16(const char *s, unsigned int base, u16 *res)
 	rv = kstrtoull(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (unsigned long long)(u16)tmp)
+	if (tmp != (u16)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -284,7 +284,7 @@ int kstrtos16(const char *s, unsigned int base, s16 *res)
 	rv = kstrtoll(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (long long)(s16)tmp)
+	if (tmp != (s16)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -299,7 +299,7 @@ int kstrtou8(const char *s, unsigned int base, u8 *res)
 	rv = kstrtoull(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (unsigned long long)(u8)tmp)
+	if (tmp != (u8)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
@@ -314,7 +314,7 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
 	rv = kstrtoll(s, base, &tmp);
 	if (rv < 0)
 		return rv;
-	if (tmp != (long long)(s8)tmp)
+	if (tmp != (s8)tmp)
 		return -ERANGE;
 	*res = tmp;
 	return 0;
-- 
1.8.4.2+fc1

[PATCH 5/5] params: improve standard definitions

[Felipe Contreras]

We are repeating the functionality of kstrtol in param_set_long, and the
same for kstrtoint. We can get rid of the extra code by using the right
functions.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 kernel/params.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index c00d5b5..48e1a81 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -227,17 +227,10 @@ int parse_args(const char *doing,
 }
 
 /* Lazy bastard, eh? */
-#define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
+#define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
 	int param_set_##name(const char *val, const struct kernel_param *kp) \
 	{								\
-		tmptype l;						\
-		int ret;						\
-									\
-		ret = strtolfn(val, 0, &l);				\
-		if (ret < 0 || ((type)l != l))				\
-			return ret < 0 ? ret : -EINVAL;			\
-		*((type *)kp->arg) = l;					\
-		return 0;						\
+		return strtolfn(val, 0, (type *)kp->arg);		\
 	}								\
 	int param_get_##name(char *buffer, const struct kernel_param *kp) \
 	{								\
@@ -253,13 +246,13 @@ int parse_args(const char *doing,
 	EXPORT_SYMBOL(param_ops_##name)
 
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, kstrtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, kstrtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, kstrtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
+STANDARD_PARAM_DEF(short, short, "%hi", kstrtos16);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", kstrtou16);
+STANDARD_PARAM_DEF(int, int, "%i", kstrtoint);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", kstrtouint);
+STANDARD_PARAM_DEF(long, long, "%li", kstrtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", kstrtoul);
 
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
-- 
1.8.4.2+fc1

Re: [PATCH 5/5] params: improve standard definitions

[Rusty Russell]

Felipe Contreras <felipe.contreras@gmail.com> writes:
> We are repeating the functionality of kstrtol in param_set_long, and the
> same for kstrtoint. We can get rid of the extra code by using the right
> functions.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Thanks, they're new since this code was written.  I didn't know about them...

Applied!
Rusty.

> ---
>  kernel/params.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index c00d5b5..48e1a81 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -227,17 +227,10 @@ int parse_args(const char *doing,
>  }
>  
>  /* Lazy bastard, eh? */
> -#define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)      	\
> +#define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
>  	int param_set_##name(const char *val, const struct kernel_param *kp) \
>  	{								\
> -		tmptype l;						\
> -		int ret;						\
> -									\
> -		ret = strtolfn(val, 0, &l);				\
> -		if (ret < 0 || ((type)l != l))				\
> -			return ret < 0 ? ret : -EINVAL;			\
> -		*((type *)kp->arg) = l;					\
> -		return 0;						\
> +		return strtolfn(val, 0, (type *)kp->arg);		\
>  	}								\
>  	int param_get_##name(char *buffer, const struct kernel_param *kp) \
>  	{								\
> @@ -253,13 +246,13 @@ int parse_args(const char *doing,
>  	EXPORT_SYMBOL(param_ops_##name)
>  
>  
> -STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, kstrtoul);
> -STANDARD_PARAM_DEF(short, short, "%hi", long, kstrtol);
> -STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, kstrtoul);
> -STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
> -STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, kstrtoul);
> -STANDARD_PARAM_DEF(long, long, "%li", long, kstrtol);
> -STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, kstrtoul);
> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
> +STANDARD_PARAM_DEF(short, short, "%hi", kstrtos16);
> +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", kstrtou16);
> +STANDARD_PARAM_DEF(int, int, "%i", kstrtoint);
> +STANDARD_PARAM_DEF(uint, unsigned int, "%u", kstrtouint);
> +STANDARD_PARAM_DEF(long, long, "%li", kstrtol);
> +STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", kstrtoul);
>  
>  int param_set_charp(const char *val, const struct kernel_param *kp)
>  {
> -- 
> 1.8.4.2+fc1