Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Mon, 2017-12-18 at 17:56 +, Bart Van Assche wrote:
> On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> > On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> > 
> > > > Today when we run checkers we get so many warnings it is too hard to
> > > > make any sense of it.
> > > 
> > > Here is a list of the checkpatch messages for drivers/infiniband
> > > sorted by type.
> > > 
> > > Many of these might be corrected by using
> > > 
> > > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> > >   $(git ls-files drivers/infiniband/)
> > 
> > How many of these do you think it is worth to fix?
> > 
> > We do get a steady trickle of changes in this topic every cycle.
> > 
> > Is it better to just do a big number of them all at once? Do you have
> > an idea how disruptive this kind of work is to the whole patch flow
> > eg new patches no longer applying to for-next, backports no longer
> > applying, merge conflicts?
> 
> In my opinion patches that only change the coding style and do not change any
> functionality are annoying. Before posting a patch that fixes a bug the change
> history (git log -p) has to be cheched to figure out which patch introduced
> the bug. Patches that only change coding style pollute the change history.

I agree with you - the problem is that style issues should not have existed. 
But when they do it becomes a problem to remove them and a problem to 
keep them - for instance us who try to be compliant by having style helpers 
in our editor, we end up having to manually revert old style mistakes back in
to avoid making unrelated whitespace changes or similar.

I think what we get with this patch set is that there's a way to rein in the
issues and to enable some regression testing without enforcing a lot of work 
or annoying history issues *right away*. That way this problem can be tackled 
when
appropriate for the subsystem in question, and the reason for holding back on 
some changes can be documented in the local runchecks.cfg file, focusing
willing helping hands on the issues considered most important for that 
subsystem.

Thanks,
Knut
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 02:05:15PM +0100, Knut Omang wrote:
 
> https://github.com/torvalds/linux/compare/master...knuto:runchecks

Several of these to rdma/core do not look so big, you should think
about sending them..

Jason
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Bart Van Assche]

On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> 
> > > Today when we run checkers we get so many warnings it is too hard to
> > > make any sense of it.
> > 
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> >   $(git ls-files drivers/infiniband/)
> 
> How many of these do you think it is worth to fix?
> 
> We do get a steady trickle of changes in this topic every cycle.
> 
> Is it better to just do a big number of them all at once? Do you have
> an idea how disruptive this kind of work is to the whole patch flow
> eg new patches no longer applying to for-next, backports no longer
> applying, merge conflicts?

In my opinion patches that only change the coding style and do not change any
functionality are annoying. Before posting a patch that fixes a bug the change
history (git log -p) has to be cheched to figure out which patch introduced
the bug. Patches that only change coding style pollute the change history.

Bart.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Jason Gunthorpe]

On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:

> > Today when we run checkers we get so many warnings it is too hard to
> > make any sense of it.
> 
> Here is a list of the checkpatch messages for drivers/infiniband
> sorted by type.
> 
> Many of these might be corrected by using
> 
> $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
>   $(git ls-files drivers/infiniband/)

How many of these do you think it is worth to fix?

We do get a steady trickle of changes in this topic every cycle.

Is it better to just do a big number of them all at once? Do you have
an idea how disruptive this kind of work is to the whole patch flow
eg new patches no longer applying to for-next, backports no longer
applying, merge conflicts?

Jason
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Leon Romanovsky]

On Mon, Dec 18, 2017 at 07:39:50PM +0100, Knut Omang wrote:
> On Mon, 2017-12-18 at 17:56 +, Bart Van Assche wrote:
> > On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> > > On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> > >
> > > > > Today when we run checkers we get so many warnings it is too hard to
> > > > > make any sense of it.
> > > >
> > > > Here is a list of the checkpatch messages for drivers/infiniband
> > > > sorted by type.
> > > >
> > > > Many of these might be corrected by using
> > > >
> > > > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> > > >   $(git ls-files drivers/infiniband/)
> > >
> > > How many of these do you think it is worth to fix?
> > >
> > > We do get a steady trickle of changes in this topic every cycle.
> > >
> > > Is it better to just do a big number of them all at once? Do you have
> > > an idea how disruptive this kind of work is to the whole patch flow
> > > eg new patches no longer applying to for-next, backports no longer
> > > applying, merge conflicts?
> >
> > In my opinion patches that only change the coding style and do not change 
> > any
> > functionality are annoying. Before posting a patch that fixes a bug the 
> > change
> > history (git log -p) has to be cheched to figure out which patch introduced
> > the bug. Patches that only change coding style pollute the change history.
>
> I agree with you - the problem is that style issues should not have existed.
> But when they do it becomes a problem to remove them and a problem to
> keep them - for instance us who try to be compliant by having style helpers
> in our editor, we end up having to manually revert old style mistakes back in
> to avoid making unrelated whitespace changes or similar.

