checkpatch: warn about using CONFIG_EXPERIMENTAL

PATCH In review Kees Cook 2 participants 4 messages Nov 12, 2012 Checkpatch

Share & tools

Kees Cook

author

2012-11-12 22:50 UTC #0 lore raw

PATCH 1/1 checkpatch: warn about using CONFIG_EXPERIMENTAL

This config item has not carried much meaning for a while now and is almost always enabled by default. As agreed during the Linux kernel summit, it is being removed. This will discourage future addition of CONFIG_EXPERIMENTAL while it is being phased out.

Cc Andy Whitcroft <[email protected]>

Cc Joe Perches <[email protected]>

Signed-off-by Kees Cook <[email protected]>

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

scripts/checkpatch.pl +17 -0

index f18750e..41e364b 100755
@@ -1757,6 +1757,15 @@ sub process {
 			#print "is_start<$is_start> is_end<$is_end> length<$length>\n";
 		}
 
+# discourage the addition of CONFIG_EXPERIMENTAL in Kconfig.
+		if ($realfile =~ /Kconfig/ &&
+		    $line =~ /.\s*depends on\s+.*\bEXPERIMENTAL\b/) {
+			WARN("CONFIG_EXPERIMENTAL",
+			     "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+			     "For alternatives, see " +
+			     "https://lkml.org/lkml/2012/10/23/580\n");
+		}
+
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
 		    ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
 			my $flag = $1;

