Re: [PATCH 3/4] gcov: add gcov profiling infrastructure

Peter Oberparleiter
2009-06-04T08:27:25Z

Andrew Morton wrote:
> On Wed, 03 Jun 2009 17:26:22 +0200
> Peter Oberparleiter <oberpar@linux.vnet.ibm.com> wrote:
>
>>> Right - the sscanf would make sense if kernel parameters could contain
>>> spaces (in that case it catches <number><blanks><garbage> input) which
>>> it can't so strtoul() would indeed make more sense. I'll prepare an
>>> updated patch and send it out later today.
>> See below for the updated patch that uses strtoul instead of sscanf.
>> This patch replaces kernel-constructor-support.patch in the -mm tree:
>
> umm, no it doesn't. I get the below incremental patch, against
> gcov-add-gcov-profiling-infrastructure.patch:

Ah, right. That answers my question whether further changes should be
posted as complete or incremental patches.

> --- a/kernel/gcov/fs.c~gcov-add-gcov-profiling-infrastructure-update
> +++ a/kernel/gcov/fs.c
> @@ -70,15 +70,8 @@ static int gcov_persist = 1;
>
> static int __init gcov_persist_setup(char *str)
> {
> - int val;
> - char delim;
> -
> - if (sscanf(str, "%d %c", &val, &delim) != 1) {
> - pr_warning("invalid gcov_persist parameter '%s'\n", str);
> - return 0;
> - }
> - pr_info("setting gcov_persist to %d\n", val);
> - gcov_persist = val;
> + gcov_persist = simple_strtoul(str, NULL, 0);
> + pr_info("setting gcov_persist to %d\n", gcov_persist);
>
> return 1;
> }
> _
>
> arguably we should use strict_strtoul(), but the kernel is a lot less
> fussy about boot parameters than it is with sysfs writes, etc. If you
> fat-finger your grub.conf, you lose and we don't tell you.

Agreed. Which leads me to another question: is it ok to post many small
patches for -mm for minor changes like this one (simple_ -> strict_) or
do you rather prefer these to be collected into somewhat larger patches
that fix multiple minor issues?