If the checkpatch.pl complains about coding style for the new patch in
newly added code, I'm asking from the author to prepare cleanup patch so
it will be applied before actual patch.

In case, complains are for code which patch are not touching, I'm
submitting it as is.

Thanks


signature.asc
Description: PGP signature
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> 
> > > Today when we run checkers we get so many warnings it is too hard to
> > > make any sense of it.
> > 
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> >   $(git ls-files drivers/infiniband/)
> 
> How many of these do you think it is worth to fix?
> 
> We do get a steady trickle of changes in this topic every cycle.
> 
> Is it better to just do a big number of them all at once?

I think so.

> Do you have
> an idea how disruptive this kind of work is to the whole patch flow
> eg new patches no longer applying to for-next, backports no longer
> applying, merge conflicts?

Some do complain about backport patch purity.

I think that difficulty is overstated, but then
again, I don't do backports very often.

I think the best time for any rather wholesale
change is immediately after an rc-1 so overall
in-flight patch conflict volume is minimized.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Mon, 2017-12-18 at 07:30 -0800, Joe Perches wrote:
> On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:
> > > Here is a list of the checkpatch messages for drivers/infiniband
> > > sorted by type.
> > > 
> > > Many of these might be corrected by using
> > > 
> > > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> > >   $(git ls-files drivers/infiniband/)
> > 
> > Yes, and I already did that work piece by piece for individual types,
> > just to test the runchecks tool, and want to post that set once the 
> > runchecks script and Makefile changes itself are in,
> 
> I think those are independent of any runcheck tool changes and
> could be posted now.  In general, don't keep patches in a local
> tree waiting on some other unrelated patch.

It becomes related in that the runchecks.cfg file is updated 
in all the patches to keep 'make C=2' run with 0 errors while 
enabling more checks. I think they serve well as examples of 
how a workflow with runchecks could be.

> Just fyi:
> 
> There is a script that helps automate checkpatch "by-type" conversions
> with compilation, .o difference checking, and git commit editing.
> 
> https://lkml.org/lkml/2014/7/11/794

oh - good to know - seems it would have been a good help
during my little exercise..

Thanks,
Knut
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Sun, 2017-12-17 at 22:00 -0800, Joe Perches wrote:
> On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> > On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> > 
> > > > I like the ability to add more checkers and keep then in the main
> > > > upstream tree. But adding overrides for specific subsystems goes against
> > > > the policy that all subsystems should be treated equally.
> > > 
> > > This is a tool to enable automated testing for as many checks as
> > > possible, as soon as possible. Like any tool, it can be misused, but
> > > that's IMHO an orthogonal problem that I think the maintainers will
> > > be more than capable of preventing.
> > > 
> > > Think of this as a tightening screw: We eliminate errors class by
> > > class or file by file, and in the same commit narrows in the list of
> > > exceptions. That way we can fix issues piece by piece while avoiding
> > > a lot of regressions in already clean parts.
> > 
> > Since you used drivers/infiniband as an example for this script..
> > 
> > I will say I agree with this idea.
> > 
> > It is not that we *want* infiniband to be different from the rest of
> > the kernel, it is that we have this historical situation where we
> > don't have a code base that already passes the various static checker
> > things.
> > 
> > I would like it very much if I could run 'make static checker' and see
> > no warnings. This helps me know that I when I accept patches I am not
> > introducing new problems to code that has already been cleaned up.
> > 
> > Today when we run checkers we get so many warnings it is too hard to
> > make any sense of it.
> 
> Here is a list of the checkpatch messages for drivers/infiniband
> sorted by type.
> 
> Many of these might be corrected by using
> 
> $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
>   $(git ls-files drivers/infiniband/)

Yes, and I already did that work piece by piece for individual types,
just to test the runchecks tool, and want to post that set once the 
runchecks script and Makefile changes itself are in,

see:

https://github.com/torvalds/linux/compare/master...knuto:runchecks

What I personally like about this approach is that with the runchecks.cfg file
one can focus easily on one or two check types at a time, create a commit out 
of that
and at the same time "tighten" runchecks.cfg - changes in that file then serves 
as 
documentation for what got changed and use maintainers can use comments to 
indicate 
why the remaining exceptions are still there - and there's a one-stop for 
anyone 
wanting to contribute to checkpatch/sparse/doc fixes.

