Re: checkpatch and kernel/sched.c

[Willy Tarreau <w@1wt.eu>]

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