From fe13a1b01c61801065571b4049e8c6bd544742b7 Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Fri, 17 Mar 2023 19:03:49 -0600 Subject: [PATCH] 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 Change-Id: I1a0d2f839949debf346aa15c65b0f407e0ce7057 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/50331 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Arshad Hussain Reviewed-by: Neil Brown Reviewed-by: Oleg Drokin --- contrib/scripts/checkpatch.pl | 47 ++++++++++++++++++++++++++++--------------- contrib/scripts/spelling.txt | 1 + 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/contrib/scripts/checkpatch.pl b/contrib/scripts/checkpatch.pl index e91aeb3..b9132d1 100755 --- 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 --- 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 -- 1.8.3.1