linux-kernel - Re: [RFC patch] checkpatch: Add a test for long function definitions (>200 lines)

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:   Sun, 17 Dec 2017 14:22:24 -0800
From:   Joe Perches <joe@...ches.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dan Carpenter <error27@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [RFC patch] checkpatch: Add a test for long function
 definitions (>200 lines)

On Sun, 2017-12-17 at 13:46 -0800, Linus Torvalds wrote:
> On Sat, Dec 16, 2017 at 5:26 PM, Joe Perches <joe@...ches.com> wrote:
> > > 
> > >    I'm not expecting you to be able to write a perl script that checks
> > >    the first line, but we have way too many 200-plus line functions in
> > >    the kernel.  I'd like a warning on anything over 200 lines (a factor
> > >    of 4 over Linus's stated goal).
> > 
> > In response to Matthew's request:
> > 
> > This is a possible checkpatch warning for long
> > function definitions.
> 
> So I'm not sure a line count makes sense.
> 
> Sometimes long functions can be sensible, if they are basically just
> one big case-statement or similar.
> 
> Looking at one of your examples: futex_requeue() is indeed a long
> function, but that's mainly because it has a lot of comments about
> exactly what is going on, and while it only has one (fairly small)
> case statement, the rest of it is very similar (ie "in this case, do
> XYZ").
> 
> Another case I looked at - try_to_unmap_one() - had very similar
> behavior.  It's long, but it's not long for the wrong reasons.
> 
> And yes, "copy_process()" is disgusting, and probably _could_ be split
> up a bit, but at the same time the bulk of the lines there really is
> just the "initialize all the parts of the "struct task_struct".
> 
> And other times, I suspect even a 50-line function is way too dense,
> just because it's doing crazy things.
> 
> So I have a really hard time with some arbitrary line limit. At eh
> very least, I think it should ignore comments and whitespace lines.

That part is easy enough to do. (below)

> And yes, some real "complexity analysis" might give a much more sane
> limit, but I don't even know what that would be or how it would work.

I suspect there are better tools (like gnu complexity)
for this and I'm not at all tied to this as a checkpatch
feature.

btw: futex_requeue line count is now 140 so it doesn't warn.

---
 scripts/checkpatch.pl | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 168687ae24fa..99c065f90360 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -59,6 +59,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1;
+my $max_function_length = 200;
 
 sub help {
 	my ($exitcode) = @_;
@@ -2202,6 +2203,8 @@ sub process {
 	my $realcnt = 0;
 	my $here = '';
 	my $context_function;		#undef'd unless there's a known function
+	my $context_function_start;
+	my $context_function_lines;
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
@@ -2341,6 +2344,8 @@ sub process {
 			} else {
 				undef $context_function;
 			}
+			undef $context_function_start;
+			$context_function_lines = 0;
 			next;
 
 # track the line number as we move through the hunk, note that
@@ -3200,11 +3205,25 @@ sub process {
 		if ($sline =~ /^\+\{\s*$/ &&
 		    $prevline =~ /^\+(?:(?:(?:$Storage|$Inline)\s*)*\s*$Type\s*)?($Ident)\(/) {
 			$context_function = $1;
+			$context_function_start = $realline;
+			$context_function_lines = 0;
+		}
+
+# if in a function, count the non-blank lines
+		if (defined ($context_function) && $sline !~ /^[ \+]\s*$/) {
+			$context_function_lines++;
 		}
 
 # check if this appears to be the end of function declaration
 		if ($sline =~ /^\+\}\s*$/) {
+			if (defined($context_function_start) &&
+			    $context_function_lines > $max_function_length) {
+				WARN("LONG_FUNCTION",
+				     "'$context_function' function definition is " . $context_function_lines . " statement lines, perhaps refactor\n" . $herecurr);
+			}
 			undef $context_function;
+			undef $context_function_start;
+			$context_function_lines = 0;
 		}
 
 # check indentation of any line with a bare else
@@ -5983,6 +6002,8 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*{/s) {
 			$context_function = $1;
+			$context_function_start = $realline;
+			$context_function_lines = 0;
 
 # check for multiline function definition with misplaced open brace
 			my $ok = 0;