linux-kernel - Re: [PATCH V2] replace strict_strtoul to kstrto[*] and check return value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 07 Aug 2011 17:36:50 +0000
From:	Florian Tobias Schandinat <FlorianSchandinat@....de>
To:	stufever@...il.com
CC:	linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
	lethal@...ux-sh.org, Wang Shaoyan <wangshaoyan.pt@...bao.com>
Subject: Re: [PATCH V2] replace strict_strtoul to kstrto[*] and check return
 value

Hi Wang,

On 08/07/2011 03:28 PM, stufever@...il.com wrote:
> From: Wang Shaoyan<wangshaoyan.pt@...bao.com>
>
> Change since V1:
>    1.use kstrto*, not kstrtoul
>    2.replace&buf[0] to buf

it is good that you wrote what you changed. But it would be better if you write 
it below the commit message like this

<commit message>

Signed-off-by:
---
    drivers/video/via/viafbdev.c |  126 ++++++++++++++++++++++-------------------
    1 files changed, 68 insertions(+), 58 deletions(-)

Change since V1:

as nobody really cares what changed in different patch versions once the final 
version is applied in git. And if you do it this way I could just do "git am -s 
<patch>" and would get the commit message without the history.

> This commit replace the function strict_strtoul(becasue commit 33ee3b2e), and check the return value to avoid such warning:
>
>    drivers/video/via/viafbdev.c:1992: warning: ignoring return value of 'kstrtoul', declared with attribute warn_unused_result

Thanks this looks much saner. Just 2 little issues before it is good enough, I 
think.

./scripts/checkpatch.pl ./\[PATCH\ V2\]\ replace\ strict_strtoul\ to\ 
kstrto\[\*\]\ and\ check\ return\ value.eml
ERROR: trailing whitespace
#173: FILE: drivers/video/via/viafbdev.c:1974:
+^I^I^Iif (kstrtoint(this_opt + 21, 0, $

ERROR: trailing whitespace
#177: FILE: drivers/video/via/viafbdev.c:1978:
+^I^I^Iif (kstrtoint(this_opt + 19, 0, $

ERROR: trailing whitespace
#212: FILE: drivers/video/via/viafbdev.c:1991:
+^I^I^Iif (kstrtoint(this_opt + 30, 0, $

ERROR: trailing whitespace
#220: FILE: drivers/video/via/viafbdev.c:1999:
+^I^I^Iif (kstrtoint(this_opt + 24, 0, $

total: 4 errors, 0 warnings, 173 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
       scripts/cleanfile

./[PATCH V2] replace strict_strtoul to kstrto[*] and check return value.eml has 
style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Can you please fix those?


> Signed-off-by: Wang Shaoyan<wangshaoyan.pt@...bao.com>
> ---
>   drivers/video/via/viafbdev.c |  126 ++++++++++++++++++++++-------------------
>   1 files changed, 68 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> index 53aa443..7074fc7 100644
> --- a/drivers/video/via/viafbdev.c
> +++ b/drivers/video/via/viafbdev.c

[snip]

> @@ -1325,7 +1328,8 @@ static ssize_t viafb_dfpl_proc_write(struct file *file,
>   	if (copy_from_user(&buf[0], buffer, length))
>   		return -EFAULT;
>   	buf[length - 1] = '\0';	/*Ensure end string */
> -	strict_strtoul(&buf[0], 0, (unsigned long *)&reg_val);
> +	if (kstrtou8(buf, -1,&reg_val)<  0)
> +		return -EINVAL;
>   	viafb_write_reg_mask(CR99, VIACR, reg_val, 0x0f);
>   	return count;
>   }

Using -1 instead 0 as base here? Why? What does this cause?
I assume that it should be 0.


Thanks,

Florian Tobias Schandinat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/