Re: checkpatch and kernel/sched.c

Re: checkpatch and kernel/sched.c

[Ingo Molnar]

(lkml Cc:-ed - this might be of interest to others too)

* Andy Whitcroft <apw@shadowen.org> wrote:

> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> +EXPORT_SYMBOL_GPL(cpu_clock);

yes, this is a legit warning and i fix it every time i see it. (I cannot 
fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for 
cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll 
fix it once i find the patch :)

> WARNING: printk() should include KERN_ facility level
> #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> +	printk("%-13.13s %c", p->comm,
> 
> WARNING: printk() should include KERN_ facility level
> #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> +				printk("\n");
> 
> WARNING: printk() should include KERN_ facility level
> #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> +				printk("\n");
> 
> WARNING: printk() should include KERN_ facility level
> #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> +			printk(" %s", str);
> 
> These are actually only in debug code and so are unimportant, but 
> technically they are wrong.  This check is a very difficult one in the 
> face of these constructs.  But in this case I think it has got the 
> report right.

this is actually a false positive - as the debug code constructs a 
printk output _without_ \n. So the script should check whether there's 
any \n in the printk string - if there is none, do not emit a warning. 
(if you implement that then i think it can remain a warning and does not 
need to move to CHECK.)

> At this point _if_ we took an error we would not have _DEBUG open and so
> a start would be required, otherwise not.
> 
>                         printk(" %s", str);
> 
> To be 100% correct I assume it would need to have a 
> printk(KERN_DEBUG); after each of the KERN_ERR printk's.

i've fixed this in my tree. But i dont think checkpatch.pl notices this 
level of bug - it just detects the missing KERN_ prefix in printk(), 
right?

> WARNING: braces {} are not necessary for single statement blocks
> #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> +	if (parent->groups == parent->groups->next) {
> +		pflags &= ~(SD_LOAD_BALANCE |
> +				SD_BALANCE_NEWIDLE |
> +				SD_BALANCE_FORK |
> +				SD_BALANCE_EXEC |
> +				SD_SHARE_CPUPOWER |
> +				SD_SHARE_PKG_RESOURCES);
> +	}
> 
> Ok, this one is "correct" at least for the rules as I have them.
> Perhaps the message should not be emitted for very long blocks?

If a statement is not single-line, then braces _are_ fine. Where does 
CodingStyle say that it's not fine?

> WARNING: no space between function name and open parenthesis '('
> #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> +__setup ("isolcpus=", isolated_cpu_setup);
> 
> Almost all other instances of __setup do not have a space, so I assume
> this is a reasonable report.

yes. I have just fixed it in my tree.

> WARNING: externs should be avoided in .c files
> #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> +	extern char __sched_text_start[], __sched_text_end[];
> 
> That one ... dunno.  This check is a difficult one.  Should it be a 
> CHECK?

no, this is a legitimate warning - externs in .c files should move into 
the proper .h file. So i'd suggest to keep this a WARNING.

there are a few other valid warnings that checkpatch.pl emits: such as 
the kfree one - i fixed that too in sched.c.

(Could you send me the v11-to-be version of checkpatch.pl so that i can 
check the remaining issues?)

	Ingo

Re: checkpatch and kernel/sched.c

[Andrew Morton]

On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <mingo@elte.hu> wrote:

> 
> (lkml Cc:-ed - this might be of interest to others too)
> 
> * Andy Whitcroft <apw@shadowen.org> wrote:
> 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > +EXPORT_SYMBOL_GPL(cpu_clock);
> 
> yes, this is a legit warning and i fix it every time i see it. (I cannot 
> fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for 
> cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll 
> fix it once i find the patch :)

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch

