Re: checkpatch and kernel/sched.c

Willy Tarreau
2007-10-02T04:34:39Z

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