LU-16634 build: improve checkpatch warnings

Change checkpatch.pl to allow RETURN/GOTO as "end of switch case".

Improve CERROR/CWARN/LCONSOLE/CDEBUG message checking/warning to
print more useful message style advice than just "think hard".

Allow "DFID|DOSTID" within long error strings without complaint.

Add a spelling.txt rule to warn if version checks are added in a
test for future versions.  This is mostly useful for maintenance
branches when patches are being backported with test cases.

Test-Parameters: trivial
Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Change-Id: I1a0d2f839949debf346aa15c65b0f407e0ce7057
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/50331
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Arshad Hussain <arshad.hussain@aeoncomputing.com>
Reviewed-by: Neil Brown <neilb@suse.de>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/contrib/scripts/checkpatch.pl b/contrib/scripts/checkpatch.pl
index e91aeb3..b9132d1 100755 (executable)
--- a/contrib/scripts/checkpatch.pl
+++ b/contrib/scripts/checkpatch.pl
@@ -2894,6 +2894,10 @@ sub process {
                        if ($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(?:KERN_\S+\s*|[^"]*))?($String\s*(?:|,|\)\s*;)\s*)$/ &&
                            length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $max_line_length) {
                                $msg_type = "";
+                       # a Lustre message that contains embedded formatting
+                       } elsif ($line =~ /^\+\s*(?:$logFunctions\s*\()?($String(?:DFID|DOSTID)$String\s*(?:|,|\)\s*;)?\s*)$/ &&
+                                length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $max_line_length) {
+                               $msg_type = ""
 
                        # lines with only strings (w/ possible termination)
                        # #defines with only strings
@@ -5168,31 +5172,42 @@ sub process {
                        }
                }
 
-# try to replace assertions with error handling
+# Lustre try to replace assertions with error handling
                if ($line =~ /\bLASSERTF?\s*\(/) {
                    WARN("LASSERT",
-                        "try to replace assertions with error handling\n" .
+                        "Try to replace assertions with error handling\n" .
                         $herecurr);
                }
 
-# avoid new console messages
-               if ($line =~ /\bLCONSOLE[A-Z_]*\s*\(/) {
-                   WARN("LCONSOLE",
-                        "avoid adding new console messages\n" .
-                        $herecurr);
-               }
+# Lustre minimize new CERROR messages
+               if ($line =~ /\b(CEMERG|CERROR|CNETERR|CWARN|LCONSOLE)\s*\(/) {
+                       if ($rawline !~ /\(\"\%s: /) {
+                               WARN("CERROR_DEV",
+                                    "Console messages should start with '%s:' to print device name\n" . $herecurr)
+                       }
 
-# minimize new CERROR messages
-               if ($line =~ /\bC(EMERG|ERROR|NETERR|WARN)\s*\(/) {
-                   WARN("CERROR",
-                        "think hard when adding new CERROR messages\n" .
-                        $herecurr);
+                       # Check for "rc = %d" or "rc = %ld" at the end of errors
+                       if ($line !~ /LCONSOLE/ && $rawline !~ /: rc = \%l?d\\n\",/) {
+                               WARN("CERROR_RET",
+                                    "Console messages should end with ': rc = %d' or 'rc = %ld'\n" . $herecurr);
+                       }
+
+                       # This is fine as we are only matching the first part.
+                       if ($line =~ /(CEMERG|CERROR|CNETERR)/) {
+                               WARN("CERROR",
+                                    "Errors should be useful to fix failure conditions, not status/debug\n" . $herecurr);
+                       }
+               }
+# Lustre avoid unlimited message printing to the console
+               if ($line =~ /CDEBUG\((D_ERROR|D_WARNING|D_CONSOLE|[a-z])/) {
+                       WARN("CDEBUG_LIMIT",
+                            "CDEBUG does not rate-limit console messages, use CDEBUG_LIMIT\n". $herecurr);
                }
 
-# don't allow GPLv3 license files
+# Lustre don't allow GPLv3 license files
                if ($rawline =~ /version 3/) {
                        WARN("GPLV3",
-                            "using GPLv3 is usually wrong\n" . $herecurr);
+                            "Using GPLv3 is usually wrong\n" . $herecurr);
                }
 
 # check for single line unbalanced braces
@@ -6116,7 +6131,7 @@ sub process {
                                next if ($fline =~ /^.[\s$;]*$/);
                                $has_statement = 1;
                                $count++;
-                               $has_break = 1 if ($fline =~ /\bswitch\b|\b(?:break\s*;[\s$;]*$|return\b|goto\b|continue\b)/);
+                               $has_break = 1 if ($fline =~ /\bswitch\b|\b(?:break\s*;[\s$;]*$|return\b|goto\b|continue\b)/i);
                        }
                        if (!$has_break && $has_statement) {
                                WARN("MISSING_BREAK",

diff --git a/contrib/scripts/spelling.txt b/contrib/scripts/spelling.txt
index 8f6dd2c..5dbf09c 100644 (file)
--- a/contrib/scripts/spelling.txt
+++ b/contrib/scripts/spelling.txt
@@ -210,4 +210,5 @@ struct timeval||struct timespec64
 tempnam||mkstemp
 time_t||timeout_t
 timer_setup||cfs_timer_setup
+version_code.*2.1[7-9]||version 2.16.x should be used
 wait_queue_t||wait_queue_entry_t