And it will be possible to run bisect with make C=2 and always have 0 errors.

Thanks,
Knut
> 
>5243 CHECK:CAMELCASE
>4487 WARNING:LONG_LINE
>1755 CHECK:PARENTHESIS_ALIGNMENT
>1664 CHECK:SPACING
> 910 WARNING:FUNCTION_ARGUMENTS
> 742 CHECK:OPEN_ENDED_LINE
> 685 CHECK:BRACES
> 643 CHECK:UNNECESSARY_PARENTHESES
> 478 WARNING:SIZEOF_PARENTHESIS
> 361 WARNING:UNSPECIFIED_INT
> 342 WARNING:LONG_LINE_COMMENT
> 338 ERROR:SPACING
> 338 CHECK:LINE_SPACING
> 306 WARNING:SPLIT_STRING
> 278 WARNING:SPACING
> 242 WARNING:SYMBOLIC_PERMS
> 194 WARNING:BLOCK_COMMENT_STYLE
> 175 CHECK:BIT_MACRO
> 158 WARNING:SPACE_BEFORE_TAB
> 154 WARNING:LINE_SPACING
> 139 CHECK:MACRO_ARG_REUSE
> 133 CHECK:UNCOMMENTED_DEFINITION
> 122 CHECK:AVOID_BUG
> 103 CHECK:COMPARISON_TO_NULL
> 101 WARNING:ENOSYS
>  89 WARNING:BRACES
>  78 WARNING:PREFER_PR_LEVEL
>  74 WARNING:MULTILINE_DEREFERENCE
>  59 CHECK:TYPO_SPELLING
>  52 WARNING:EMBEDDED_FUNCTION_NAME
>  52 CHECK:MULTIPLE_ASSIGNMENTS
>  50 CHECK:PREFER_KERNEL_TYPES
>  45 WARNING:RETURN_VOID
>  39 WARNING:UNNECESSARY_ELSE
>  38 ERROR:POINTER_LOCATION
>  37 WARNING:ALLOC_WITH_MULTIPLY
>  36 CHECK:ALLOC_SIZEOF_STRUCT
>  35 CHECK:AVOID_EXTERNS
>  34 WARNING:PRINTK_WITHOUT_KERN_LEVEL
>  33 ERROR:CODE_INDENT
>  32 WARNING:PREFER_PACKED
>  32 CHECK:LOGICAL_CONTINUATIONS
>  29 WARNING:MEMORY_BARRIER
>  29 WARNING:LEADING_SPACE
>  28 WARNING:DEEP_INDENTATION
>  27 CHECK:USLEEP_RANGE
>  23 WARNING:SUSPECT_CODE_INDENT
>  23 ERROR:TRAILING_STATEMENTS
>  21 WARNING:LONG_LINE_STRING
>  20 WARNING:CONSIDER_KSTRTO
>  18 WARNING:CONSTANT_COMPARISON
>  18 ERROR:OPEN_BRACE
>  15 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
>  14 WARNING:VOLATILE
>  14 ERROR:SWITCH_CASE_INDENT_LEVEL
>  11 WARNING:OOM_MESSAGE
>  11 WARNING:INCLUDE_LINUX
>  10 WARNING:SSCANF_TO_KSTRTO
>  10 WARNING:INDENTED_LABEL
>   9 ERROR:GLOBAL_INITIALISERS
>   9 ERROR:COMPLEX_MACRO
>   9 ERROR:ASSIGN_IN_IF
>   8 WARNING:UNNECESSARY_BREAK
>   6 WARNING:PRINTF_L
>   6 WARNING:MISORDERED_TYPE
>   6 ERROR:INITIALISED_STATIC
>   5 WARNING:TABSTOP
>   5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
>   5 WARNING:NAKED_SSCANF

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Mon, 2017-12-18 at 14:05 +0100, Knut Omang wrote:
> > Here is a list of the checkpatch messages for drivers/infiniband
> > sorted by type.
> > 
> > Many of these might be corrected by using
> > 
> > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> >   $(git ls-files drivers/infiniband/)
> 
> Yes, and I already did that work piece by piece for individual types,
> just to test the runchecks tool, and want to post that set once the 
> runchecks script and Makefile changes itself are in,

I think those are independent of any runcheck tool changes and
could be posted now.  In general, don't keep patches in a local
tree waiting on some other unrelated patch.

Just fyi:

There is a script that helps automate checkpatch "by-type" conversions
with compilation, .o difference checking, and git commit editing.