> > WARNING: printk() should include KERN_ facility level
> > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > +	printk("%-13.13s %c", p->comm,
> > 
> > WARNING: printk() should include KERN_ facility level
> > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > +				printk("\n");
> > 
> > WARNING: printk() should include KERN_ facility level
> > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > +				printk("\n");
> > 
> > WARNING: printk() should include KERN_ facility level
> > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > +			printk(" %s", str);
> > 
> > These are actually only in debug code and so are unimportant, but 
> > technically they are wrong.  This check is a very difficult one in the 
> > face of these constructs.  But in this case I think it has got the 
> > report right.
> 
> this is actually a false positive - as the debug code constructs a 
> printk output _without_ \n. So the script should check whether there's 
> any \n in the printk string - if there is none, do not emit a warning. 
> (if you implement that then i think it can remain a warning and does not 
> need to move to CHECK.)

Yeah, it does that sometimes.  I don't think it's fixable within the scope
of checkpatch.  It needs to check whether some preceding printk which might
not even be in the patch has a \n:

	printk(KERN_ERR "foo");
	<100 lines of whatever>
+	printk("bar\n");

we're screwed...

> > At this point _if_ we took an error we would not have _DEBUG open and so
> > a start would be required, otherwise not.
> > 
> >                         printk(" %s", str);
> > 
> > To be 100% correct I assume it would need to have a 
> > printk(KERN_DEBUG); after each of the KERN_ERR printk's.
> 
> i've fixed this in my tree. But i dont think checkpatch.pl notices this 
> level of bug - it just detects the missing KERN_ prefix in printk(), 
> right?
> 
> > WARNING: braces {} are not necessary for single statement blocks
> > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > +	if (parent->groups == parent->groups->next) {
> > +		pflags &= ~(SD_LOAD_BALANCE |
> > +				SD_BALANCE_NEWIDLE |
> > +				SD_BALANCE_FORK |
> > +				SD_BALANCE_EXEC |
> > +				SD_SHARE_CPUPOWER |
> > +				SD_SHARE_PKG_RESOURCES);
> > +	}
> > 
> > Ok, this one is "correct" at least for the rules as I have them.
> > Perhaps the message should not be emitted for very long blocks?
> 
> If a statement is not single-line, then braces _are_ fine. Where does 
> CodingStyle say that it's not fine?

I'd disagree with checkpatch there.  Again, it might be hard to fix.  Wanna
rename it to checkpatch-and-suggest-stuff-to-think-about.pl?

> > WARNING: no space between function name and open parenthesis '('
> > #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> > +__setup ("isolcpus=", isolated_cpu_setup);
> > 
> > Almost all other instances of __setup do not have a space, so I assume
> > this is a reasonable report.
> 
> yes. I have just fixed it in my tree.
> 
> > WARNING: externs should be avoided in .c files
> > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > +	extern char __sched_text_start[], __sched_text_end[];
> > 
> > That one ... dunno.  This check is a difficult one.  Should it be a 
> > CHECK?
> 
> no, this is a legitimate warning - externs in .c files should move into 
> the proper .h file. So i'd suggest to keep this a WARNING.

Yes.  When the symbol is defined in .S or vmlinux.lds we have
traditionally declared it in .c.

But why?  It _is_ a global symbol.  We might as well declare it in .h. 
That's consistent, and prevents duplicated declarations from popping up
later on.

Re: checkpatch and kernel/sched.c

[Avi Kivity]

Andrew Morton wrote:
>> this is actually a false positive - as the debug code constructs a 
>> printk output _without_ \n. So the script should check whether there's 
>> any \n in the printk string - if there is none, do not emit a warning. 
>> (if you implement that then i think it can remain a warning and does not 
>> need to move to CHECK.)
>>     
>
> Yeah, it does that sometimes.  I don't think it's fixable within the scope
> of checkpatch.  It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
>
> 	printk(KERN_ERR "foo");
> 	<100 lines of whatever>
> +	printk("bar\n");
>
> we're screwed...
>
>   

Isn't that broken on SMP (or with preemption) anyway?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: checkpatch and kernel/sched.c

[Andrew Morton]

On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <avi@qumranet.com> wrote:

> Andrew Morton wrote:
> >> this is actually a false positive - as the debug code constructs a 
> >> printk output _without_ \n. So the script should check whether there's 
> >> any \n in the printk string - if there is none, do not emit a warning. 
> >> (if you implement that then i think it can remain a warning and does not 
> >> need to move to CHECK.)
> >>     
> >
> > Yeah, it does that sometimes.  I don't think it's fixable within the scope
> > of checkpatch.  It needs to check whether some preceding printk which might
> > not even be in the patch has a \n:
> >
> > 	printk(KERN_ERR "foo");
> > 	<100 lines of whatever>
> > +	printk("bar\n");
> >
> > we're screwed...
> >
> >   
> 
> Isn't that broken on SMP (or with preemption) anyway?

Yep.  Or with interrupts...

Re: checkpatch and kernel/sched.c

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <avi@qumranet.com> wrote:
> 
> > Andrew Morton wrote:
> > >> this is actually a false positive - as the debug code constructs a 
> > >> printk output _without_ \n. So the script should check whether there's 
> > >> any \n in the printk string - if there is none, do not emit a warning. 
> > >> (if you implement that then i think it can remain a warning and does not 
> > >> need to move to CHECK.)
> > >>     
> > >
> > > Yeah, it does that sometimes.  I don't think it's fixable within the scope
> > > of checkpatch.  It needs to check whether some preceding printk which might
> > > not even be in the patch has a \n:
> > >
> > > 	printk(KERN_ERR "foo");
> > > 	<100 lines of whatever>
> > > +	printk("bar\n");
> > >
> > > we're screwed...
> > >
> > >   
> > 
> > Isn't that broken on SMP (or with preemption) anyway?
> 
> Yep.  Or with interrupts...

not if it's a boot-time only debug check before SMP bringup. (as it is 
in sched.c) We could make this intention explicit via a simple 
raw_printk() wrapper to printk, which could be used without KERN_.

	Ingo

