Re: [PATCH v2] kdb: use kstrto* instead of simple_strto

[Daniel Thompson <daniel.thompson@linaro.org>]

On 31/05/15 18:59, Filip Ayazi wrote:
> Replace simple_strto with appropriate kstrto functions as recommended
> by checkpatch, change pid, sig types to unsigned int, int respectively.

A changelog describing the changes are in v2 would be nice here.


> Signed-off-by: Filip Ayazi <filipayazi@gmail.com>
> ---
>   kernel/debug/kdb/kdb_main.c | 56 +++++++++++++++------------------------------
>   1 file changed, 19 insertions(+), 37 deletions(-)

I can't find any bugs introduced by this patch although I do have some 
cosmetic nitpicks below. You can add my reviewed-by to the next release:

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 4121345..6458330 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -296,7 +296,8 @@ static int kdbgetulenv(const char *match, unsigned long *value)
>   	if (strlen(ep) == 0)
>   		return KDB_NOENVVALUE;
>
> -	*value = simple_strtoul(ep, NULL, 0);
> +	if (kstrtoul(ep, 0, value) != 0)
                                   ^^^^^
Checking for != 0 is what if does anyway. A generally clearer code 
pattern for kernel code that returns 0 on success is:

err = kstrtoul(...);
if (err)
	handle_error();


> +		return KDB_BADINT;
>
>   	return 0;
>   }
> @@ -334,20 +335,15 @@ int kdbgetintenv(const char *match, int *value)
>    */
>   int kdbgetularg(const char *arg, unsigned long *value)
>   {
> -	char *endp;
>   	unsigned long val;
>
> -	val = simple_strtoul(arg, &endp, 0);
> -
> -	if (endp == arg) {
> -		/*
> -		 * Also try base 16, for us folks too lazy to type the
> -		 * leading 0x...
> -		 */
> -		val = simple_strtoul(arg, &endp, 16);
> -		if (endp == arg)
> +	/*
> +	 * Also try base 16, for us folks too lazy to type the
> +	 * leading 0x...
> +	 */
> +	if (kstrtoul(arg, 0, &val) != 0)
> +		if (kstrtoul(arg, 16, &val) != 0)
>   			return KDB_BADINT;

Whether or not it is good taste to introduce 'err' here is debatable 
since it makes the code flow pretty verbose. However it would still be 
good to get rid of the '!= 0'.


> -	}
>
>   	*value = val;

kstrtoul() does not write to its argument unless it is successful. That 
means we no longer need a temporary variable here; just pass value into 
kstrtoul().


> @@ -356,17 +352,11 @@ int kdbgetularg(const char *arg, unsigned long *value)
>
>   int kdbgetu64arg(const char *arg, u64 *value)
>   {
> -	char *endp;
>   	u64 val;
>
> -	val = simple_strtoull(arg, &endp, 0);
> -
> -	if (endp == arg) {
> -
> -		val = simple_strtoull(arg, &endp, 16);
> -		if (endp == arg)
> +	if (kstrtoull(arg, 0, &val) != 0)
> +		if (kstrtoull(arg, 16, &val) != 0)
>   			return KDB_BADINT;

+1


> -	}
>
>   	*value = val;
>
> @@ -402,10 +392,9 @@ int kdb_set(int argc, const char **argv)
>   	 */
>   	if (strcmp(argv[1], "KDBDEBUG") == 0) {
>   		unsigned int debugflags;
> -		char *cp;
>
> -		debugflags = simple_strtoul(argv[2], &cp, 0);
> -		if (cp == argv[2] || debugflags & ~KDB_DEBUG_FLAG_MASK) {
> +		if (kstrtouint(argv[2], 0, &debugflags) != 0
> +		    || debugflags & ~KDB_DEBUG_FLAG_MASK) {

+1


>   			kdb_printf("kdb: illegal debug flags '%s'\n",
>   				    argv[2]);
>   			return 0;
> @@ -1588,10 +1577,8 @@ static int kdb_md(int argc, const char **argv)
>   		if (!argv[0][3])
>   			valid = 1;
>   		else if (argv[0][3] == 'c' && argv[0][4]) {
> -			char *p;
> -			repeat = simple_strtoul(argv[0] + 4, &p, 10);
> +			valid = !kstrtoint(argv[0]+4, 10, &repeat);
>   			mdcount = ((repeat * bytesperword) + 15) / 16;
> -			valid = !*p;
>   		}
>   		last_repeat = repeat;

Interesting! Your changes mean we no longer store the, known invalid, 
return value from simple_strtoul into last_repeat. Since last_repeat is 
a static variable and can influence future commands that's a *good* change.


>   	} else if (strcmp(argv[0], "md") == 0)
> @@ -2091,13 +2078,10 @@ static int kdb_dmesg(int argc, const char **argv)
>   	if (argc > 2)
>   		return KDB_ARGCOUNT;
>   	if (argc) {
> -		char *cp;
> -		lines = simple_strtol(argv[1], &cp, 0);
> -		if (*cp)
> +		if (kstrtoint(argv[1], 0, &lines) != 0)
>   			lines = 0;
>   		if (argc > 1) {
> -			adjust = simple_strtoul(argv[2], &cp, 0);
> -			if (*cp || adjust < 0)
> +			if (kstrtoint(argv[2], 0, &adjust) != 0 || adjust < 0)
>   				adjust = 0;

+2


>   		}
>   	}
> @@ -2436,16 +2420,15 @@ static int kdb_help(int argc, const char **argv)
>    */
>   static int kdb_kill(int argc, const char **argv)
>   {
> -	long sig, pid;
> -	char *endp;
> +	unsigned int pid;

Why not use pid_t/kstrtoint() here? The code already has a check to cope 
with the user passing a negative value.


> +	int sig;
>   	struct task_struct *p;
>   	struct siginfo info;
>
>   	if (argc != 2)
>   		return KDB_ARGCOUNT;
>
> -	sig = simple_strtol(argv[1], &endp, 0);
> -	if (*endp)
> +	if (kstrtoint(argv[1], 0, &sig) != 0)
>   		return KDB_BADINT;
>   	if (sig >= 0) {
>   		kdb_printf("Invalid signal parameter.<-signal>\n");
> @@ -2453,8 +2436,7 @@ static int kdb_kill(int argc, const char **argv)
>   	}
>   	sig = -sig;
>
> -	pid = simple_strtol(argv[2], &endp, 0);
> -	if (*endp)
> +	if (kstrtouint(argv[2], 0, &pid) != 0)
>   		return KDB_BADINT;
>   	if (pid <= 0) {
>   		kdb_printf("Process ID must be large than 0.\n");
>