https://lkml.org/lkml/2014/7/11/794

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> 
> > > I like the ability to add more checkers and keep then in the main
> > > upstream tree. But adding overrides for specific subsystems goes against
> > > the policy that all subsystems should be treated equally.
> > 
> > This is a tool to enable automated testing for as many checks as
> > possible, as soon as possible. Like any tool, it can be misused, but
> > that's IMHO an orthogonal problem that I think the maintainers will
> > be more than capable of preventing.
> > 
> > Think of this as a tightening screw: We eliminate errors class by
> > class or file by file, and in the same commit narrows in the list of
> > exceptions. That way we can fix issues piece by piece while avoiding
> > a lot of regressions in already clean parts.
> 
> Since you used drivers/infiniband as an example for this script..
> 
> I will say I agree with this idea.

> It is not that we *want* infiniband to be different from the rest of
> the kernel, it is that we have this historical situation where we
> don't have a code base that already passes the various static checker
> things.

I poked around trying make M= in different parts of the kernel to get some 
"mileage" and to get as much examples as possible, and it appears most of 
the kernel has issues of sorts. Also Joe and others keep adding more checks 
as well, and we'd want that process to coexist with the need for automatic 
regression testing in this area.

> I would like it very much if I could run 'make static checker' and see
> no warnings. 

which is just what is the result with 'make C=2 M=drivers/infiniband/core' 
and the patches #1 and #5 in this set in case not everyone got the point,

> This helps me know that I when I accept patches I am not
> introducing new problems to code that has already been cleaned up.
> 
> Today when we run checkers we get so many warnings it is too hard to
> make any sense of it.
> 
> Being able to say File X is now clean for check XYZ seems very useful
> and may motivate people to clean up the files they actualy care
> about...
> 
> > > There was discussion at Kernel Summit about how the different
> > > subsystems already have different rules. This appears to be a way
> > > to make that worse.
> > 
> > IMHO this is a tool that should help maintainers implement the
> > policies they desire.  But the tool itself does not dictate any
> > such.
> 
> Yes, again, in infiniband we like to see checkpatch be good for new
> submission, even if that clashes with surrounding code. For instance
> we have a mixture of sizeof foo and sizeof(foo) styles in the same
> file/function now.

That's one of the fixes that the excellent --fix-inplace handles so 
one of my patches here

https://github.com/torvalds/linux/compare/master...knuto:runchecks

fixes those in drivers/infiniband/core.

> I certainly don't want to tell people they need to follow some
> different style from 10 years ago when they send patches.
> 

Thanks,
Knut

> Jason
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Jason Gunthorpe]

On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:

> > I like the ability to add more checkers and keep then in the main
> > upstream tree. But adding overrides for specific subsystems goes against
> > the policy that all subsystems should be treated equally.
> 
> This is a tool to enable automated testing for as many checks as
> possible, as soon as possible. Like any tool, it can be misused, but
> that's IMHO an orthogonal problem that I think the maintainers will
> be more than capable of preventing.
> 
> Think of this as a tightening screw: We eliminate errors class by
> class or file by file, and in the same commit narrows in the list of
> exceptions. That way we can fix issues piece by piece while avoiding
> a lot of regressions in already clean parts.

Since you used drivers/infiniband as an example for this script..

I will say I agree with this idea.

It is not that we *want* infiniband to be different from the rest of
the kernel, it is that we have this historical situation where we
don't have a code base that already passes the various static checker
things.

I would like it very much if I could run 'make static checker' and see
no warnings. This helps me know that I when I accept patches I am not
introducing new problems to code that has already been cleaned up.

Today when we run checkers we get so many warnings it is too hard to
make any sense of it.

Being able to say File X is now clean for check XYZ seems very useful
and may motivate people to clean up the files they actualy care
about...

> > There was discussion at Kernel Summit about how the different
> > subsystems already have different rules. This appears to be a way
> > to make that worse.
> 
> IMHO this is a tool that should help maintainers implement the
> policies they desire.  But the tool itself does not dictate any
> such.

Yes, again, in infiniband we like to see checkpatch be good for new
submission, even if that clashes with surrounding code. For instance
we have a mixture of sizeof foo and sizeof(foo) styles in the same
file/function now.

I certainly don't want to tell people they need to follow some
different style from 10 years ago when they send patches.