@@ -1912,6 +1921,14 @@ sub process {
 # check we are in a valid C source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c)$/);
 
+# discourage the addition of CONFIG_EXPERIMENTAL in #if(def).
+		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_EXPERIMENTAL\b/) {
+			WARN("CONFIG_EXPERIMENTAL",
+			     "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+			     "For alternatives, see " +
+			     "https://lkml.org/lkml/2012/10/23/580\n");
+		}
+
 # check for RCS/CVS revision markers
 		if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) {
 			WARN("CVS_KEYWORD",

Signature · 6 lines

1.7.9.5


-- 
Kees Cook
Chrome OS Security

Joe Perches

maintainer

2012-11-12 23:06 UTC #1 lore raw

Quoted 4 lines · Kees Cook

This config item has not carried much meaning for a while now and is almost always enabled by default. As agreed during the Linux kernel summit, it is being removed. This will discourage future addition of CONFIG_EXPERIMENTAL while it is being phased out.

You should also cc Andrew Morton (cc'd)

Quoted 12 lines

Cc Andy Whitcroft <[email protected]>
Cc Joe Perches <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 scripts/checkpatch.pl |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
scripts/checkpatch.pl +0 -0
index f18750e..41e364b 100755

scripts/checkpatch.pl sub process { · line 1762

Quoted 6 lines

scripts/checkpatch.pl +3 -0

@@ -1757,6 +1757,15 @@ sub process {
 			#print "is_start<$is_start> is_end<$is_end> length<$length>\n";
 		}
 
+# discourage the addition of CONFIG_EXPERIMENTAL in Kconfig.
+		if ($realfile =~ /Kconfig/ &&
+		    $line =~ /.\s*depends on\s+.*\bEXPERIMENTAL\b/) {

This is after the deleted lines check so this won't trigger on deleted lines.

scripts/checkpatch.pl sub process { · line 1766

Quoted 4 lines

scripts/checkpatch.pl +4 -0

+			WARN("CONFIG_EXPERIMENTAL",
+			     "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+			     "For alternatives, see " +
+			     "https://lkml.org/lkml/2012/10/23/580\n");

I'd much prefer this be on a single line and not using the + string operator as that style is not currently used in checkpatch.

scripts/checkpatch.pl sub process { · line 1925

Quoted 11 lines

scripts/checkpatch.pl +4 -0

+		}
+
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
 		    ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
 			my $flag = $1;

@@ -1912,6 +1921,14 @@ sub process {
 # check we are in a valid C source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c)$/);
 
+# discourage the addition of CONFIG_EXPERIMENTAL in #if(def).
+		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_EXPERIMENTAL\b/) {

Same as previous reply just a few minutes ago

There are many config uses like:

        #if defined(CONFIG_FOO)
and
        #if defined CONFIG_FOO || defined CONFIG_BAR

so I think any line that has this specific config option is suspect.

Quoted 5 lines

scripts/checkpatch.pl +5 -0

+			WARN("CONFIG_EXPERIMENTAL",
+			     "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+			     "For alternatives, see " +
+			     "https://lkml.org/lkml/2012/10/23/580\n");
+		}

Kees Cook

author

2012-11-12 23:11 UTC #2 lore raw

Quoted 23 lines · Joe Perches

Quoted 4 lines · Kees Cook
This config item has not carried much meaning for a while now and is almost always enabled by default. As agreed during the Linux kernel summit, it is being removed. This will discourage future addition of CONFIG_EXPERIMENTAL while it is being phased out.
You should also cc Andrew Morton (cc'd)
Quoted 15 lines
Cc Andy Whitcroft <[email protected]>
Cc Joe Perches <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 scripts/checkpatch.pl |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
scripts/checkpatch.pl +0 -0
index f18750e..41e364b 100755
@@ -1757,6 +1757,15 @@ sub process {
                      #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
              }
 

Quoted 6 lines

scripts/checkpatch.pl sub process { · line 1762
Quoted 3 lines
scripts/checkpatch.pl +3 -0
+# discourage the addition of CONFIG_EXPERIMENTAL in Kconfig.
+             if ($realfile =~ /Kconfig/ &&
+                 $line =~ /.\s*depends on\s+.*\bEXPERIMENTAL\b/) {
This is after the deleted lines check so this won't trigger on deleted lines.

Correct -- I don't want to trigger on deleted lines. That's what we want to see in patches. :)

Quoted 7 lines

scripts/checkpatch.pl sub process { · line 1766
Quoted 4 lines
scripts/checkpatch.pl +4 -0
+                     WARN("CONFIG_EXPERIMENTAL",
+                          "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+                          "For alternatives, see " +
+                          "https://lkml.org/lkml/2012/10/23/580\n");
I'd much prefer this be on a single line and not using the + string operator as that style is not currently used in checkpatch.

Hah. Okay. We should exclude checkpatch.pl from the 80 character check it warns about. ;)

Quoted 14 lines

scripts/checkpatch.pl sub process { · line 1925

Quoted 11 lines

scripts/checkpatch.pl +4 -0

+             }
+
              if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
                  ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
                      my $flag = $1;

@@ -1912,6 +1921,14 @@ sub process {
 # check we are in a valid C source file if not then ignore this hunk
              next if ($realfile !~ /\.(h|c)$/);
 
+# discourage the addition of CONFIG_EXPERIMENTAL in #if(def).
+             if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_EXPERIMENTAL\b/) {

Same as previous reply just a few minutes ago

Quoted 4 lines

There are many config uses like:

        #if defined(CONFIG_FOO)
and
        #if defined CONFIG_FOO || defined CONFIG_BAR

Right, this will catch those. "if.*" above

Quoted 9 lines

so I think any line that has this specific config option is suspect.

Quoted 5 lines

scripts/checkpatch.pl +5 -0

+                     WARN("CONFIG_EXPERIMENTAL",
+                          "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+                          "For alternatives, see " +
+                          "https://lkml.org/lkml/2012/10/23/580\n");
+             }

Thanks!

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Joe Perches

maintainer

2012-11-12 23:19 UTC #3 lore raw

Quoted 7 lines · Kees Cook

Quoted 6 lines · Joe Perches

Quoted 4 lines · Kees Cook

scripts/checkpatch.pl +4 -0

+                     WARN("CONFIG_EXPERIMENTAL",
+                          "Use of CONFIG_EXPERIMENTAL is deprecated. " +
+                          "For alternatives, see " +
+                          "https://lkml.org/lkml/2012/10/23/580\n");

Quoted 5 lines

Quoted 2 lines
I'd much prefer this be on a single line and not using the + string operator as that style is not currently used in checkpatch.
Hah. Okay. We should exclude checkpatch.pl from the 80 character check it warns about. ;)

;)

checkpatch is for .[ch] files, it isn't for anything else really.