linux-kernel - Re: [PATCH] add a trivial patch style checker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 May 2007 10:05:16 +0100
From:	Andy Whitcroft <apw@...dowen.org>
To:	Andi Kleen <andi@...stfloor.org>
CC:	Andrew Morton <akpm@...l.org>, Randy Dunlap <rdunlap@...otime.net>,
	Joel Schopp <jschopp@...tin.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] add a trivial patch style checker

Andi Kleen wrote:
> Andy Whitcroft <apw@...dowen.org> writes:
>> +
>> +# no BUG() or BUG_ON()
>> +		if ($line =~ /\b(BUG|BUG_ON)\b/) {
>> +			print "Try to use WARN_ON & Recovery code rather than BUG() or BUG_ON()\n";
> 
> Just outlawing BUG_ON doesn't seem like a good idea to me. We'll just end
> up with lots of untested and likely buggy recovery code or no asserts. Both
> would be bad.

Thats not an unreasonable position.  And I tend to agree with it.
Either we try and have two levels Warnings and Errors, or we just drop
this one for now.

Anyone got an oppinion, so we can get a consensus.

>> +#need space before brace following if, while, etc
>> +		if($line=~/\(.*\){/) {
>> +			print ("need a space before the brace\n");
>> +			print "$herecurr";
>> +			$clean = 0;		
>> +		}
>> +
>> +#gotos aren't indented
> 
> You mean goto labels? Surely goto statements are to be indented.
> Confusing message

Yes, changed to labels

>> +		if($line=~/^\s*[A-Za-z\d_]+:/ and !($line=~/^\s*default:/)){
>> +			print "Gotos should not be indented\n";
>> +			print "$herecurr";
>> +			$clean = 0;
>> +		}
> 
> emacs generates one space label in front of a goto label. I wouldn't
> outlaw this.

Yep, we also now allow one space something to do with diff -p not
getting confused...

>> +# don't include <linux/video_decoder.h>
> 
> It would be probably better to define some syntax that makes it possible
> to auto extract those from feature-removal-schedule.txt. Otherwise
> long term this will become messy.

Yeah, that is a very sensible idea.

> Possible further checks that might make sense:
> - panic() anywhere in drivers/* 
> - externs in .c files without asmlinkage
> - general checking that everything in a fully visible {} block is the right 
> indentation
> 
> -Andi

Thanks.

-apw
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/