Jason
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Sun, 2017-12-17 at 22:00 -0700, Jason Gunthorpe wrote:
> On Sun, Dec 17, 2017 at 03:14:10AM +0100, Knut Omang wrote:
> 
> > > I like the ability to add more checkers and keep then in the main
> > > upstream tree. But adding overrides for specific subsystems goes against
> > > the policy that all subsystems should be treated equally.
> > 
> > This is a tool to enable automated testing for as many checks as
> > possible, as soon as possible. Like any tool, it can be misused, but
> > that's IMHO an orthogonal problem that I think the maintainers will
> > be more than capable of preventing.
> > 
> > Think of this as a tightening screw: We eliminate errors class by
> > class or file by file, and in the same commit narrows in the list of
> > exceptions. That way we can fix issues piece by piece while avoiding
> > a lot of regressions in already clean parts.
> 
> Since you used drivers/infiniband as an example for this script..
> 
> I will say I agree with this idea.
> 
> It is not that we *want* infiniband to be different from the rest of
> the kernel, it is that we have this historical situation where we
> don't have a code base that already passes the various static checker
> things.
> 
> I would like it very much if I could run 'make static checker' and see
> no warnings. This helps me know that I when I accept patches I am not
> introducing new problems to code that has already been cleaned up.
> 
> Today when we run checkers we get so many warnings it is too hard to
> make any sense of it.

Here is a list of the checkpatch messages for drivers/infiniband
sorted by type.

Many of these might be corrected by using

$ ./scripts/checkpatch.pl -f --fix-inplace --types= \
  $(git ls-files drivers/infiniband/)

   5243 CHECK:CAMELCASE
   4487 WARNING:LONG_LINE
   1755 CHECK:PARENTHESIS_ALIGNMENT
   1664 CHECK:SPACING
