LinuxカーネルHack: scripts/checkpatch.plでパッチスタイルの適合性を判定する
昨日からLKMLの購読をはじめた。がんがんメールが飛んでくる。
つい最近届いた「[PATCH 01/10] Fs: ext4: acl.c: fixed indent issue」というメールがあって、内容を見ると、単にインデントを修正しただけという内容。
Found and corrected indent issue using checkpatch.pl
checkpatch.plってなんだろう?と思って、findをかけてみる。
% find . | grep checkpatch.pl ./scripts/checkpatch.pl
checkpatch.plの変更履歴を追ってみる。checkpatch.plは要はパッチのコードチェッカーと。コーディングスタイルから外れているものや、関数等の利用が適切でないなどを指摘してくれる。2007年6月から導入された仕組みの模様。以下はcheckpatch.plが追加された時のコミット。
% git log -p --stat --color --full-diff scripts/checkpatch.pl | less -NMR [...] 15948 commit 0a920b5b666d0be8141bd1ce620fffa7de96b81b 15949 Author: Andy Whitcroft <apw@shadowen.org> 15950 Date: Fri Jun 1 00:46:48 2007 -0700 15951 15952 add a trivial patch style checker 15953 15954 We are seeing increasing levels of minor patch style violations in submissions 15955 to the mailing lists as well as making it into the tree. These detract from 15956 the quality of the submission and cause unnessary work for reviewers. 15957 15958 As a first step package up the current state of the patch style checker and 15959 include it in the kernel tree. Add instructions suggesting running it on 15960 submissions. This adds version v0.01 of the checkpatch.pl script. 15961 15962 Signed-off-by: Andy Whitcroft <apw@shadowen.org> 15963 Signed-off-by: Joel Schopp <jschopp@austin.ibm.com> 15964 Cc: Randy Dunlap <rdunlap@xenotime.net> 15965 Cc: Dave Jones <davej@codemonkey.org.uk> 15966 Signed-off-by: Andrew Morton <akpm@linux-foundation.org> 15967 Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> 15968 --- 15969 Documentation/SubmitChecklist | 6 + 15970 Documentation/SubmittingPatches | 39 ++- 15971 Documentation/feature-removal-schedule.txt | 1 + 15972 MAINTAINERS | 16 +- 15973 scripts/checkpatch.pl | 595 ++++++++++++++++++++++++++++ 15974 5 files changed, 644 insertions(+), 13 deletions(-) [...]
パッチを投稿する前に、コーディングスタイルを整えて、最低限checkpatch.plを通すようにと、パッチ投稿手順に明記されている。
% less Documentation/SubmittingPatches [...] 4) Style check your changes. Check your patch for basic style violations, details of which can be found in Documentation/CodingStyle. Failure to do so simply wastes the reviewers time and will get your patch rejected, probably without even being read. At a minimum you should check your patches with the patch style checker prior to submission (scripts/checkpatch.pl). You should be able to justify all violations that remain in your patch. [...]
最近の変更履歴を読んでみる。どうやら結構細かいチェックまでやってくれるっぽい。msleep()の引数の中までチェックしてくれると。
% git log -p --stat --color --full-diff scripts/checkpatch.pl | less -NMR
[...]
55 commit 09ef87255da0e005dd0ab39ca25714208cf29cb3
56 Author: Patrick Pannuto <ppannuto@codeaurora.org>
57 Date: Mon Aug 9 17:21:02 2010 -0700
58
59 checkpatch: warn about unexpectedly long msleep's
60
61 As explained in Documentation/timers/timers-howto.txt, msleep's of < 20ms
62 may sleep for as long as 20ms. Caller's of msleep(1) or msleep(2), etc
63 are likely not to expect this quirky behavior - warn them.
64
65 Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
66 Cc: Thomas Gleixner <tglx@linutronix.de>
67 Cc: Ingo Molnar <mingo@elte.hu>
68 Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
69 Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
70 ---
71 scripts/checkpatch.pl | 7 +++++++
72 1 files changed, 7 insertions(+), 0 deletions(-)
73
74 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
75 index 8b69af8..dd11bbe 100755
76 --- a/scripts/checkpatch.pl
77 +++ b/scripts/checkpatch.pl
78 @@ -2592,6 +2592,13 @@ sub process {
79 }
80 }
81
82 +# warn about unexpectedly long msleep's
83 + if ($line =~ /\bmsleep\s*\((\d+)\);/) {
84 + if ($1 < 20) {
85 + WARN("msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\n" . 85 $line);
86 + }
87 + }
88 +
[...]ちなみに、Documentation/feature-removal-schedule.txtには、将来廃止予定のファイルや関数がリストアップされていて、checkpatch.plはそれらを使用したら、警告が出る仕組みにもなってる。今の所、feature-removal-schedule.txtには「Check:」からはじまる行は数箇所なので、それほど恩恵は無いかもしれないけど。
% cat scripts/checkpatch.pl
[...]
my @dep_includes = ();
my @dep_functions = ();
my $removal = "Documentation/feature-removal-schedule.txt";
if ($tree && -f "$root/$removal") {
open(my $REMOVE, '<', "$root/$removal") ||
die "$P: $removal: open failed - $!\n";
while (<$REMOVE>) {
if (/^Check:\s+(.*\S)/) {
for my $entry (split(/[, ]+/, $1)) {
if ($entry =~ m@include/(.*)@) {
push(@dep_includes, $1);
} elsif ($entry !~ m@/@) {
push(@dep_functions, $entry);
}
}
}
}
close($REMOVE);
}
[...]
# don't include deprecated include files (uses RAW line)
for my $inc (@dep_includes) {
if ($rawline =~ m@^.\s*\#\s*include\s*\<$inc>@) {
ERROR("Don't use <$inc>: see Documentation/feature-removal-schedule.txt\n" . $herecurr);
}
}
# don't use deprecated functions
for my $func (@dep_functions) {
if ($line =~ /\b$func\b/) {
ERROR("Don't use $func(): see Documentation/feature-removal-schedule.txt\n" . $herecurr);
}
}どんな感じでチェックされるのか試してみる。以下のような変更をしてみた。
% git diff
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index a0d847c..e6d5e17 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -93,6 +93,7 @@
#include <net/checksum.h>
#include <net/xfrm.h>
#include <net/inet_common.h>
+#include <linux/videodev.h>
/*
* Build xmit assembly blocks
@@ -221,6 +222,13 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
local_bh_enable();
return NULL;
}
+ // C99 comment
+ printk("No facility");
+ foo("Looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong message");
+ printk(KERN_WARN "Looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong message");
+ indented_with_spaces_not_tabs();
+ msleep(10);
+ kernel_thread();
return sk;
}
おー。なるほど、こんな感じでチェックしてくれるのか。checkpatch.plのように、解決したいドメインにあわせて、自前でコードチェックツールを作るというアイデアは、実際の仕事でも使えるアイデアだな。
% git diff > /tmp/diff && perl scripts/checkpatch.pl /tmp/diff
ERROR: Don't use <linux/videodev.h>: see Documentation/feature-removal-schedule.txt
#9: FILE: net/ipv4/icmp.c:96:
+#include <linux/videodev.h>
ERROR: do not use C99 // comments
#17: FILE: net/ipv4/icmp.c:225:
+ // C99 comment
WARNING: printk() should include KERN_ facility level
#18: FILE: net/ipv4/icmp.c:226:
+ printk("No facility");
WARNING: line over 80 characters
#19: FILE: net/ipv4/icmp.c:227:
+ foo("Looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong message");
ERROR: code indent should use tabs where possible
#21: FILE: net/ipv4/icmp.c:229:
+ indented_with_spaces_not_tabs();$
WARNING: please, no space for starting a line,
excluding comments
#21: FILE: net/ipv4/icmp.c:229:
+ indented_with_spaces_not_tabs();$
WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
+ msleep(10);
ERROR: Don't use kernel_thread(): see Documentation/feature-removal-schedule.txt
#23: FILE: net/ipv4/icmp.c:231:
+ kernel_thread();
ERROR: Missing Signed-off-by: line(s)
total: 5 errors, 4 warnings, 20 lines checked
/tmp/diff has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
