On Mon, Oct 01, 2007 at 12:30:07AM -0700, Andrew Morton wrote: > On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <[EMAIL PROTECTED]> wrote: > > > > > (lkml Cc:-ed - this might be of interest to others too) > > > > * Andy Whitcroft <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/