910 WARNING:FUNCTION_ARGUMENTS
742 CHECK:OPEN_ENDED_LINE
685 CHECK:BRACES
643 CHECK:UNNECESSARY_PARENTHESES
478 WARNING:SIZEOF_PARENTHESIS
361 WARNING:UNSPECIFIED_INT
342 WARNING:LONG_LINE_COMMENT
338 ERROR:SPACING
338 CHECK:LINE_SPACING
306 WARNING:SPLIT_STRING
278 WARNING:SPACING
242 WARNING:SYMBOLIC_PERMS
194 WARNING:BLOCK_COMMENT_STYLE
175 CHECK:BIT_MACRO
158 WARNING:SPACE_BEFORE_TAB
154 WARNING:LINE_SPACING
139 CHECK:MACRO_ARG_REUSE
133 CHECK:UNCOMMENTED_DEFINITION
122 CHECK:AVOID_BUG
103 CHECK:COMPARISON_TO_NULL
101 WARNING:ENOSYS
 89 WARNING:BRACES
 78 WARNING:PREFER_PR_LEVEL
 74 WARNING:MULTILINE_DEREFERENCE
 59 CHECK:TYPO_SPELLING
 52 WARNING:EMBEDDED_FUNCTION_NAME
 52 CHECK:MULTIPLE_ASSIGNMENTS
 50 CHECK:PREFER_KERNEL_TYPES
 45 WARNING:RETURN_VOID
 39 WARNING:UNNECESSARY_ELSE
 38 ERROR:POINTER_LOCATION
 37 WARNING:ALLOC_WITH_MULTIPLY
 36 CHECK:ALLOC_SIZEOF_STRUCT
 35 CHECK:AVOID_EXTERNS
 34 WARNING:PRINTK_WITHOUT_KERN_LEVEL
 33 ERROR:CODE_INDENT
 32 WARNING:PREFER_PACKED
 32 CHECK:LOGICAL_CONTINUATIONS
 29 WARNING:MEMORY_BARRIER
 29 WARNING:LEADING_SPACE
 28 WARNING:DEEP_INDENTATION
 27 CHECK:USLEEP_RANGE
 23 WARNING:SUSPECT_CODE_INDENT
 23 ERROR:TRAILING_STATEMENTS
 21 WARNING:LONG_LINE_STRING
 20 WARNING:CONSIDER_KSTRTO
 18 WARNING:CONSTANT_COMPARISON
 18 ERROR:OPEN_BRACE
 15 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
 14 WARNING:VOLATILE
 14 ERROR:SWITCH_CASE_INDENT_LEVEL
 11 WARNING:OOM_MESSAGE
 11 WARNING:INCLUDE_LINUX
 10 WARNING:SSCANF_TO_KSTRTO
 10 WARNING:INDENTED_LABEL
  9 ERROR:GLOBAL_INITIALISERS
  9 ERROR:COMPLEX_MACRO
  9 ERROR:ASSIGN_IN_IF
  8 WARNING:UNNECESSARY_BREAK
  6 WARNING:PRINTF_L
  6 WARNING:MISORDERED_TYPE
  6 ERROR:INITIALISED_STATIC
  5 WARNING:TABSTOP
  5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
  5 WARNING:NAKED_SSCANF
  4 WARNING:NEEDLESS_IF
  4 ERROR:RETURN_PARENTHESES
  4 CHECK:BOOL_COMPARISON
  3 WARNING:TRAILING_SEMICOLON
  3 WARNING:STATIC_CONST_CHAR_ARRAY
  3 ERROR:TRAILING_WHITESPACE
  2 WARNING:UNNECESSARY_PARENTHESES
  2 WARNING:MISSING_SPACE
  2 WARNING:LOGGING_CONTINUATION
  2 CHECK:ARCH_DEFINES
  1 WARNING:TYPECAST_INT_CONSTANT
  1 WARNING:PREFER_DEV_LEVEL
  1 WARNING:NR_CPUS
  1 WARNING:NEW_TYPEDEFS
  1 WARNING:MINMAX
  1 WARNING:MACRO_WITH_FLOW_CONTROL
  1 WARNING:LINE_CONTINUATIONS
  1 WARNING:DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON
  1 WARNING:DEFAULT_NO_BREAK
  1 WARNING:CONST_STRUCT
  1 WARNING:CONSIDER_COMPLETION
  1 ERROR:WHILE_AFTER_BRACE
  1 ERROR:ELSE_AFTER_BRACE
  1 CHECK:REDUNDANT_CODE

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:25 +0100
> Knut Omang  wrote:
> 
> > This patch series implements features to make it easier to run checkers on 
> > the
> > entire kernel as part of automatic and developer testing.
> > 
> > This is done by replacing the sparse specific setup for the C={1,2} variable
> > in the makefiles with setup for running scripts/runchecks, a new program 
> > that
> > can run any number of different "checkers". The behaviour of runchecks is
> > defined by simple "global" configuration in scripts/runchecks.cfg which can 
> > be
> > extended by local configuration applying to individual files, directories or
> > subtrees in the source.
> > 
> > It also fixes a minor issue with "checkpatch --fix-inplace" found during 
> > testing
> > (patch #3).
> > 
> > The runchecks.cfg files are parsed according to this minimal language:
> > 
> ># comments
> ># "Global configuration in scripts/runchecks.cfg:
> >checker 
> >typedef NAME regex
> >run  > 
> ># "local" configuration:
> >line_len 
> >except checkpatch_type [files ...]
> >pervasive checkpatch_type1 [checkpatch_type2 ...]
> > 
> > With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
> > look for a file named 'runchecks.cfg' in the same directory as the source 
> > file.
> > If that file exists, it will be parsed and it's local configuration applied 
> > to
> > allow suppression on a per checker, per check and per file basis.
> > If a "local" configuration does not exist, either in the source directory or
> > above, make will simply silently ignore the file.
> > 
> > The idea is that the community can work to add runchecks.cfg files to
> > directories, serving both as documentation and as a way for subsystem
> > maintainers to enforce policies and individual tastes as well as TODOs 
> > and/or
> > priorities, to make it easier for newcomers to contribute in this area. By
> > ignoring directories/subtrees without such files, automation can start right
> > away as it is trivially possible to run errorless with C=2 for the entire
> > kernel.
> > 
> > For the checker maintainers this should be a benefit as well: new
> > or improved checks would generate new errors/warnings. With automatic 
> > testing
> > for the checkers, these new checks would generate error reports and cause
> > builds to fail. Adding the new check a 'pervasive' option at the top level 
> > or
> > even for specific files, marked with a "new check - please consider fixing" 
> > comment
> > or similar would make those builds pass while documenting and making the 
> > new check
> > more visible.
> > 
> > The patches includes a documentation file with some more details.
> > 
> > This patch set has evolved from an earlier shell script implementation I 
> > made
> > as only a wrapper script around checkpatch. That version have been used for 
> > a
> > number of years on a driver project I worked on where we had automatic 
> > checkin
> > regression testing. I extended that to also run checkpatch to avoid having 
> > to
> > clean up frequent unintended whitespace changes and style violations from 
> > others...
> > 
> > I have also tested this version on some directories I am familiar with.  The
> > result of that work is available in two patch sets of 10 and 11 patches, 
> > but we
> > agreed that it would be better to post them as separate patch sets later.
> > 
> > Those patch sets illustrates how I picture the "flow" from just "reining 
> > in" the
> > checkpatch detections to actually fixing classes of checkpatch issues one by
> > one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings 
> > at
> > any commit boundary.
> > 
> > The combined set is available here:
> > 
> >git://github.com/knuto/linux.git  branch runchecks
> > 
> > I only include version 0 of runchecks.cfg in the two directories I
> > worked on here as the final two patches. These files both documents where
> > the issues are in those two directories, and can be used by the maintainer
> > to indicate to potential helpers what to focus on as I have tried to
> > illustrate by means of comments.
> > 
> > Changes from v1:
> > -
> > Based on feedback, the implementation is completely rewritten and extended.
> > Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
> > generic solution implemented in python, for any type of checkers, extendable
> > through some basic generic functionality, and for special needs by 
> > subclassing
> > the Checker class in the implementation.
> > 
> > This implementation fully supports checkpatch, sparse and
> > checkdoc == kernel-doc -none, and also has been tested with coccicheck.
> > To facilitate the same mechanism of using check types to filter what
> > checks to be suppressed, I introduced the concept of "typedefs" which allows
> > runchecks to 

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 17:27 +0100, Knut Omang wrote:
> On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> > On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > > This patch series implements features to make it easier to run checkers 
> > > on the
> > > entire kernel as part of automatic and developer testing.
> > 
> > This seems like a useful and reasonable series, thanks.
> 
> thanks, appreciated,
> 
> > Do please take Julia's grammar updates.
> 
> will do,
> 
> > How is this series to be applied?
> 
> I am open for suggestions.
> 
> Let's leave patch #3 out for now.
> 
> It's safe to apply #1 (and #2) alone.

I'd just as soon combine patch #1 and an updated 
patch #2 into a single patch and get that applied
sooner than later.

> They don't break anything by getting into the RDMA or RDS subsystem 
> without patch #1, alternatively they can go together with the set of cleanup 
> patches I
> have prepared for each of RDMA and RDS after #1 is in.

right.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 09:47 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:25 +0100
> Knut Omang  wrote:
> 
> > This patch series implements features to make it easier to run checkers on 
> > the
> > entire kernel as part of automatic and developer testing.
> > 
> > This is done by replacing the sparse specific setup for the C={1,2} variable
> > in the makefiles with setup for running scripts/runchecks, a new program 
> > that
> > can run any number of different "checkers". The behaviour of runchecks is
> > defined by simple "global" configuration in scripts/runchecks.cfg which can 
> > be
> > extended by local configuration applying to individual files, directories or
> > subtrees in the source.
[]
> I like the ability to add more checkers and keep then in the main
> upstream tree. But adding overrides for specific subsystems goes against
> the policy that all subsystems should be treated equally.
> 
> There was discussion at Kernel Summit about how the different
> subsystems already have different rules. This appears to be a
> way to make that worse.

I think that's OK and somewhat reasonable.

What is perhaps unreasonable is requiring subsystems with
a local specific style to change to some universal style.

see comments like:

https://lkml.org/lkml/2017/12/11/689
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Stephen Hemminger]