Re: checkpatch and kernel/sched.c

[Sam Ravnborg]

> > 
> > > WARNING: externs should be avoided in .c files
> > > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > > +	extern char __sched_text_start[], __sched_text_end[];
> > > 
> > > That one ... dunno.  This check is a difficult one.  Should it be a 
> > > CHECK?
> > 
> > no, this is a legitimate warning - externs in .c files should move into 
> > the proper .h file. So i'd suggest to keep this a WARNING.
> 
> Yes.  When the symbol is defined in .S or vmlinux.lds we have
> traditionally declared it in .c.
> 
> But why?  It _is_ a global symbol.  We might as well declare it in .h. 
> That's consistent, and prevents duplicated declarations from popping up
> later on.
It would be nice to collect all linker symbol externs in one place..
asm-$(ARCH)/???.h

We could even auto-generate it but thats not worth it I think.

	Sam

Re: checkpatch and kernel/sched.c

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> > 
> > yes, this is a legit warning and i fix it every time i see it. (I 
> > cannot fix this one now because mainline does not have an 
> > EXPORT_SYMBOL_GPL for cpu_clock(), it's added in -mm? But i cannot 
> > find it in mm either. I'll fix it once i find the patch :)
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch

ok - i have picked up the sched.c bit for sched-devel.git. (see below)

	Ingo

---------------->
Subject: sched: export cpu_clock()
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

export cpu_clock() - the preferred API instead of sched_clock().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -469,6 +469,7 @@ unsigned long long cpu_clock(int cpu)
 
 	return now;
 }
+EXPORT_SYMBOL_GPL(cpu_clock);
 
 #ifndef prepare_arch_switch
 # define prepare_arch_switch(next)	do { } while (0)

Re: checkpatch and kernel/sched.c

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > > WARNING: braces {} are not necessary for single statement blocks
> > > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > > +	if (parent->groups == parent->groups->next) {
> > > +		pflags &= ~(SD_LOAD_BALANCE |
> > > +				SD_BALANCE_NEWIDLE |
> > > +				SD_BALANCE_FORK |
> > > +				SD_BALANCE_EXEC |
> > > +				SD_SHARE_CPUPOWER |
> > > +				SD_SHARE_PKG_RESOURCES);
> > > +	}
> > > 
> > > Ok, this one is "correct" at least for the rules as I have them.
> > > Perhaps the message should not be emitted for very long blocks?
> > 
> > If a statement is not single-line, then braces _are_ fine. Where does 
> > CodingStyle say that it's not fine?
> 
> I'd disagree with checkpatch there.  Again, it might be hard to fix.  
> Wanna rename it to checkpatch-and-suggest-stuff-to-think-about.pl?

i think checkpatch.pl is quite close to its most useful purpose and 
role: that of a reliable tool that i can use in an automated fashion 
too, with only very rare false positive - and not the current 50%-80% 
false positives rate. (And i'm fighting hard for Andy to realize the 
true scope and purpose of this script ;-)

	Ingo

Re: checkpatch and kernel/sched.c

[Andy Whitcroft]

On Mon, Oct 01, 2007 at 12:30:07AM -0700, Andrew Morton wrote:
> On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > (lkml Cc:-ed - this might be of interest to others too)
> > 
> > * Andy Whitcroft <apw@shadowen.org> wrote:
> > 
> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> > 
> > yes, this is a legit warning and i fix it every time i see it. (I cannot 
> > fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for 
> > cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll 
> > fix it once i find the patch :)
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
> 
> > > WARNING: printk() should include KERN_ facility level
> > > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > > +	printk("%-13.13s %c", p->comm,
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > > +				printk("\n");
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > > +				printk("\n");
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > > +			printk(" %s", str);
> > > 
> > > These are actually only in debug code and so are unimportant, but 
> > > technically they are wrong.  This check is a very difficult one in the 
> > > face of these constructs.  But in this case I think it has got the 
> > > report right.
> > 
> > this is actually a false positive - as the debug code constructs a 
> > printk output _without_ \n. So the script should check whether there's 
> > any \n in the printk string - if there is none, do not emit a warning. 
> > (if you implement that then i think it can remain a warning and does not 
> > need to move to CHECK.)
> 
> Yeah, it does that sometimes.  I don't think it's fixable within the scope
> of checkpatch.  It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
> 
> 	printk(KERN_ERR "foo");
> 	<100 lines of whatever>
> +	printk("bar\n");
> 
> we're screwed...

Actually checkpatch does take that into account.  If the printk has no
precceding printk ending in a newline, within the scope of the diff
then the warning is suppressed.

However, in this mm/sched.c case there are paths through the code which
do violate the "every line starts with KERN_" mantra.  Now checkpatch is
actually not clever enough to spot that these are conditional, but in
this case there is an error.  Its not important as it is debug.  But I
contend there is an erorr.  When an error is thrown we close the
unfinished line, emit a line at KERN_ERR and then continue to emit the
previous line without switching to KERN_DEBUG again.

> > > At this point _if_ we took an error we would not have _DEBUG open and so
> > > a start would be required, otherwise not.
> > > 
> > >                         printk(" %s", str);
> > > 
> > > To be 100% correct I assume it would need to have a 
> > > printk(KERN_DEBUG); after each of the KERN_ERR printk's.
> > 
> > i've fixed this in my tree. But i dont think checkpatch.pl notices this 
> > level of bug - it just detects the missing KERN_ prefix in printk(), 
> > right?
> > 
> > > WARNING: braces {} are not necessary for single statement blocks
> > > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > > +	if (parent->groups == parent->groups->next) {
> > > +		pflags &= ~(SD_LOAD_BALANCE |
> > > +				SD_BALANCE_NEWIDLE |
> > > +				SD_BALANCE_FORK |
> > > +				SD_BALANCE_EXEC |
> > > +				SD_SHARE_CPUPOWER |
> > > +				SD_SHARE_PKG_RESOURCES);
> > > +	}
> > > 
> > > Ok, this one is "correct" at least for the rules as I have them.
> > > Perhaps the message should not be emitted for very long blocks?
> > 
> > If a statement is not single-line, then braces _are_ fine. Where does 
> > CodingStyle say that it's not fine?
> 
> I'd disagree with checkpatch there.  Again, it might be hard to fix.  Wanna
> rename it to checkpatch-and-suggest-stuff-to-think-about.pl?

Checkpatch is capable of making the determination as at the time we
emit the warning we have all of the block in question.  We already allow
a number of other exceptions.  Just I think what I think the "rule" is
and its actuallity are not the same.

Are we saying the rule is "braces are not required for single _line_
blocks?

> > > WARNING: no space between function name and open parenthesis '('
> > > #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> > > +__setup ("isolcpus=", isolated_cpu_setup);
> > > 
> > > Almost all other instances of __setup do not have a space, so I assume
> > > this is a reasonable report.
> > 
> > yes. I have just fixed it in my tree.
> > 
> > > WARNING: externs should be avoided in .c files
> > > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > > +	extern char __sched_text_start[], __sched_text_end[];
> > > 
> > > That one ... dunno.  This check is a difficult one.  Should it be a 
> > > CHECK?
> > 
> > no, this is a legitimate warning - externs in .c files should move into 
> > the proper .h file. So i'd suggest to keep this a WARNING.
> 
> Yes.  When the symbol is defined in .S or vmlinux.lds we have
> traditionally declared it in .c.
> 
> But why?  It _is_ a global symbol.  We might as well declare it in .h. 
> That's consistent, and prevents duplicated declarations from popping up
> later on.

-apw

Re: checkpatch and kernel/sched.c

[Willy Tarreau]

On Mon, Oct 01, 2007 at 12:30:07AM -0700, Andrew Morton wrote:
> On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > (lkml Cc:-ed - this might be of interest to others too)
> > 
> > * Andy Whitcroft <apw@shadowen.org> wrote:
> > 
> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> > 
> > yes, this is a legit warning and i fix it every time i see it. (I cannot 
> > fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for 
> > cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll 
> > fix it once i find the patch :)
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
> 
> > > WARNING: printk() should include KERN_ facility level
> > > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > > +	printk("%-13.13s %c", p->comm,
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > > +				printk("\n");
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > > +				printk("\n");
> > > 
> > > WARNING: printk() should include KERN_ facility level
> > > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > > +			printk(" %s", str);
> > > 
> > > These are actually only in debug code and so are unimportant, but 
> > > technically they are wrong.  This check is a very difficult one in the 
> > > face of these constructs.  But in this case I think it has got the 
> > > report right.
> > 
> > this is actually a false positive - as the debug code constructs a 
> > printk output _without_ \n. So the script should check whether there's 
> > any \n in the printk string - if there is none, do not emit a warning. 
> > (if you implement that then i think it can remain a warning and does not 
> > need to move to CHECK.)
> 
> Yeah, it does that sometimes.  I don't think it's fixable within the scope
> of checkpatch.  It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
> 
> 	printk(KERN_ERR "foo");
> 	<100 lines of whatever>
> +	printk("bar\n");
> 
> we're screwed...

Well, I think that we could do something like this :

#define KERN_CONT ""
...
	printk(KERN_ERR "foo");
	<100 lines of whatever>
	printk(KERN_CONT "bar\n");

It would indicate the author's *intent* which is to continue a line which
has already been started. It would both permit us to remove false positives
from automated scripts, and to read the code more easily. And this is not a
big constaint for the author, given that such constructs are quite rare.

Regards,
Willy

[patch] printk: add KERN_CONT annotation

[Ingo Molnar]

* Willy Tarreau <w@1wt.eu> wrote:

> Well, I think that we could do something like this :
> 
> #define KERN_CONT ""
> ...
> 	printk(KERN_ERR "foo");
> 	<100 lines of whatever>
> 	printk(KERN_CONT "bar\n");
> 
> It would indicate the author's *intent* which is to continue a line 
> which has already been started. It would both permit us to remove 
> false positives from automated scripts, and to read the code more 
> easily. And this is not a big constaint for the author, given that 
> such constructs are quite rare.

ah, this is even nicer than the raw_printk() thing i suggested, and it 
also nicely documents the intention of the author. Patch attached below.

And i'd like to stress the principle that is followed here: in this 
particular case the checkpatch.pl warning is very useful, but still 
there are false positives. Fortunately they are so rare that it's worth 
annotating those few exceptions in the source. Note that the goal is 
still to be able to achieve 100% warning-free source code. _That_ should 
be the driving principle behind checkpatch.pl warnings.

	Ingo

------------------>
Subject: printk: add KERN_CONT annotation
From: Ingo Molnar <mingo@elte.hu>

printk: add the KERN_CONT annotation (which is empty string but via 
which checkpatch.pl can notice that the lacking KERN_ level is fine). 
This useful for multiple calls of hand-crafted printk output done by 
early debug code or similar.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/kernel.h |    7 +++++++
 kernel/sched.c         |   22 +++++++++++-----------
 2 files changed, 18 insertions(+), 11 deletions(-)

Index: linux/include/linux/kernel.h
===================================================================
--- linux.orig/include/linux/kernel.h
+++ linux/include/linux/kernel.h
@@ -61,6 +61,13 @@ extern const char linux_proc_banner[];
 #define	KERN_INFO	"<6>"	/* informational			*/
 #define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
 
+/*
+ * Annotation for a "continued" line of log printout (only done after a
+ * line that had no enclosing \n). Only to be used by core/arch code
+ * during early bootup (a continued line is not SMP-safe otherwise).
+ */
+#define	KERN_CONT	""
+
 extern int console_printk[];
 
 #define console_loglevel (console_printk[0])
Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -4770,18 +4770,18 @@ static void show_task(struct task_struct
 	unsigned state;
 
 	state = p->state ? __ffs(p->state) + 1 : 0;
-	printk("%-13.13s %c", p->comm,
+	printk(KERN_INFO "%-13.13s %c", p->comm,
 		state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?');
 #if BITS_PER_LONG == 32
 	if (state == TASK_RUNNING)
-		printk(" running  ");
+		printk(KERN_CONT " running  ");
 	else
-		printk(" %08lx ", thread_saved_pc(p));
+		printk(KERN_CONT " %08lx ", thread_saved_pc(p));
 #else
 	if (state == TASK_RUNNING)
-		printk("  running task    ");
+		printk(KERN_CONT "  running task    ");
 	else
-		printk(" %016lx ", thread_saved_pc(p));
+		printk(KERN_CONT " %016lx ", thread_saved_pc(p));
 #endif
 #ifdef CONFIG_DEBUG_STACK_USAGE
 	{
@@ -4791,7 +4791,7 @@ static void show_task(struct task_struct
 		free = (unsigned long)n - (unsigned long)end_of_stack(p);
 	}
 #endif
-	printk("%5lu %5d %6d\n", free, p->pid, p->parent->pid);
+	printk(KERN_CONT "%5lu %5d %6d\n", free, p->pid, p->parent->pid);
 
 	if (state != TASK_RUNNING)
 		show_stack(p, NULL);
@@ -5527,20 +5527,20 @@ static void sched_domain_debug(struct sc
 			}
 
 			if (!group->__cpu_power) {
-				printk("\n");
+				printk(KERN_CONT "\n");
 				printk(KERN_ERR "ERROR: domain->cpu_power not "
 						"set\n");
 				break;
 			}
 
 			if (!cpus_weight(group->cpumask)) {
-				printk("\n");
+				printk(KERN_CONT "\n");
 				printk(KERN_ERR "ERROR: empty group\n");
 				break;
 			}
 
 			if (cpus_intersects(groupmask, group->cpumask)) {
-				printk("\n");
+				printk(KERN_CONT "\n");
 				printk(KERN_ERR "ERROR: repeated CPUs\n");
 				break;
 			}
@@ -5548,11 +5548,11 @@ static void sched_domain_debug(struct sc
 			cpus_or(groupmask, groupmask, group->cpumask);
 
 			cpumask_scnprintf(str, NR_CPUS, group->cpumask);
-			printk(" %s", str);
+			printk(KERN_CONT " %s", str);
 
 			group = group->next;
 		} while (group != sd->groups);
-		printk("\n");
+		printk(KERN_CONT "\n");
 
 		if (!cpus_equal(sd->span, groupmask))
 			printk(KERN_ERR "ERROR: groups don't span "

Re: [patch] printk: add KERN_CONT annotation

[Jörn Engel]

On Tue, 2 October 2007 07:18:52 +0200, Ingo Molnar wrote:
> 
> ah, this is even nicer than the raw_printk() thing i suggested, and it 
> also nicely documents the intention of the author. Patch attached below.

KERN_CONT was brought up in the linux-tiny discussion.  Not sure if you
want to get involved in that, but there may be value in adding one
variant of KERN_CONT per debug level:
http://lkml.org/lkml/2007/9/30/151

> And i'd like to stress the principle that is followed here: in this 
> particular case the checkpatch.pl warning is very useful, but still 
> there are false positives. Fortunately they are so rare that it's worth 
> annotating those few exceptions in the source. Note that the goal is 
> still to be able to achieve 100% warning-free source code. _That_ should 
> be the driving principle behind checkpatch.pl warnings.

Thank you for working on this.  I had nearly given up on checkpatch
before.

Jörn

-- 
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein

Re: [patch] printk: add KERN_CONT annotation

[Joe Perches]

On Tue, 2007-10-02 at 07:18 +0200, Ingo Molnar wrote:
> +#define	KERN_CONT	""

This doesn't work with printk(char** array[index]) continuations
or with strings with embedded KERN_ prefixes.

printk(array[index])
printk(version)

Re: [patch] printk: add KERN_CONT annotation

[Jan Engelhardt]

On Oct 2 2007 08:41, Joe Perches wrote:
>On Tue, 2007-10-02 at 07:18 +0200, Ingo Molnar wrote:
>> +#define	KERN_CONT	""
>
>This doesn't work with printk(char** array[index]) continuations
>or with strings with embedded KERN_ prefixes.

Huh?
...Ah. Yeah, pasting a string literal with a variable won't work.
But then again, you should not use printk(var) directly, but always
use printk("%s", var)

Re: [patch] printk: add KERN_CONT annotation

[Joe Perches]

On Tue, 2007-10-02 at 17:45 +0200, Jan Engelhardt wrote:
> always use printk("%s", var)

You have to use indirect arguments to log something?
Don't you think that's a stupid rule?

Look at the version strings that are used as __devinitdata.

Re: [patch] printk: add KERN_CONT annotation

[Jan Engelhardt]

On Oct 2 2007 09:03, Joe Perches wrote:
>On Tue, 2007-10-02 at 17:45 +0200, Jan Engelhardt wrote:
>> always use printk("%s", var)
>
>You have to use indirect arguments to log something?

No, you do not have to.

>Don't you think that's a stupid rule?

Not at all. var may contain format specifiers, which poses a
certain security issue into people's hands. This is already
important in userspace, so is probably even more in the kernel,
even though user-supplied strings are less common.

Re: [patch] printk: add KERN_CONT annotation

[Andrew Morton]

On Tue, 2 Oct 2007 07:18:52 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> printk: add the KERN_CONT annotation (which is empty string but via 
> which checkpatch.pl can notice that the lacking KERN_ level is fine). 
> This useful for multiple calls of hand-crafted printk output done by 
> early debug code or similar.
> 

I like it.  Sort of self-documenting notation.

> 
> Index: linux/include/linux/kernel.h
> ===================================================================
> --- linux.orig/include/linux/kernel.h
> +++ linux/include/linux/kernel.h
> @@ -61,6 +61,13 @@ extern const char linux_proc_banner[];
>  #define	KERN_INFO	"<6>"	/* informational			*/
>  #define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
>  
> +/*
> + * Annotation for a "continued" line of log printout (only done after a
> + * line that had no enclosing \n). Only to be used by core/arch code
> + * during early bootup (a continued line is not SMP-safe otherwise).
> + */
> +#define	KERN_CONT	""
> +
>  extern int console_printk[];
>  
>  #define console_loglevel (console_printk[0])
> Index: linux/kernel/sched.c

I get rejects from the sched.c hunk and that's your stuff anwyay, so I
dropped that bit.

Re: [patch] printk: add KERN_CONT annotation

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 2 Oct 2007 07:18:52 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > printk: add the KERN_CONT annotation (which is empty string but via 
> > which checkpatch.pl can notice that the lacking KERN_ level is fine). 
> > This useful for multiple calls of hand-crafted printk output done by 
> > early debug code or similar.
> > 
> 
> I like it.  Sort of self-documenting notation.

cool. Please push into v2.6.24.

> >  #define console_loglevel (console_printk[0])
> > Index: linux/kernel/sched.c
> 
> I get rejects from the sched.c hunk and that's your stuff anwyay, so I 
> dropped that bit.

yeah, i'll fix sched.c up once it goes upstream.

	Ingo