[PATCH 1/5] checkpatch: fix false errors due to macro concatenation

[PATCH 1/5] checkpatch: fix false errors due to macro concatenation

[Daniel Walker]

The macro concatenation (##) sequence can cause false errors when checking
macro's. Checkpatch doesn't currently know about the operator.

For example this line,

+ 	entry = (struct ftrace_raw_##call *)raw_data;                   \

is correct but it produces the following error,

ERROR: need consistent spacing around '*' (ctx:WxB)
+       entry = (struct ftrace_raw_##call *)raw_data;\
                                          ^

The line above doesn't have any spacing problems, and if you remove the
macro concatenation sequence checkpatch doesn't give any errors. This change
resolves this by just always removing "##" in every line checked.

Signed-off-by: Daniel Walker <dwalker@fifo99.com>
---
 scripts/checkpatch.pl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2d5ece7..09bab22 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -397,6 +397,11 @@ sub sanitise_line {
 		$res =~ s@(\#\s*(?:error|warning)\s+).*@$1$clean@;
 	}
 
+	# The macro concatenation sequence is unique so we can just delete it.
+	# If it's not deleted it screws up the rest of the matching and can
+	# result in false errors.
+	$res =~ s/($Ident|\s)\s*\#\#\s*($Ident|\s)/$1$2/g;
+
 	return $res;
 }
 
-- 
1.5.6.3

[PATCH 2/5] checkpatch: fix hang in relative indent checking

[Daniel Walker]

I ran this command on v2.6.31 ,

./scripts/checkpatch.pl --file net/decnet/dn_fib.c

which resulted in checkpatch hanging without any output.

The lines that cause the hang are,

#define for_nexthops(fi) { int nhsel; const struct dn_fib_nh *nh;\
        for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)

The hang happend in the relative indent checking code. Checkpatch has the
following comment around the relative indent checking block,

# Also ignore a loop construct at the end of a
# preprocessor statement.

Since the line it's hanging on exactly fits the comment it shouldn't even be
checking this line. To resolve this I just prevent the checking like the
comment says should happen.

Signed-off-by: Daniel Walker <dwalker@fifo99.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 09bab22..1c48a6c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1519,7 +1519,7 @@ sub process {
 
 			my $cond_ptr = -1;
 			$continuation = 0;
-			while ($cond_ptr != $cond_lines) {
+			while ($check && $cond_ptr != $cond_lines) {
 				$cond_ptr = $cond_lines;
 
 				# If we see an #else/#elif then the code
-- 
1.5.6.3

[PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

There are times when maintainers intentially don't follow the coding
style. When that happens it means some errors need to be ignored, so
that other errors can be focused on.

To handle that I added a blacklist to checkpatch. The blacklist holds the
file names and errors which are ignored. The output is modified to
remove the errors from the list and not to count them.

When the blacklist kicks in there is a note that does list how many
errors got removed and that it was due to a blacklist entry. There is
also a new option "--noblacklist" that allows the errors to be added
back as it was without the blacklist.

There is also a small fix I added to correct a problem when "--file" is
used. The patch output had one level of the directory structure
removed, which prevented the blacklist from catching those filenames.

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Daniel Walker <dwalker@fifo99.com>
---
 scripts/checkpatch.pl |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1c48a6c..c7f741f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -26,6 +26,7 @@ my $check = 0;
 my $summary = 1;
 my $mailback = 0;
 my $summary_file = 0;
+my $noblacklist = 0;
 my $root;
 my %debug;
 GetOptions(
@@ -42,6 +43,7 @@ GetOptions(
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
 	'summary-file!'	=> \$summary_file,
+	'noblacklist'	=> \$noblacklist,
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
@@ -61,6 +63,7 @@ if ($#ARGV < 0) {
 	print "         --root           => path to the kernel tree root\n";
 	print "         --no-summary     => suppress the per-file summary\n";
 	print "         --summary-file   => include the filename in summary\n";
+	print "         --noblacklist    => enable blacklisted file checking\n";
 	exit(1);
 }
 
@@ -99,6 +102,16 @@ if ($tree) {
 	}
 }
 
+# This blacklist should be used to remove errors that certain maintainers have
+# ordained as good for whatever reason. This list should not get very long.
+my @blacklist = (
+	# ftrace uses large numbers of spaces and tabs to space out certain
+	# macro in the include files. It's known, and it's doubtful any clean
+	# up there will be accepted.
+	[ 'include/trace/events/', 'space prohibited after that open parenthesis'],
+	[ 'include/trace/events/', 'space prohibited before that close parenthes'],
+);
+
 my $emitted_corrupt = 0;
 
 our $Ident       = qr{[A-Za-z_][A-Za-z\d_]*};
@@ -1005,6 +1018,19 @@ sub report {
 	if (defined $tst_only && $_[0] !~ /\Q$tst_only\E/) {
 		return 0;
 	}
+
+	# Check that this code isn't in the black list.
+	if (!$noblacklist) {
+		for my $blacked_out (@blacklist) {
+			my $file = ${$blacked_out}[0];
+			my $msg = ${$blacked_out}[1];
+
+			if ($_[0] =~ /FILE:\s$file/m && $_[0] =~ /$msg/m) {
+				our $cnt_blacklisted++;
+				return 0;
+			}
+		}
+	}
 	my $line = $prefix . $_[0];
 
 	$line = (split('\n', $line))[0] . "\n" if ($terse);
@@ -1085,6 +1111,7 @@ sub process {
 	our $cnt_error = 0;
 	our $cnt_warn = 0;
 	our $cnt_chk = 0;
+	our $cnt_blacklisted = 0;
 
 	# Trace the real file/line as we go.
 	my $realfile = '';
@@ -1243,9 +1270,11 @@ sub process {
 		# extract the filename as it passes
 		if ($line=~/^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
-			$realfile =~ s@^([^/]*)/@@;
+			if (!$file) {
+				$realfile =~ s@^([^/]*)/@@;
+				$p1_prefix = $1;
+			}
 
-			$p1_prefix = $1;
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
 				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
@@ -2606,6 +2635,9 @@ sub process {
 			"$cnt_lines lines checked\n";
 		print "\n" if ($quiet == 0);
 	}
+	if ($cnt_blacklisted != 0 && !$noblacklist && $quiet == 0) {
+		print "NOTE: $cnt_blacklisted errors have been removed due to the blacklist.\n\n"
+	}
 
 	if ($clean == 1 && $quiet == 0) {
 		print "$vname has no obvious style problems and is ready for submission.\n"
-- 
1.5.6.3

[PATCH 4/5] checkpatch: fix __attribute__ matching

[Daniel Walker]

In the following code,

union thread_union init_thread_union
	__attribute__((__section__(".data.init_task"))) =
		{ INIT_THREAD_INFO(init_task) };

There is a non-conforming declaration. It should really be like the
following,

union thread_union init_thread_union
	__attribute__((__section__(".data.init_task"))) = {
		INIT_THREAD_INFO(init_task)
};

However, checkpatch doesn't catch this right now because it doesn't
correctly evaluate the "__attribute__". I just fixed it to pattern
match the attribute in the case above.

Signed-off-by: Daniel Walker <dwalker@fifo99.com>
---
 scripts/checkpatch.pl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c7f741f..fd4fe03 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -134,7 +134,7 @@ our $Attribute	= qr{
 			____cacheline_aligned|
 			____cacheline_aligned_in_smp|
 			____cacheline_internodealigned_in_smp|
-			__weak
+			__weak|(?:__attribute__\(.*\))
 		  }x;
 our $Modifier;
 our $Inline	= qr{inline|__always_inline|noinline};
@@ -1628,7 +1628,7 @@ sub process {
 		}
 
 # check for initialisation to aggregates open brace on the next line
-		if ($prevline =~ /$Declare\s*$Ident\s*=\s*$/ &&
+		if (($prevline =~ /$Declare\s*$Ident\s*=\s*$/ || $prevline =~ /$Attribute\s*=\s*$/) &&
 		    $line =~ /^.\s*{/) {
 			ERROR("that open brace { should be on the previous line\n" . $hereprev);
 		}
-- 
1.5.6.3

[PATCH 5/5] checkpatch: fix false EXPORT_SYMBOL warning

[Daniel Walker]

Ingo reported that the following lines triggered a false warning,

static struct lock_class_key rcu_lock_key;
struct lockdep_map rcu_lock_map =
        STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key);
EXPORT_SYMBOL_GPL(rcu_lock_map);

from kernel/rcutree.c , and the false warning looked like this,

WARNING: EXPORT_SYMBOL(foo); should immediately follow its
function/variable
+EXPORT_SYMBOL_GPL(rcu_lock_map);

This change corrects this. It was caused because checkpatch doesn't check
more than one line above the "EXPORT_SYMBOL" for additional context (ie.
variable name, or initializer). Things are somewhat more complicated
because STATIC_LOCKDEP_MAP_INIT() doesn't accept the variable name that
is being initialized. I just added another check that checks two lines
above "EXPORT_SYMBOL" for the variable declaration.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Daniel Walker <dwalker@fifo99.com>
---
 scripts/checkpatch.pl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fd4fe03..9996bfb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1629,7 +1629,7 @@ sub process {
 
 # check for initialisation to aggregates open brace on the next line
 		if (($prevline =~ /$Declare\s*$Ident\s*=\s*$/ || $prevline =~ /$Attribute\s*=\s*$/) &&
-		    $line =~ /^.\s*{/) {
+		     $line =~ /^.\s*{/) {
 			ERROR("that open brace { should be on the previous line\n" . $hereprev);
 		}
 
@@ -1665,7 +1665,9 @@ sub process {
 				^.LIST_HEAD\(\Q$name\E\)|
 				^.$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(|
 				\b\Q$name\E(?:\s+$Attribute)?\s*(?:;|=|\[)
-			    )/x) {
+			    )/x && defined $lines[$linenr - 3] &&
+			    $lines[$linenr - 3] !~ /(?:\b\Q$name\E(?:\s+$Attribute)?\s*(?:;|=|\[))/
+				) {
 				WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
 			}
 		}
-- 
1.5.6.3

Re: [PATCH 5/5] checkpatch: fix false EXPORT_SYMBOL warning

[Andy Whitcroft]

On Mon, Sep 21, 2009 at 07:14:51PM -0700, Daniel Walker wrote:
> Ingo reported that the following lines triggered a false warning,
> 
> static struct lock_class_key rcu_lock_key;
> struct lockdep_map rcu_lock_map =
>         STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key);
> EXPORT_SYMBOL_GPL(rcu_lock_map);
> 
> from kernel/rcutree.c , and the false warning looked like this,
> 
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its
> function/variable
> +EXPORT_SYMBOL_GPL(rcu_lock_map);
> 
> This change corrects this. It was caused because checkpatch doesn't check
> more than one line above the "EXPORT_SYMBOL" for additional context (ie.
> variable name, or initializer). Things are somewhat more complicated
> because STATIC_LOCKDEP_MAP_INIT() doesn't accept the variable name that
> is being initialized. I just added another check that checks two lines
> above "EXPORT_SYMBOL" for the variable declaration.

In theory the thing we are exporting can be an arbitrary number of lines
prior to the EXPORT_SYMBOL statement.  We actually want to look at the
statement before the EXPORT_*. 

I had a go at doing it this way and it seems to work on my test sets.
Perhaps you could test the version at the url below:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

NOTE: you want at least version 0.29-5-* which is in the process of 
mirroring out.

-apw

Re: [PATCH 5/5] checkpatch: fix false EXPORT_SYMBOL warning

[Daniel Walker]

On Wed, 2009-09-30 at 18:46 +0100, Andy Whitcroft wrote:

> In theory the thing we are exporting can be an arbitrary number of lines
> prior to the EXPORT_SYMBOL statement.  We actually want to look at the
> statement before the EXPORT_*. 

Why not maintain a variable that holds the name of the function of
structure that is currently getting parsed .. So that you wouldn't need
to look back X lines to find anything ? Or did you do that in the
0.29-5-* version?

Daniel

Re: [PATCH 5/5] checkpatch: fix false EXPORT_SYMBOL warning

[Andy Whitcroft]

On Thu, Oct 01, 2009 at 07:28:11AM -0700, Daniel Walker wrote:
> On Wed, 2009-09-30 at 18:46 +0100, Andy Whitcroft wrote:
> 
> > In theory the thing we are exporting can be an arbitrary number of lines
> > prior to the EXPORT_SYMBOL statement.  We actually want to look at the
> > statement before the EXPORT_*. 
> 
> Why not maintain a variable that holds the name of the function of
> structure that is currently getting parsed .. So that you wouldn't need
> to look back X lines to find anything ? Or did you do that in the
> 0.29-5-* version?

We already have the concept of the current statement which is used
mostly for conditional handling.  I leverage that to say if the 'next'
statement is an EXPORT_SYMBOL_* does this statement have anything to say
about the exported symbol name.  Seems to work better.

-apw

Re: [PATCH 4/5] checkpatch: fix __attribute__ matching

[Andy Whitcroft]

On Mon, Sep 21, 2009 at 07:14:50PM -0700, Daniel Walker wrote:
> In the following code,
> 
> union thread_union init_thread_union
> 	__attribute__((__section__(".data.init_task"))) =
> 		{ INIT_THREAD_INFO(init_task) };
> 
> There is a non-conforming declaration. It should really be like the
> following,
> 
> union thread_union init_thread_union
> 	__attribute__((__section__(".data.init_task"))) = {
> 		INIT_THREAD_INFO(init_task)
> };
> 
> However, checkpatch doesn't catch this right now because it doesn't
> correctly evaluate the "__attribute__". I just fixed it to pattern
> match the attribute in the case above.
> 
> Signed-off-by: Daniel Walker <dwalker@fifo99.com>
> ---
>  scripts/checkpatch.pl |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c7f741f..fd4fe03 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -134,7 +134,7 @@ our $Attribute	= qr{
>  			____cacheline_aligned|
>  			____cacheline_aligned_in_smp|
>  			____cacheline_internodealigned_in_smp|
> -			__weak
> +			__weak|(?:__attribute__\(.*\))

The problem with the __attribute__ match is that it is impossible to
sensibly write as a regular-expression as it has nested round brackets
within it.  I do wonder why we care what is before the equals.  I
suspect that any assignment ='s followed by a newline, followed by a {
is wrong.  There are few places that a { is right on the next line.

I'll try that one out and see if it fires any false positives.  Its
passing my tests here.

Could you see if the version at the url below works better for you:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

NOTE: you want at least version 0.29-5-* which is in the process of 
mirroring out.

-apw

Re: [PATCH 4/5] checkpatch: fix __attribute__ matching

[Daniel Walker]

On Wed, 2009-09-30 at 18:46 +0100, Andy Whitcroft wrote:

> The problem with the __attribute__ match is that it is impossible to
> sensibly write as a regular-expression as it has nested round brackets
> within it.  I do wonder why we care what is before the equals.  I
> suspect that any assignment ='s followed by a newline, followed by a {
> is wrong.  There are few places that a { is right on the next line.

Yeah, I was thinking about that also .. I though there might be some
"= {" situation I wasn't thinking of tho.

> I'll try that one out and see if it fires any false positives.  Its
> passing my tests here.
> 
> Could you see if the version at the url below works better for you:
> 
>   http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

I'm wondering about your release cycle .. You seem to be selectively
sending patches to Andrew ? Have you considered putting all your changes
into Linux-Next for instance then just keep up with the merge-window
cycle ? Either that or send everything to Andrew.. Either way, you would
have all the changes getting tested, instead of something like above
where is "testing" or a version number at an obscure url location..

Daniel

Re: [PATCH 4/5] checkpatch: fix __attribute__ matching

[Andy Whitcroft]

On Thu, Oct 01, 2009 at 07:26:12AM -0700, Daniel Walker wrote:
> On Wed, 2009-09-30 at 18:46 +0100, Andy Whitcroft wrote:
> 
> > The problem with the __attribute__ match is that it is impossible to
> > sensibly write as a regular-expression as it has nested round brackets
> > within it.  I do wonder why we care what is before the equals.  I
> > suspect that any assignment ='s followed by a newline, followed by a {
> > is wrong.  There are few places that a { is right on the next line.
> 
> Yeah, I was thinking about that also .. I though there might be some
> "= {" situation I wasn't thinking of tho.
> 
> > I'll try that one out and see if it fires any false positives.  Its
> > passing my tests here.
> > 
> > Could you see if the version at the url below works better for you:
> > 
> >   http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing
> 
> I'm wondering about your release cycle .. You seem to be selectively
> sending patches to Andrew ? Have you considered putting all your changes
> into Linux-Next for instance then just keep up with the merge-window
> cycle ? Either that or send everything to Andrew.. Either way, you would
> have all the changes getting tested, instead of something like above
> where is "testing" or a version number at an obscure url location..

Linux-next might also make sense, though generally I'd seen it as an
integration test bed to catch cross tree merge conflicts and I don't
generally have that issue.  There is a wrinkle that my checkpatch tree
is separate tree because it contains a large test suite and that really
isn't something we likely want in the kernel tree itself.  I will look
at generating some real linux based branches from my tree and pushing
those to g.k.o which would be suitable for pulling into -next.

I have been distracted lately getting up to speed in a new role and that
has impacted the regular flow of checkpatch stuff.  I am hoping to get
back to normal service there.

-apw

Re: [PATCH 3/5] checkpatch: add a blacklist

[Li Zefan]

Daniel Walker wrote:
> There are times when maintainers intentially don't follow the coding
> style. When that happens it means some errors need to be ignored, so
> that other errors can be focused on.
> 
> To handle that I added a blacklist to checkpatch. The blacklist holds the
> file names and errors which are ignored. The output is modified to
> remove the errors from the list and not to count them.
> 
> When the blacklist kicks in there is a note that does list how many
> errors got removed and that it was due to a blacklist entry. There is
> also a new option "--noblacklist" that allows the errors to be added
> back as it was without the blacklist.
> 

So, for this piece of code:

TRACE_EVENT(...

	TP_fast_assign(
		__entry->foo = bar( xxx );
	),
)

checkpatch won't report the spaces inside bar()?
If so, I don't like this patch.

Could you just teach checkpatch to recognize those macros used
in TRACE_EVENT(), if those coding-style "errors" bother you
so much that you can't put up with them?

Re: [PATCH 3/5] checkpatch: add a blacklist

[Andy Whitcroft]

On Tue, Sep 22, 2009 at 02:29:37PM +0800, Li Zefan wrote:
> Daniel Walker wrote:
> > There are times when maintainers intentially don't follow the coding
> > style. When that happens it means some errors need to be ignored, so
> > that other errors can be focused on.
> > 
> > To handle that I added a blacklist to checkpatch. The blacklist holds the
> > file names and errors which are ignored. The output is modified to
> > remove the errors from the list and not to count them.
> > 
> > When the blacklist kicks in there is a note that does list how many
> > errors got removed and that it was due to a blacklist entry. There is
> > also a new option "--noblacklist" that allows the errors to be added
> > back as it was without the blacklist.
> > 
> 
> So, for this piece of code:
> 
> TRACE_EVENT(...
> 
> 	TP_fast_assign(
> 		__entry->foo = bar( xxx );
> 	),
> )
> 
> checkpatch won't report the spaces inside bar()?
> If so, I don't like this patch.
> 
> Could you just teach checkpatch to recognize those macros used
> in TRACE_EVENT(), if those coding-style "errors" bother you
> so much that you can't put up with them?


Yeah I think that blanket ignoring spacing throughout the file seems
dangerous.  If these are going to show up a lot then it seems more sensible
to special case TRACE_EVENT or whatever is triggering the actual 'false'
matches.  I also suspect the 'this should never get long' argument will
not be true.  Once you can have an exception people will add them all over

Care to share an example of a change which is triggereing so we can
better target the exception.

-apw

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-09-30 at 16:27 +0100, Andy Whitcroft wrote:

> Yeah I think that blanket ignoring spacing throughout the file seems
> dangerous.  If these are going to show up a lot then it seems more sensible
> to special case TRACE_EVENT or whatever is triggering the actual 'false'
> matches.  I also suspect the 'this should never get long' argument will
> not be true.  Once you can have an exception people will add them all over

It's not ignoring all spacing .. It's just ignoring a single error in a
single directory (or single file).. So it's very specific.. If you did
just match TRACE_EVENT like you suggest then what happens when another
different call in the same code has similar spacing , which could easily
happen.. You also are basically matching a style defect, which doesn't
make much sense to me.. Then one day the person that added the errors
has a revelation , and removes all the errors. Then all the work that
went into the matching is poof worthless.. This list could get to be
10-20 items lots (still not that long) , and writing individual matching
for each of those items and maintaining it would be more work that is
necessary ..

In terms of the list getting long or not, your basically in control of
it since you maintain checkpatch .. If you leave it without some sort of
blacklist, then you end up with whole sections of code where the
developers don't use checkpatch at all (or very little)..

> Care to share an example of a change which is triggereing so we can
> better target the exception.

Basically any file in include/trace/event/ will trigger the blacklist
(listed in the perl code along with the errors that are filtered out)..

In include/trace/events/ext4.h for example the following code,

TRACE_EVENT(ext4_free_inode,
        TP_PROTO(struct inode *inode),

        TP_ARGS(inode),

        TP_STRUCT__entry(
                __field(        dev_t,  dev                     )
                __field(        ino_t,  ino                     )
                __field(        umode_t, mode                   )
                __field(        uid_t,  uid                     )
                __field(        gid_t,  gid                     )
                __field(        blkcnt_t, blocks                )
        ),

        TP_fast_assign(
                __entry->dev    = inode->i_sb->s_dev;
                __entry->ino    = inode->i_ino;
                __entry->mode   = inode->i_mode;
                __entry->uid    = inode->i_uid;
                __entry->gid    = inode->i_gid;
                __entry->blocks = inode->i_blocks;
        ),

        TP_printk("dev %s ino %lu mode %d uid %u gid %u blocks %llu",
                  jbd2_dev_to_name(__entry->dev), (unsigned long) __entry->ino,
                  __entry->mode, __entry->uid, __entry->gid,
                  (unsigned long long) __entry->blocks)
);



Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Steven Rostedt]

On Thu, 2009-10-01 at 07:18 -0700, Daniel Walker wrote:
> On Wed, 2009-09-30 at 16:27 +0100, Andy Whitcroft wrote:

>         TP_STRUCT__entry(
>                 __field(        dev_t,  dev                     )
>                 __field(        ino_t,  ino                     )
>                 __field(        umode_t, mode                   )
>                 __field(        uid_t,  uid                     )
>                 __field(        gid_t,  gid                     )
>                 __field(        blkcnt_t, blocks                )
>         ),

Yes, the whitespaces for __field and __array as well as __field_ext and
__dynamic_array, and __sting, in this file can be ignored. As for the
rest of the file, perhaps it would be good to still check them.

-- Steve

Re: [PATCH 3/5] checkpatch: add a blacklist

[Krzysztof Halasa]

Daniel Walker <dwalker@fifo99.com> writes:

> In terms of the list getting long or not, your basically in control of
> it since you maintain checkpatch .. If you leave it without some sort of
> blacklist, then you end up with whole sections of code where the
> developers don't use checkpatch at all (or very little)..

Why? I routinely ignore specific warnings from checkpatch while paying
attention to other ones. That's precisely what one should expect if the
code in question technically violates the common style (or whatever is
it). I don't think we need exceptions in the checkpatch as they would
make it less useful and less reliable.
-- 
Krzysztof Halasa

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Tue, 2009-10-06 at 22:50 +0200, Krzysztof Halasa wrote:
> Daniel Walker <dwalker@fifo99.com> writes:
> 
> > In terms of the list getting long or not, your basically in control of
> > it since you maintain checkpatch .. If you leave it without some sort of
> > blacklist, then you end up with whole sections of code where the
> > developers don't use checkpatch at all (or very little)..
> 
> Why? I routinely ignore specific warnings from checkpatch while paying
> attention to other ones. That's precisely what one should expect if the
> code in question technically violates the common style (or whatever is
> it). I don't think we need exceptions in the checkpatch as they would
> make it less useful and less reliable.

This thread is specifically about checkpatch errors .. checkpatch
warnings can be ignored, but errors you can't usually ignore.. If your
ignoring errors then either checkpatch is producing bogus output that
needs to be corrected, or it's something you really should fix..

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Krzysztof Halasa]

Daniel Walker <dwalker@fifo99.com> writes:

> This thread is specifically about checkpatch errors .. checkpatch
> warnings can be ignored, but errors you can't usually ignore..

Of course I can and do :-)

> If your
> ignoring errors then either checkpatch is producing bogus output that
> needs to be corrected, or it's something you really should fix..

Neither.
But unfortunately I don't have examples handy.

My POV must be a bit different: I treat errors like another class of
warnings (perhaps more important that "mere" warnings but still not
authoritative).

This is BTW precisely what is needed WRT to that chunk of code
(include/trace/events/ext4.h, I assume checkpatch produces "error"
there) - though I think I'd format it a bit differently.


Perhaps checkpatch should stop producing "errors" (which are meaningless
as checkpatch has no authority to veto anything - a human has to decide)
and should simply give some severity code?
OTOH I ignore error/warning distinction completely, perhaps the
distinction is bogus? Not sure.
-- 
Krzysztof Halasa

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 12:17 +0200, Krzysztof Halasa wrote:
> Daniel Walker <dwalker@fifo99.com> writes:
> 
> > This thread is specifically about checkpatch errors .. checkpatch
> > warnings can be ignored, but errors you can't usually ignore..
> 
> Of course I can and do :-)
> 
> > If your
> > ignoring errors then either checkpatch is producing bogus output that
> > needs to be corrected, or it's something you really should fix..
> 
> Neither.
> But unfortunately I don't have examples handy.
> 
> My POV must be a bit different: I treat errors like another class of
> warnings (perhaps more important that "mere" warnings but still not
> authoritative).

>From my perspective Documentation/SubmittingPatches really dictates what
you should be doing with checkpatch, since that was signed off on by
Andy (on this thread) and Linus .. In that document I think checkpatch
is given authority, rather than what your suggesting where it's just
something you can use or not, and ignore or not like it has no meaning
at all..

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Krzysztof Halasa]

Daniel Walker <dwalker@fifo99.com> writes:

>>From my perspective Documentation/SubmittingPatches really dictates what
> you should be doing with checkpatch, since that was signed off on by
> Andy (on this thread) and Linus .. In that document I think checkpatch
> is given authority, rather than what your suggesting where it's just
> something you can use or not, and ignore or not like it has no meaning
> at all..

Checkpatch is a tool. How a tool can have authority? Code authors can
have authority. Maintainers can have authority. Linus as the "top"
maintainer can have authority. But a tool?

If checkpatch had any authority, the file in question (ext4.h) would
have to be "fixed" without questions and exceptions.

I don't say its warnings and errors have no meaning at all. It may be
very helpful at times, but still it's only a tool.
-- 
Krzysztof Halasa

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 16:44 +0200, Krzysztof Halasa wrote:
> Daniel Walker <dwalker@fifo99.com> writes:
> 
> >>From my perspective Documentation/SubmittingPatches really dictates what
> > you should be doing with checkpatch, since that was signed off on by
> > Andy (on this thread) and Linus .. In that document I think checkpatch
> > is given authority, rather than what your suggesting where it's just
> > something you can use or not, and ignore or not like it has no meaning
> > at all..
> 
> Checkpatch is a tool. How a tool can have authority? Code authors can
> have authority. Maintainers can have authority. Linus as the "top"
> maintainer can have authority. But a tool?
> 
> If checkpatch had any authority, the file in question (ext4.h) would
> have to be "fixed" without questions and exceptions.
> 
> I don't say its warnings and errors have no meaning at all. It may be
> very helpful at times, but still it's only a tool.

Right it's a tool .. However, you should use it and you should follow
it. If for some reason you disagree with the tool you have to give at
least an arguable reason why, not just "It's a guide", "I don't like the
coding style." etc..

In the case of Steven's code he has an arguable reason why he's not
following checkpatch..

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Alan Cox]

> Right it's a tool .. However, you should use it and you should follow
> it. If for some reason you disagree with the tool you have to give at
> least an arguable reason why, not just "It's a guide", "I don't like the
> coding style." etc..

Those are perfectly good reasons.

> In the case of Steven's code he has an arguable reason why he's not
> following checkpatch..

Checkpatch is not very bright, it has no understanding of style beyond
playing with pattern regexps. It's a rather dim tool that helps people
get work done. (*)

When used at random to "validate" submissions to the kernel the result is
about as useful as a square wheel on a hovercraft.

Alan
(*) or as some would have it a rather dim tool used by even dimmer tools
to make noise on kernel list.

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 16:11 +0100, Alan Cox wrote:
> > Right it's a tool .. However, you should use it and you should follow
> > it. If for some reason you disagree with the tool you have to give at
> > least an arguable reason why, not just "It's a guide", "I don't like the
> > coding style." etc..
> 
> Those are perfectly good reasons.

I don't think they are.. Like adding spaces for tabs is ok cause
checkpatch is a guide right?

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Alan Cox]

On Wed, 07 Oct 2009 08:41:08 -0700
Daniel Walker <dwalker@fifo99.com> wrote:

> On Wed, 2009-10-07 at 16:11 +0100, Alan Cox wrote:
> > > Right it's a tool .. However, you should use it and you should follow
> > > it. If for some reason you disagree with the tool you have to give at
> > > least an arguable reason why, not just "It's a guide", "I don't like the
> > > coding style." etc..
> > 
> > Those are perfectly good reasons.
> 
> I don't think they are.. Like adding spaces for tabs is ok cause
> checkpatch is a guide right?

Yes.. if it was such a big deal someone would have updated the git commit
tools to fix them as we do to strim off trailing spaces.

They have *zero* impact on performance, maintainability or readability
providing they don't mess up the indentation - and the kernel is full of
them.

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 16:52 +0100, Alan Cox wrote:
> On Wed, 07 Oct 2009 08:41:08 -0700
> Daniel Walker <dwalker@fifo99.com> wrote:
> 
> > On Wed, 2009-10-07 at 16:11 +0100, Alan Cox wrote:
> > > > Right it's a tool .. However, you should use it and you should follow
> > > > it. If for some reason you disagree with the tool you have to give at
> > > > least an arguable reason why, not just "It's a guide", "I don't like the
> > > > coding style." etc..
> > > 
> > > Those are perfectly good reasons.
> > 
> > I don't think they are.. Like adding spaces for tabs is ok cause
> > checkpatch is a guide right?
> 
> Yes.. if it was such a big deal someone would have updated the git commit
> tools to fix them as we do to strim off trailing spaces.
> 
> They have *zero* impact on performance, maintainability or readability
> providing they don't mess up the indentation - and the kernel is full of
> them.

It has an impact on maintainability and readability.. Take a source file
with tabs, then remove all the tabs and insert spaces. It's pretty
annoying and I would not want to touch code like that. Trailing
whitespace is a lot less annoying than using spaces instead of tabs.

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Steven Rostedt]

On Wed, 2009-10-07 at 07:26 -0700, Daniel Walker wrote:
> On Wed, 2009-10-07 at 12:17 +0200, Krzysztof Halasa wrote:
> > Daniel Walker <dwalker@fifo99.com> writes:
> > 
> > > This thread is specifically about checkpatch errors .. checkpatch
> > > warnings can be ignored, but errors you can't usually ignore..
> > 
> > Of course I can and do :-)
> > 
> > > If your
> > > ignoring errors then either checkpatch is producing bogus output that
> > > needs to be corrected, or it's something you really should fix..
> > 
> > Neither.
> > But unfortunately I don't have examples handy.
> > 
> > My POV must be a bit different: I treat errors like another class of
> > warnings (perhaps more important that "mere" warnings but still not
> > authoritative).
> 
> >From my perspective Documentation/SubmittingPatches really dictates what
> you should be doing with checkpatch, since that was signed off on by
> Andy (on this thread) and Linus .. In that document I think checkpatch
> is given authority, rather than what your suggesting where it's just
> something you can use or not, and ignore or not like it has no meaning
> at all..

Daniel,

This is getting old. You've successfully entered the /dev/null folder to
several major developers.

The checkpatch.pl script is a very useful tool. I run it on all my
patches to make sure that I don't have any silly formatting errors. It
even catches some real bugs now and then.

That said, if we really wanted to have checkpatch as a authoritative
tool, it would be executed by a bot on all patches submitted to LKML
(which you seem to have put on yourself to do). But if Linus or others
wanted that, they would have set it up.

We assume that the maintainers of the system are competent enough to
keep a decent formatting style that conforms to the rest of the kernel.
There are some instances that the style may change to cover cases that
are unique, like the events headers.

Really it should be up to the maintainer to tell a submitter that they
need to run checkpatch. You are coming out as the checkpatch Nazi leader
to "enforce" your will of the tool on others. And when they tell you,
it's not that big of a deal, you have a conniption.

So my advice to you is to take a chill pill (they come in chewables) and
relax a bit on this topic. If you had just sent out some nice emails to
obvious breakage in patches, then it would have been fine. But you are
coming across a bit too authoritarian, and it is becoming quite
annoying.

-- Steve

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 11:08 -0400, Steven Rostedt wrote:
> Daniel,
> 
> This is getting old. You've successfully entered the /dev/null folder to
> several major developers.

Getting into a /dev/null folder for code comments is just absolutely
insane to me. Any one that puts me into /dev/null has some pretty low
tolerances ..

What's getting old exactly ? The fact that Krzysztof and I are having a
discussion about this?

> The checkpatch.pl script is a very useful tool. I run it on all my
> patches to make sure that I don't have any silly formatting errors. It
> even catches some real bugs now and then.
> 
> That said, if we really wanted to have checkpatch as a authoritative
> tool, it would be executed by a bot on all patches submitted to LKML
> (which you seem to have put on yourself to do). But if Linus or others
> wanted that, they would have set it up.

You have a much different impression of this list than I do.. From my
perspective this list is made up of 1000's of people each having their
own agenda.. I have an agenda , you have one, everyone has one of their
own.. By saying "if Linus or others wanted that, they would have set it
up." . Your basically saying that only some "cool" people can have
specific agenda's and some (me) can't have agenda's , which to me is
totally bogus and wrong..

You had your chance to comments on my activity already, and did I take
your advice or anyones advice from this list? Do you see lots of emails
from me on checkpatch errors constantly??

> We assume that the maintainers of the system are competent enough to
> keep a decent formatting style that conforms to the rest of the kernel.
> There are some instances that the style may change to cover cases that
> are unique, like the events headers.

I don't totally disagree with that, however as I'm telling Krzysztof
even maintainers should have a good reason why they are deviating from
it.

> Really it should be up to the maintainer to tell a submitter that they
> need to run checkpatch. You are coming out as the checkpatch Nazi leader
> to "enforce" your will of the tool on others. And when they tell you,
> it's not that big of a deal, you have a conniption.

conniption? I hope your joking.. I argue sure, which is my right to do..
Clearly I can't force people to do anything, like I can't force you to
change your events header files. I gave you an alternative, you didn't
use it, and there is nothing else I can do about it..

> So my advice to you is to take a chill pill (they come in chewables) and
> relax a bit on this topic. If you had just sent out some nice emails to
> obvious breakage in patches, then it would have been fine. But you are
> coming across a bit too authoritarian, and it is becoming quite
> annoying.

Well there is a potentially easy way for you to stop me.. All you have
to do is write a patch that modifies Documentation/SubmittingPatches .
I'm not trying to bluff you at all, I fully expect you to submit a patch
that changes that .. If it goes in then that's what I will follow. 

You'll notice also I'm not sending many emails recently on this subject
right? It's like you want to harp on this more than I do ..

Just relax the submission rules so that checkpatch is basically an
optional part of the submission process. Adding that you don't actually
need to run it, you don't need a good reason not to follow the rules
etc.. Or expand on it to fully explain what you think the deal is or
should be.

Daniel

Re: [PATCH 3/5] checkpatch: add a blacklist

[Krzysztof Halasa]

Daniel Walker <dwalker@fifo99.com> writes:

> Just relax the submission rules so that checkpatch is basically an
> optional part of the submission process. Adding that you don't actually
> need to run it, you don't need a good reason not to follow the rules
> etc.. Or expand on it to fully explain what you think the deal is or
> should be.

Oh come on... The SubmittingPatches is a HOWTO-style document for people
who want to get their patches merged. Obviously a common sense dictates
you need a good reason to ignore this or that. "Looks better" and
"I find it easier to work with" are good reasons since this is source
code, for humans to work with.

BTW, the file says:

"Check your patches with the patch style checker prior to submission
(scripts/checkpatch.pl).  The style checker should be viewed as
a guide not as the final word.  If your code looks better with
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
a violation then its probably best left alone.

The checker reports at three levels:
 - ERROR: things that are very likely to be wrong
                               ^^^^^^
 - WARNING: things requiring careful review
 - CHECK: things requiring thought"

Only "very likely".

WRT tabs vs spaces, I wonder if using only spaces would be a better
idea. Theoretically using tabs for syntactic indentation only is better,
but the tools (editors) aren't up to the task yet.
-- 
Krzysztof Halasa

Re: [PATCH 3/5] checkpatch: add a blacklist

[Daniel Walker]

On Wed, 2009-10-07 at 23:30 +0200, Krzysztof Halasa wrote:
> Daniel Walker <dwalker@fifo99.com> writes:
> 
> > Just relax the submission rules so that checkpatch is basically an
> > optional part of the submission process. Adding that you don't actually
> > need to run it, you don't need a good reason not to follow the rules
> > etc.. Or expand on it to fully explain what you think the deal is or
> > should be.
> 
> Oh come on... The SubmittingPatches is a HOWTO-style document for people
> who want to get their patches merged. Obviously a common sense dictates
> you need a good reason to ignore this or that. "Looks better" and
> "I find it easier to work with" are good reasons since this is source
> code, for humans to work with.

The whole point of this black list is to allow people to give good
reasons why they don't follow the tool.. Once in the blacklist those
errors that are getting ignored become formally ignored.. So I haven't
been against people having a good reason not to fix the errors, but you
still need a good reason. I've checked 1000's of patches and Steven's
code is the only one that I've found which uniformly violates the coding
style ..

> BTW, the file says:
> 
> "Check your patches with the patch style checker prior to submission
> (scripts/checkpatch.pl).  The style checker should be viewed as
> a guide not as the final word.  If your code looks better with
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> a violation then its probably best left alone.

I think that exists since the tool can parse incorrectly sometimes and
flag things that it shouldn't be flagging (often times are checkpatch
defects).. For instance a large macro that only creates a function would
not need to be wrapped in a "do { } while(0);" .. So in those cases the
tool can't be the final word, this list (or a list) needs to be the
final word. Usually in those cases there is no need to explain the error
since it's clear what's happening in the code..

> The checker reports at three levels:
>  - ERROR: things that are very likely to be wrong
>                                ^^^^^^

Well, "very likely" is pretty strong wording for a this kind of tool ..

>  - WARNING: things requiring careful review
>  - CHECK: things requiring thought"
> 
> Only "very likely".
> 
> WRT tabs vs spaces, I wonder if using only spaces would be a better
> idea. Theoretically using tabs for syntactic indentation only is better,
> but the tools (editors) aren't up to the task yet.

You can usually change your editor to conform .. I know emacs usually
does spaces for tabs, but I'm sure you can change it's config to use
real tabs .. I don't use emacs tho.

Daniel

Re: [PATCH 2/5] checkpatch: fix hang in relative indent checking

[Andy Whitcroft]

On Mon, Sep 21, 2009 at 07:14:48PM -0700, Daniel Walker wrote:
> I ran this command on v2.6.31 ,
> 
> ./scripts/checkpatch.pl --file net/decnet/dn_fib.c
> 
> which resulted in checkpatch hanging without any output.
> 
> The lines that cause the hang are,
> 
> #define for_nexthops(fi) { int nhsel; const struct dn_fib_nh *nh;\
>         for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)
> 
> The hang happend in the relative indent checking code. Checkpatch has the
> following comment around the relative indent checking block,
> 
> # Also ignore a loop construct at the end of a
> # preprocessor statement.
> 
> Since the line it's hanging on exactly fits the comment it shouldn't even be
> checking this line. To resolve this I just prevent the checking like the
> comment says should happen.

Ok, this actually seems to have already been fixed in the version Andrew
already has.  Specifically it was fixed by the change in:

    checkpatch: indent checks -- stop when we run out of continuation lines

I assume this is happening with v0.28?  Could you retest that one with
the version at the URL below for me to confirm.

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-0.29

Thanks for the patch.

-apw

Re: [PATCH 1/5] checkpatch: fix false errors due to macro concatenation

[Andy Whitcroft]

On Mon, Sep 21, 2009 at 07:14:47PM -0700, Daniel Walker wrote:
> The macro concatenation (##) sequence can cause false errors when checking
> macro's. Checkpatch doesn't currently know about the operator.
> 
> For example this line,
> 
> + 	entry = (struct ftrace_raw_##call *)raw_data;                   \
> 
> is correct but it produces the following error,
> 
> ERROR: need consistent spacing around '*' (ctx:WxB)
> +       entry = (struct ftrace_raw_##call *)raw_data;\
>                                           ^
> 
> The line above doesn't have any spacing problems, and if you remove the
> macro concatenation sequence checkpatch doesn't give any errors. This change
> resolves this by just always removing "##" in every line checked.

Ok, just removing these characters in the conversion changes the
relative length of the converted form and breaks position reporting for
other checks, for instance if I stupidly convert the ## to # so its
still invalid we then get this:

+	entry = (struct ftrace_raw_##call *)raw_data; \

It is probabally more correct to include this <ident> ## <ident> form in
the definition of an identifier.  I've respun this patch to do just that
and it looks like its working as we would hope.

I will get this tested properly and add it to my next batch.

Perhaps you could test the version at the url below and see if it works
better:

  http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

NOTE: you want at least version 0.29-5-* which is in the process of
mirroring out.

-apw

Re: [PATCH 1/5] checkpatch: fix false errors due to macro concatenation

[Daniel Walker]

On Wed, 2009-09-30 at 18:46 +0100, Andy Whitcroft wrote:

> Ok, just removing these characters in the conversion changes the
> relative length of the converted form and breaks position reporting for
> other checks, for instance if I stupidly convert the ## to # so its
> still invalid we then get this:
> 
> +	entry = (struct ftrace_raw_##call *)raw_data; \
> 
> It is probabally more correct to include this <ident> ## <ident> form in
> the definition of an identifier.  I've respun this patch to do just that
> and it looks like its working as we would hope.

It could just become "__" something that would match on an ident and
keep the spacing the same.

Daniel