apw at shadowen
Oct 1, 2007, 5:34 AM
Post #9 of 10
(1629 views)
Permalink
|
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 [at] elte> wrote:
>
> >
> > (lkml Cc:-ed - this might be of interest to others too)
> >
> > * Andy Whitcroft <apw [at] shadowen> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
|