On Sat, 16 Dec 2017 15:42:25 +0100
Knut Omang  wrote:

> This patch series implements features to make it easier to run checkers on the
> entire kernel as part of automatic and developer testing.
> 
> This is done by replacing the sparse specific setup for the C={1,2} variable
> in the makefiles with setup for running scripts/runchecks, a new program that
> can run any number of different "checkers". The behaviour of runchecks is
> defined by simple "global" configuration in scripts/runchecks.cfg which can be
> extended by local configuration applying to individual files, directories or
> subtrees in the source.
> 
> It also fixes a minor issue with "checkpatch --fix-inplace" found during 
> testing
> (patch #3).
> 
> The runchecks.cfg files are parsed according to this minimal language:
> 
># comments
># "Global configuration in scripts/runchecks.cfg:
>checker 
>typedef NAME regex
>run  
># "local" configuration:
>line_len 
>except checkpatch_type [files ...]
>pervasive checkpatch_type1 [checkpatch_type2 ...]
> 
> With "make C=2" runchecks first parse the file scripts/runchecks.cfg, then
> look for a file named 'runchecks.cfg' in the same directory as the source 
> file.
> If that file exists, it will be parsed and it's local configuration applied to
> allow suppression on a per checker, per check and per file basis.
> If a "local" configuration does not exist, either in the source directory or
> above, make will simply silently ignore the file.
> 
> The idea is that the community can work to add runchecks.cfg files to
> directories, serving both as documentation and as a way for subsystem
> maintainers to enforce policies and individual tastes as well as TODOs and/or
> priorities, to make it easier for newcomers to contribute in this area. By
> ignoring directories/subtrees without such files, automation can start right
> away as it is trivially possible to run errorless with C=2 for the entire
> kernel.
> 
> For the checker maintainers this should be a benefit as well: new
> or improved checks would generate new errors/warnings. With automatic testing
> for the checkers, these new checks would generate error reports and cause
> builds to fail. Adding the new check a 'pervasive' option at the top level or
> even for specific files, marked with a "new check - please consider fixing" 
> comment
> or similar would make those builds pass while documenting and making the new 
> check
> more visible.
> 
> The patches includes a documentation file with some more details.
> 
> This patch set has evolved from an earlier shell script implementation I made
> as only a wrapper script around checkpatch. That version have been used for a
> number of years on a driver project I worked on where we had automatic checkin
> regression testing. I extended that to also run checkpatch to avoid having to
> clean up frequent unintended whitespace changes and style violations from 
> others...
> 
> I have also tested this version on some directories I am familiar with.  The
> result of that work is available in two patch sets of 10 and 11 patches, but 
> we
> agreed that it would be better to post them as separate patch sets later.
> 
> Those patch sets illustrates how I picture the "flow" from just "reining in" 
> the
> checkpatch detections to actually fixing classes of checkpatch issues one by
> one, while updating the checkpatch.cfg file(s) to have 0 errors or warnings at
> any commit boundary.
> 
> The combined set is available here:
> 
>git://github.com/knuto/linux.git  branch runchecks
> 
> I only include version 0 of runchecks.cfg in the two directories I
> worked on here as the final two patches. These files both documents where
> the issues are in those two directories, and can be used by the maintainer
> to indicate to potential helpers what to focus on as I have tried to
> illustrate by means of comments.
> 
> Changes from v1:
> -
> Based on feedback, the implementation is completely rewritten and extended.
> Instead of patches to checkpatch, and a sole checkpatch focus, it is now a
> generic solution implemented in python, for any type of checkers, extendable
> through some basic generic functionality, and for special needs by subclassing
> the Checker class in the implementation.
> 
> This implementation fully supports checkpatch, sparse and
> checkdoc == kernel-doc -none, and also has been tested with coccicheck.
> To facilitate the same mechanism of using check types to filter what
> checks to be suppressed, I introduced the concept of "typedefs" which allows
> runchecks to effectively augment the check type space of the checker in cases
> where types either are not available at all (checkdoc) or where only a few
> can be filtered out (sparse)
> 
> With this in place it also became trivial to make the look and feel similar
> for sparse and checkdoc as for checkpatch, with some optional color support

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 09:00 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 17:27 +0100, Knut Omang wrote:
> > On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> > > On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > > > This patch series implements features to make it easier to run checkers 
> > > > on the
> > > > entire kernel as part of automatic and developer testing.
> > > 
> > > This seems like a useful and reasonable series, thanks.
> > 
> > thanks, appreciated,
> > 
> > > Do please take Julia's grammar updates.
> > 
> > will do,
> > 
> > > How is this series to be applied?
> > 
> > I am open for suggestions.
> > 
> > Let's leave patch #3 out for now.
> > 
> > It's safe to apply #1 (and #2) alone.
> 
> I'd just as soon combine patch #1 and an updated 
> patch #2 into a single patch and get that applied
> sooner than later.

Ok, will do that.

> > They don't break anything by getting into the RDMA or RDS subsystem 
> > without patch #1, alternatively they can go together with the set of 
> > cleanup patches I
> > have prepared for each of RDMA and RDS after #1 is in.
> 
> right.

Thanks,
Knut
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Joe Perches]

On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> This patch series implements features to make it easier to run checkers on the
> entire kernel as part of automatic and developer testing.

This seems like a useful and reasonable series, thanks.
Do please take Julia's grammar updates.

How is this series to be applied?

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2 0/5] Support for generalized use of make C={1, 2} via a wrapper program

[Knut Omang]

On Sat, 2017-12-16 at 07:21 -0800, Joe Perches wrote:
> On Sat, 2017-12-16 at 15:42 +0100, Knut Omang wrote:
> > This patch series implements features to make it easier to run checkers on 
> > the
> > entire kernel as part of automatic and developer testing.
> 
> This seems like a useful and reasonable series, thanks.

thanks, appreciated,

> Do please take Julia's grammar updates.

will do,

> How is this series to be applied?

I am open for suggestions.

Let's leave patch #3 out for now.

It's safe to apply #1 (and #2) alone.

Patches #4 and #5 only have value once patch #1 is accepted,
and I included them in the set for documentation purposes.

They don't break anything by getting into the RDMA or RDS subsystem 
without patch #1, alternatively they can go together with the set of cleanup 
patches I
have prepared for each of RDMA and RDS after #1 is in.

Thanks,
Knut
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci