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

[Knut Omang]

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 <name>
typedef NAME regex
run <list of checkers or "all"

# "local" configuration:
line_len <n>
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
too, to make fixing issues in the code, the goal of this whole exercise,
much more pleasant IMHO :-)

Thanks,
Knut

Knut Omang (5):
runchecks: Generalize make C={1,2} to support multiple checkers
Documentation: Add doc for runchecks, a checker runner
checkpatch: Improve --fix-inplace for TABSTOP
rds: Add runchecks.cfg for net/rds
RDMA/core: Add runchecks.cfg for drivers/infiniband/core

Documentation/dev-tools/coccinelle.rst | 12 +-
Documentation/dev-tools/index.rst | 1 +-
Documentation/dev-tools/runchecks.rst | 215 ++++++++-
Documentation/dev-tools/sparse.rst | 30 +-
Documentation/kbuild/kbuild.txt | 9 +-
Makefile | 23 +-
drivers/infiniband/core/runchecks.cfg | 83 +++-
net/rds/runchecks.cfg | 76 +++-
scripts/Makefile.build | 4 +-
scripts/checkpatch.pl | 2 +-
scripts/runchecks | 734 ++++++++++++++++++++++++++-
scripts/runchecks.cfg | 63 ++-
scripts/runchecks_help.txt | 43 ++-
13 files changed, 1274 insertions(+), 21 deletions(-)
create mode 100644 Documentation/dev-tools/runchecks.rst
create mode 100644 drivers/infiniband/core/runchecks.cfg
create mode 100644 net/rds/runchecks.cfg
create mode 100755 scripts/runchecks
create mode 100644 scripts/runchecks.cfg
create mode 100644 scripts/runchecks_help.txt

base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215

[Knut Omang]

The runchecks program unifies configuration, processing
and output for multiple checker tools to make them
all run as part of the C=1 or C=2 option to make.

Currently with full support and unified behaviour for
sparse: sparse
checkpatch: scripts/checkpatch.pl
checkdoc: kernel-doc -none

In principle supported but not unified in output(yet):
coccinelle: scripts/coccicheck

Introduces a new documentation section titled
"Makefile support for running checkers"

Also updates documentation for the make C= option
in some other doc files, as the behaviour has
been changed to be less sparse specific and more
generic. The coccinelle documentation also had the
behaviour of C=1 and C=2 swapped.

Signed-off-by: Knut Omang <***@oracle.com>
Reviewed-by: Håkon Bugge <***@oracle.com>
Reviewed-by: Åsmund Østvold <***@oracle.com>
Reviewed-by: John Haxby <***@oracle.com>
---
Documentation/dev-tools/coccinelle.rst | 12 +-
Documentation/dev-tools/index.rst | 1 +-
Documentation/dev-tools/runchecks.rst | 215 ++++++++++++++++++++++++++-
Documentation/dev-tools/sparse.rst | 30 +++-
Documentation/kbuild/kbuild.txt | 9 +-
5 files changed, 257 insertions(+), 10 deletions(-)
create mode 100644 Documentation/dev-tools/runchecks.rst

diff --git a/Documentation/dev-tools/coccinelle.rst b/Documentation/dev-tools/coccinelle.rst
index 94f41c2..c98cc44 100644
--- a/Documentation/dev-tools/coccinelle.rst
+++ b/Documentation/dev-tools/coccinelle.rst
@@ -157,17 +157,19 @@ For example, to check drivers/net/wireless/ one may write::

make coccicheck M=drivers/net/wireless/

-To apply Coccinelle on a file basis, instead of a directory basis, the
-following command may be used::
+To apply Coccinelle as the only checker on a file basis,
+instead of a directory basis, the following command may be used::

- make C=1 CHECK="scripts/coccicheck"
+ make C=2 CF="--run:coccicheck"

-To check only newly edited code, use the value 2 for the C flag, i.e.::
+To check only newly edited code, use the value 1 for the C flag, i.e.::

- make C=2 CHECK="scripts/coccicheck"
+ make C=1 CF="--run:coccicheck"

In these modes, which works on a file basis, there is no information
about semantic patches displayed, and no commit message proposed.
+For more information about options in this calling mode, see
+Documentation/dev-tools/runchecks.rst .

This runs every semantic patch in scripts/coccinelle by default. The
COCCI variable may additionally be used to only apply a single
diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst
index e313925..cb4506d 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -16,6 +16,7 @@ whole; patches welcome!

coccinelle
sparse
+ runchecks
kcov
gcov
kasan
diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-tools/runchecks.rst
new file mode 100644
index 0000000..b25b3de
--- /dev/null
+++ b/Documentation/dev-tools/runchecks.rst
@@ -0,0 +1,215 @@
+.. Copyright 2017 Knut Omang <***@oracle.com>
+
+Makefile support for running checkers
+=====================================
+
+Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a
+lot of syntactic and semantic issues with the code, and is also constantly
+evolving and detecting more. In an ideal world, all source files should
+adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
+bells and whistles enabled, in a way that these checkers can be run as a reflex
+by developers (and by bots) from the top level Makefile for every changing
+source file. In the real world however there's a number of challenges:
+
+* Sometimes there are valid reasons for accepting violations of a checker
+ rule, even if that rule is a sensible one in the general case.
+* Some subsystems have different restrictions and requirements.
+ (Ideally, the number of subsystems with differing restrictions and
+ requirements will diminish over time.)
+* Similarly, the kernel contains a lot of code that predates the tools, or at
+ least some of the newer rules, and we would like these tools to evolve without
+ requiring the need to fix all issues detected with it in the same commit.
+ We also want to accommodate new tools, so that each new tool does not
+ have to reinvent it's own mechanism for running checks.
+* On the other hand, we want to make sure that files that are clean
+ (to some well defined extent, such as passing checkpatch or sparse
+ with checks only for certain important types of issues) keep being so.
+
+This is the purpose of ``scripts/runchecks``.
+
+The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
+``scripts`` directory, then in the directory hierarchy of the source file,
+starting where that source file is located, searching upwards. If at least
+one such file exists in the source tree, ``runchecks`` parses a set of
+rules from it, and use them to determine how to invoke a set of individual
+checker tools for a particular file. The kernel Makefile system supports using
+this feature as an integrated part of compiling the code, using the
+``C={1,2}`` option. With::
+
+ make C=1
+
+runchecks will be invoked if the file needs to be recompiled. With ::
+
+ make C=2
+
+runchecks will be invoked for all source files, even if they do not need
+recompiling. Based on the configuration, ``runchecks`` will invoke one or
+more checkers. The number and types of checkers to run are configurable and
+can also be selected on the command line::
+
+ make C=2 CF="--run:sparse,checkpatch"
+
+If only one checker is run, any parameter that is not recognized by
+runchecks itself will be forwarded to the checker. If more than one checker
+is enabled, parameters can be forwarded to a specific checker by means of
+this syntax::
+
+ make C=2 CF="--to-checkpatch:--terse"
+
+A comma separated list of parameters can be supplied if necessary.
+
+Supported syntax of the runchecks.cfg configuration file
+--------------------------------------------------------
+
+The runchecks configuration file chain can be used to set policies and "rein in"
+checker errors piece by piece for a particular subsystem or driver. It can
+also be used to mitigate and extend checkers that does not support
+selective suppression of all it's checks.
+
+Two classes of configuration is available. The first class is configuration
+that defines what checkers are enabled, and also allow a limited form of
+pattern matching to extend checking, to mitigate for checks that cannot be
+selectively disabled by command line parameters to the underlying tool
+itself. This type of configuration should go into the first accessed
+configuration file, and has been preconfigured for the currently supported
+checkers in ``scripts/runchecks.cfg``. The second class is the features for
+configuring the output of the checkers by selectively suppressing checks on
+a per file or per check basis. These typically goes in the source tree in
+the directory of the source file or above. Some of the syntax is generic
+and some is only supported by some checkers.
+
+For the first class of configuration the following syntax is supported::
+
+ # comments
+ checker checkpatch [command]
+ addflags <list of extra flags and parameters>
+ cflags
+ typedef NAME <regular expression>
+ run [checker list|all]
+
+The ``checker`` command switches ``runchecks``'s attention to a particular
+checker. The following commands until the next ``checker`` statement
+applies to that particular checker. The first occurrence of ``checker``
+also serves as a potentially defining operation, if the checker name
+has not been preconfigured. In that case, a second parameter can be used
+to provide the name of the command used to run the checker.
+A full checker integration into runchecks will typically require some
+additions to runchecks, and will then have been preconfigured,
+but simple checkers might just be configured on the fly.
+
+The ``addflags`` command incrementally adds more flags and parameters to
+the command line used to invoke the checker. This applies to all
+invocations of the checker from runchecks.
+The ``cflags`` command tells to forward all the flags and options passed to
+the compiler invocation to the checker. The default is to suppress these
+parameters when invoking the checker.
+
+The ``typedef`` command adds ``NAME`` and associates it with the given
+regular expression. This expression is used to match against standard error
+output from the checker and ``NAME`` can be used as a new named check that
+runchecks understands and that can be used with checker supported names
+below to selectively suppress that particular set of warning or error
+messages. This is useful to handle output checks for which the underlying
+checker is does not provide any suppression. Check type namespaces are
+separate for the individual checkers. You can list the state of the built in and
+configured checker and check types with::
+
+ scripts/runchecks --list
+
+The checker implementations of the ``typedef`` command also allows
+runchecks to perform some unification of output by rewriting the output
+lines, adding optional color support, and use of the new type names in the
+error output, to ease the process of updating the runchecks.cfg files.
+Having a unified representation of the error output also makes it much
+easier to do statistics or other operations on top of an aggregated output
+from several checkers.
+
+For the second class of configuration the following syntax is supported::
+
+ # comments
+ checker checker_name
+ line_len <n>
+ except check_type [files ...]
+ pervasive check_type1 [check_type2 ...]
+
+The ``line_len`` directive defines the upper bound of characters per line
+tolerated in this directory. Currently only ``checkpatch`` supports this
+command. The ``except`` directive takes a check type such as for example
+``MACRO_ARG_REUSE``, and a set of files that should not be subject to this
+particular check type. The ``pervasive`` command disables the listed types
+of checks for all the files in the subtree. The ``except`` and
+``pervasive`` directives can be used cumulatively to add more exceptions.
+
+Running checker programs from make
+----------------------------------
+
+You can run checkpatch subject to rules defined in ``runcheck.cfg`` in the
+directory of the source file by using "make P=1" to run checkpatch on all files
+that gets recompiled, or "make P=2" to run checkpatch on all source files.
+
+A make variable ``CF`` allows passing additional parameters to
+``runchecks``. You can for instance use::
+
+ make C=2 CF="--run:checkpatch --fix-inplace"
+
+to run only the ``checkpatch`` checker, and to have checkpatch trying to fix
+issues it finds - *make sure you have a clean git tree and carefully review
+the output afterwards!* Combine this with selectively enabling of types of
+errors via changes under ``checker checkpatch`` to the local
+``runchecks.cfg``, and you can focus on fixing up errors subsystem or
+driver by driver on a type by type basis.
+
+By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
+be found in the directory of the file or in the tree above. This is to
+allow builds with ``C=2`` to pass even for subsystems that has not yet done
+anything to rein in checker errors. At some point when all subsystems and
+drivers either have fixed all checker errors or added proper
+``runchecks.cfg`` files, this can be changed.
+
+To force runchecks to run a full run in directories/trees where runchecks
+does not find a ``runchecks.cfg`` file as well, use::
+
+ make C=2 CF="-f"
+
+If you like to see all the warnings and errors produced by the checkers, ignoring
+any runchecks.cfg files except the one under ``scripts``, you can use::
+
+ make C=2 CF="-n"
+
+or for a specific module directory::
+
+ make C=2 M=drivers/infiniband/core CF="--color -n -w"
+
+with the -w option to ``runchecks`` to suppress errors from any of the
+checkers and just continue on, and the ``--color`` option to present errors
+with colors where supported.
+
+Ever tightening checker rules
+-----------------------------
+
+Commit the changes to the relevant ``runchecks.cfg`` together with the code
+changes that fixes a particular type of issue, this will allow automatic
+checker running by default. This way we can ensure that new errors of that
+particular type do not inadvertently sneak in again! This can be done at
+any subsystem or module maintainer's discretion and at the right time
+without having to do it all at the same time.
+
+Before submitting your changes, verify that a full "make C=2" passes with no
+errors.
+
+Extending and improving checker support in ``runchecks``
+--------------------------------------------------------
+
+The runchecks program has been written with extensibility in mind.
+If the checker starts it's reporting lines with filename:lineno, there's a
+good chance that a new checker can simply be added by adding::
+
+ checker mychecker path_to_mychecker
+
+to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
+selective suppressions of output, however it is likely that some quirks are
+needed to make the new checker behave similar to the others, and to support
+the full set of features, such as the ``--list`` option. This is done by
+implementing a new subclass of the Checker class in ``runchecks``. This is the way
+all the available default supported checkers are implemented, and those
+relatively lean implementations could serve as examples.
diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
index 78aa00a..e3e8b27 100644
--- a/Documentation/dev-tools/sparse.rst
+++ b/Documentation/dev-tools/sparse.rst
@@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether they need to
be recompiled or not. The latter is a fast way to check the whole tree if you
have already built it.

-The optional make variable CF can be used to pass arguments to sparse. The
-build system passes -Wbitwise to sparse automatically.
+The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
+which dependent on configuration and command line options may dispatch calls to
+other checkers in addition to sparse. Details on how this works is covered
+in Documentation/dev-tools/runchecks.rst .
+
+The optional make variable CF can be used to pass arguments to runchecks for dispatch
+to sparse. If sparse is the only tool enabled, any option not recognized by
+runchecks will be forwarded to sparse. If more than one tool is active, you must
+add the parameters you want sparse to get as a comma separated list prefixed by
+``--to-sparse:``. If you want sparse to be the only checker run, and you want
+some nice colored output, you can specify this as::
+
+ make C=2 CF="--run:sparse --color"
+
+This will cause sparse to be called for all files which are supported by a valid
+runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
+details). If you want to run sparse on all files and ignore any missing
+configuration files(s), just add ``-n`` to the list of options passed to
+runchecks. This will cause runchecks to call sparse with all errors enabled for
+all files even if no valid configuration is found in the tree for the source files.
+
+By default "runchecks" is set to enable all sparse errors, but you can
+configure what checks to be applied by sparse on a per file or per subsystem
+basis. With the above invocation, make will fail and stop on the first file
+encountered with sparse errors or warnings in it. If you want to continue
+anyway, you can use::
+
+ make C=2 CF="--run:sparse --color -w"
diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
index ac2363e..260e688 100644
--- a/Documentation/kbuild/kbuild.txt
+++ b/Documentation/kbuild/kbuild.txt
@@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.

CF
--------------------------------------------------
-Additional options for sparse.
-CF is often used on the command-line like this:
+Additional options for runchecks, the generic checker runner.
+CF is often used on the command-line for instance like this:

- make CF=-Wbitwise C=2
+ make C=2 CF="--run:sparse --color -w"
+
+to run the sparse tool only, and to use colored output and continue on warnings
+or errors.

INSTALL_PATH
--------------------------------------------------

[Julia Lawall]

Post by Knut Omang
diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-tools/runchecks.rst
new file mode 100644
index 0000000..b25b3de
--- /dev/null
+++ b/Documentation/dev-tools/runchecks.rst
@@ -0,0 +1,215 @@
+
+Makefile support for running checkers
+=====================================
+
+Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a

is -> are

Post by Knut Omang
+lot of syntactic and semantic issues with the code, and is also constantly

is -> are

Post by Knut Omang
+evolving and detecting more. In an ideal world, all source files should
+adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
+bells and whistles enabled, in a way that these checkers can be run as a reflex
+by developers (and by bots) from the top level Makefile for every changing
+
+* Sometimes there are valid reasons for accepting violations of a checker
+ rule, even if that rule is a sensible one in the general case.
+* Some subsystems have different restrictions and requirements.
+ (Ideally, the number of subsystems with differing restrictions and
+ requirements will diminish over time.)
+* Similarly, the kernel contains a lot of code that predates the tools, or at
+ least some of the newer rules, and we would like these tools to evolve without
+ requiring the need to fix all issues detected with it in the same commit.
+ We also want to accommodate new tools, so that each new tool does not
+ have to reinvent it's own mechanism for running checks.

it's -> its

Post by Knut Omang
+* On the other hand, we want to make sure that files that are clean
+ (to some well defined extent, such as passing checkpatch or sparse
+ with checks only for certain important types of issues) keep being so.
+
+This is the purpose of ``scripts/runchecks``.
+
+The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
+``scripts`` directory, then in the directory hierarchy of the source file,
+starting where that source file is located, searching upwards. If at least
+one such file exists in the source tree, ``runchecks`` parses a set of
+rules from it, and use them to determine how to invoke a set of individual
+checker tools for a particular file. The kernel Makefile system supports using
+this feature as an integrated part of compiling the code, using the
+
+ make C=1
+
+
+ make C=2
+
+runchecks will be invoked for all source files, even if they do not need
+recompiling. Based on the configuration, ``runchecks`` will invoke one or
+more checkers. The number and types of checkers to run are configurable and
+
+ make C=2 CF="--run:sparse,checkpatch"
+
+If only one checker is run, any parameter that is not recognized by
+runchecks itself will be forwarded to the checker. If more than one checker
+is enabled, parameters can be forwarded to a specific checker by means of
+
+ make C=2 CF="--to-checkpatch:--terse"
+
+A comma separated list of parameters can be supplied if necessary.
+
+Supported syntax of the runchecks.cfg configuration file
+--------------------------------------------------------
+
+The runchecks configuration file chain can be used to set policies and "rein in"
+checker errors piece by piece for a particular subsystem or driver. It can
+also be used to mitigate and extend checkers that does not support

does -> do

Post by Knut Omang
+selective suppression of all it's checks.

it's -> its

Post by Knut Omang
+Two classes of configuration is available. The first class is configuration
+that defines what checkers are enabled, and also allow a limited form of
+pattern matching to extend checking, to mitigate for checks that cannot be
+selectively disabled by command line parameters to the underlying tool
+itself. This type of configuration should go into the first accessed
+configuration file, and has been preconfigured for the currently supported
+checkers in ``scripts/runchecks.cfg``. The second class is the features for
+configuring the output of the checkers by selectively suppressing checks on
+a per file or per check basis. These typically goes in the source tree in

goes -> go

Post by Knut Omang
+the directory of the source file or above. Some of the syntax is generic
+and some is only supported by some checkers.
+
+
+ # comments
+ checker checkpatch [command]
+ addflags <list of extra flags and parameters>
+ cflags
+ typedef NAME <regular expression>
+ run [checker list|all]
+
+The ``checker`` command switches ``runchecks``'s attention to a particular
+checker. The following commands until the next ``checker`` statement
+applies to that particular checker. The first occurrence of ``checker``

applies -> apply

Post by Knut Omang
+also serves as a potentially defining operation, if the checker name
+has not been preconfigured. In that case, a second parameter can be used
+to provide the name of the command used to run the checker.
+A full checker integration into runchecks will typically require some
+additions to runchecks, and will then have been preconfigured,
+but simple checkers might just be configured on the fly.
+
+The ``addflags`` command incrementally adds more flags and parameters to
+the command line used to invoke the checker. This applies to all
+invocations of the checker from runchecks.
+The ``cflags`` command tells to forward all the flags and options passed to
+the compiler invocation to the checker. The default is to suppress these
+parameters when invoking the checker.
+
+The ``typedef`` command adds ``NAME`` and associates it with the given
+regular expression. This expression is used to match against standard error
+output from the checker and ``NAME`` can be used as a new named check that
+runchecks understands and that can be used with checker supported names
+below to selectively suppress that particular set of warning or error
+messages. This is useful to handle output checks for which the underlying
+checker is does not provide any suppression. Check type namespaces are
+separate for the individual checkers. You can list the state of the built in and
+
+ scripts/runchecks --list
+
+The checker implementations of the ``typedef`` command also allows
+runchecks to perform some unification of output by rewriting the output
+lines, adding optional color support, and use of the new type names in the
+error output, to ease the process of updating the runchecks.cfg files.
+Having a unified representation of the error output also makes it much
+easier to do statistics or other operations on top of an aggregated output
+from several checkers.
+
+
+ # comments
+ checker checker_name
+ line_len <n>
+ except check_type [files ...]
+ pervasive check_type1 [check_type2 ...]
+
+The ``line_len`` directive defines the upper bound of characters per line
+tolerated in this directory. Currently only ``checkpatch`` supports this
+command. The ``except`` directive takes a check type such as for example
+``MACRO_ARG_REUSE``, and a set of files that should not be subject to this
+particular check type. The ``pervasive`` command disables the listed types
+of checks for all the files in the subtree. The ``except`` and
+``pervasive`` directives can be used cumulatively to add more exceptions.
+
+Running checker programs from make
+----------------------------------
+
+You can run checkpatch subject to rules defined in ``runcheck.cfg`` in the
+directory of the source file by using "make P=1" to run checkpatch on all files
+that gets recompiled, or "make P=2" to run checkpatch on all source files.
+
+A make variable ``CF`` allows passing additional parameters to
+
+ make C=2 CF="--run:checkpatch --fix-inplace"
+
+to run only the ``checkpatch`` checker, and to have checkpatch trying to fix

trying -> try

Post by Knut Omang
+issues it finds - *make sure you have a clean git tree and carefully review
+the output afterwards!* Combine this with selectively enabling of types of
+errors via changes under ``checker checkpatch`` to the local
+``runchecks.cfg``, and you can focus on fixing up errors subsystem or
+driver by driver on a type by type basis.
+
+By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
+be found in the directory of the file or in the tree above. This is to
+allow builds with ``C=2`` to pass even for subsystems that has not yet done
+anything to rein in checker errors. At some point when all subsystems and
+drivers either have fixed all checker errors or added proper
+``runchecks.cfg`` files, this can be changed.
+
+To force runchecks to run a full run in directories/trees where runchecks
+
+ make C=2 CF="-f"
+
+If you like to see all the warnings and errors produced by the checkers, ignoring
+
+ make C=2 CF="-n"
+
+
+ make C=2 M=drivers/infiniband/core CF="--color -n -w"
+
+with the -w option to ``runchecks`` to suppress errors from any of the
+checkers and just continue on, and the ``--color`` option to present errors
+with colors where supported.
+
+Ever tightening checker rules
+-----------------------------
+
+Commit the changes to the relevant ``runchecks.cfg`` together with the code
+changes that fixes a particular type of issue, this will allow automatic
+checker running by default. This way we can ensure that new errors of that
+particular type do not inadvertently sneak in again! This can be done at
+any subsystem or module maintainer's discretion and at the right time
+without having to do it all at the same time.
+
+Before submitting your changes, verify that a full "make C=2" passes with no
+errors.
+
+Extending and improving checker support in ``runchecks``
+--------------------------------------------------------
+
+The runchecks program has been written with extensibility in mind.
+If the checker starts it's reporting lines with filename:lineno, there's a

it's -> its

Post by Knut Omang
+
+ checker mychecker path_to_mychecker
+
+to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
+selective suppressions of output, however it is likely that some quirks are
+needed to make the new checker behave similar to the others, and to support

similar -> similarly

julia

Post by Knut Omang
+the full set of features, such as the ``--list`` option. This is done by
+implementing a new subclass of the Checker class in ``runchecks``. This is the way
+all the available default supported checkers are implemented, and those
+relatively lean implementations could serve as examples.
diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
index 78aa00a..e3e8b27 100644
--- a/Documentation/dev-tools/sparse.rst
+++ b/Documentation/dev-tools/sparse.rst
@@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether they need to
be recompiled or not. The latter is a fast way to check the whole tree if you
have already built it.
-The optional make variable CF can be used to pass arguments to sparse. The
-build system passes -Wbitwise to sparse automatically.
+The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
+which dependent on configuration and command line options may dispatch calls to
+other checkers in addition to sparse. Details on how this works is covered
+in Documentation/dev-tools/runchecks.rst .
+
+The optional make variable CF can be used to pass arguments to runchecks for dispatch
+to sparse. If sparse is the only tool enabled, any option not recognized by
+runchecks will be forwarded to sparse. If more than one tool is active, you must
+add the parameters you want sparse to get as a comma separated list prefixed by
+``--to-sparse:``. If you want sparse to be the only checker run, and you want
+
+ make C=2 CF="--run:sparse --color"
+
+This will cause sparse to be called for all files which are supported by a valid
+runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
+details). If you want to run sparse on all files and ignore any missing
+configuration files(s), just add ``-n`` to the list of options passed to
+runchecks. This will cause runchecks to call sparse with all errors enabled for
+all files even if no valid configuration is found in the tree for the source files.
+
+By default "runchecks" is set to enable all sparse errors, but you can
+configure what checks to be applied by sparse on a per file or per subsystem
+basis. With the above invocation, make will fail and stop on the first file
+encountered with sparse errors or warnings in it. If you want to continue
+
+ make C=2 CF="--run:sparse --color -w"
diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
index ac2363e..260e688 100644
--- a/Documentation/kbuild/kbuild.txt
+++ b/Documentation/kbuild/kbuild.txt
@@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
CF
--------------------------------------------------
-Additional options for sparse.
+Additional options for runchecks, the generic checker runner.
- make CF=-Wbitwise C=2
+ make C=2 CF="--run:sparse --color -w"
+
+to run the sparse tool only, and to use colored output and continue on warnings
+or errors.
INSTALL_PATH
--------------------------------------------------
--
git-series 0.9.1

[Knut Omang]

Post by Knut Omang

Post by Knut Omang
diff --git a/Documentation/dev-tools/runchecks.rst b/Documentation/dev-

tools/runchecks.rst

Post by Knut Omang
new file mode 100644
index 0000000..b25b3de
--- /dev/null
+++ b/Documentation/dev-tools/runchecks.rst
@@ -0,0 +1,215 @@
+
+Makefile support for running checkers
+=====================================
+
+Tools like sparse, coccinelle and scripts/checkpatch.pl is able to detect a

is -> are

Post by Knut Omang
+lot of syntactic and semantic issues with the code, and is also constantly

is -> are

Post by Knut Omang
+evolving and detecting more. In an ideal world, all source files should
+adhere to whatever rules imposed by checkpatch.pl and sparse etc. with all
+bells and whistles enabled, in a way that these checkers can be run as a reflex
+by developers (and by bots) from the top level Makefile for every changing
+
+* Sometimes there are valid reasons for accepting violations of a checker
+ rule, even if that rule is a sensible one in the general case.
+* Some subsystems have different restrictions and requirements.
+ (Ideally, the number of subsystems with differing restrictions and
+ requirements will diminish over time.)
+* Similarly, the kernel contains a lot of code that predates the tools, or at
+ least some of the newer rules, and we would like these tools to evolve without
+ requiring the need to fix all issues detected with it in the same commit.
+ We also want to accommodate new tools, so that each new tool does not
+ have to reinvent it's own mechanism for running checks.

it's -> its

Post by Knut Omang
+* On the other hand, we want to make sure that files that are clean
+ (to some well defined extent, such as passing checkpatch or sparse
+ with checks only for certain important types of issues) keep being so.
+
+This is the purpose of ``scripts/runchecks``.
+
+The ``runchecks`` program looks for files named ``runchecks.cfg`` in the
+``scripts`` directory, then in the directory hierarchy of the source file,
+starting where that source file is located, searching upwards. If at least
+one such file exists in the source tree, ``runchecks`` parses a set of
+rules from it, and use them to determine how to invoke a set of individual
+checker tools for a particular file. The kernel Makefile system supports using
+this feature as an integrated part of compiling the code, using the
+
+ make C=1
+
+
+ make C=2
+
+runchecks will be invoked for all source files, even if they do not need
+recompiling. Based on the configuration, ``runchecks`` will invoke one or
+more checkers. The number and types of checkers to run are configurable and
+
+ make C=2 CF="--run:sparse,checkpatch"
+
+If only one checker is run, any parameter that is not recognized by
+runchecks itself will be forwarded to the checker. If more than one checker
+is enabled, parameters can be forwarded to a specific checker by means of
+
+ make C=2 CF="--to-checkpatch:--terse"
+
+A comma separated list of parameters can be supplied if necessary.
+
+Supported syntax of the runchecks.cfg configuration file
+--------------------------------------------------------
+
+The runchecks configuration file chain can be used to set policies and "rein in"
+checker errors piece by piece for a particular subsystem or driver. It can
+also be used to mitigate and extend checkers that does not support

does -> do

Post by Knut Omang
+selective suppression of all it's checks.

it's -> its

Post by Knut Omang
+Two classes of configuration is available. The first class is configuration
+that defines what checkers are enabled, and also allow a limited form of
+pattern matching to extend checking, to mitigate for checks that cannot be
+selectively disabled by command line parameters to the underlying tool
+itself. This type of configuration should go into the first accessed
+configuration file, and has been preconfigured for the currently supported
+checkers in ``scripts/runchecks.cfg``. The second class is the features for
+configuring the output of the checkers by selectively suppressing checks on
+a per file or per check basis. These typically goes in the source tree in

goes -> go

Post by Knut Omang
+the directory of the source file or above. Some of the syntax is generic
+and some is only supported by some checkers.
+
+
+ # comments
+ checker checkpatch [command]
+ addflags <list of extra flags and parameters>
+ cflags
+ typedef NAME <regular expression>
+ run [checker list|all]
+
+The ``checker`` command switches ``runchecks``'s attention to a particular
+checker. The following commands until the next ``checker`` statement
+applies to that particular checker. The first occurrence of ``checker``

applies -> apply

Post by Knut Omang
+also serves as a potentially defining operation, if the checker name
+has not been preconfigured. In that case, a second parameter can be used
+to provide the name of the command used to run the checker.
+A full checker integration into runchecks will typically require some
+additions to runchecks, and will then have been preconfigured,
+but simple checkers might just be configured on the fly.
+
+The ``addflags`` command incrementally adds more flags and parameters to
+the command line used to invoke the checker. This applies to all
+invocations of the checker from runchecks.
+The ``cflags`` command tells to forward all the flags and options passed to
+the compiler invocation to the checker. The default is to suppress these
+parameters when invoking the checker.
+
+The ``typedef`` command adds ``NAME`` and associates it with the given
+regular expression. This expression is used to match against standard error
+output from the checker and ``NAME`` can be used as a new named check that
+runchecks understands and that can be used with checker supported names
+below to selectively suppress that particular set of warning or error
+messages. This is useful to handle output checks for which the underlying
+checker is does not provide any suppression. Check type namespaces are
+separate for the individual checkers. You can list the state of the built in and
+
+ scripts/runchecks --list
+
+The checker implementations of the ``typedef`` command also allows
+runchecks to perform some unification of output by rewriting the output
+lines, adding optional color support, and use of the new type names in the
+error output, to ease the process of updating the runchecks.cfg files.
+Having a unified representation of the error output also makes it much
+easier to do statistics or other operations on top of an aggregated output
+from several checkers.
+
+
+ # comments
+ checker checker_name
+ line_len <n>
+ except check_type [files ...]
+ pervasive check_type1 [check_type2 ...]
+
+The ``line_len`` directive defines the upper bound of characters per line
+tolerated in this directory. Currently only ``checkpatch`` supports this
+command. The ``except`` directive takes a check type such as for example
+``MACRO_ARG_REUSE``, and a set of files that should not be subject to this
+particular check type. The ``pervasive`` command disables the listed types
+of checks for all the files in the subtree. The ``except`` and
+``pervasive`` directives can be used cumulatively to add more exceptions.
+
+Running checker programs from make
+----------------------------------
+
+You can run checkpatch subject to rules defined in ``runcheck.cfg`` in the
+directory of the source file by using "make P=1" to run checkpatch on all files
+that gets recompiled, or "make P=2" to run checkpatch on all source files.
+
+A make variable ``CF`` allows passing additional parameters to
+
+ make C=2 CF="--run:checkpatch --fix-inplace"
+
+to run only the ``checkpatch`` checker, and to have checkpatch trying to fix

trying -> try

Post by Knut Omang
+issues it finds - *make sure you have a clean git tree and carefully review
+the output afterwards!* Combine this with selectively enabling of types of
+errors via changes under ``checker checkpatch`` to the local
+``runchecks.cfg``, and you can focus on fixing up errors subsystem or
+driver by driver on a type by type basis.
+
+By default runchecks will skip all files if a ``runchecks.cfg`` file cannot
+be found in the directory of the file or in the tree above. This is to
+allow builds with ``C=2`` to pass even for subsystems that has not yet done
+anything to rein in checker errors. At some point when all subsystems and
+drivers either have fixed all checker errors or added proper
+``runchecks.cfg`` files, this can be changed.
+
+To force runchecks to run a full run in directories/trees where runchecks
+
+ make C=2 CF="-f"
+
+If you like to see all the warnings and errors produced by the checkers, ignoring
+
+ make C=2 CF="-n"
+
+
+ make C=2 M=drivers/infiniband/core CF="--color -n -w"
+
+with the -w option to ``runchecks`` to suppress errors from any of the
+checkers and just continue on, and the ``--color`` option to present errors
+with colors where supported.
+
+Ever tightening checker rules
+-----------------------------
+
+Commit the changes to the relevant ``runchecks.cfg`` together with the code
+changes that fixes a particular type of issue, this will allow automatic
+checker running by default. This way we can ensure that new errors of that
+particular type do not inadvertently sneak in again! This can be done at
+any subsystem or module maintainer's discretion and at the right time
+without having to do it all at the same time.
+
+Before submitting your changes, verify that a full "make C=2" passes with no
+errors.
+
+Extending and improving checker support in ``runchecks``
+--------------------------------------------------------
+
+The runchecks program has been written with extensibility in mind.
+If the checker starts it's reporting lines with filename:lineno, there's a

it's -> its

Post by Knut Omang
+
+ checker mychecker path_to_mychecker
+
+to ``scripts/runchecks.cfg`` and suitable ``typedef`` expressions to provide
+selective suppressions of output, however it is likely that some quirks are
+needed to make the new checker behave similar to the others, and to support

similar -> similarly
julia

Thanks for the review - I'll certainly fix these for v3.

Knut

Post by Knut Omang

Post by Knut Omang
+the full set of features, such as the ``--list`` option. This is done by
+implementing a new subclass of the Checker class in ``runchecks``. This is the way
+all the available default supported checkers are implemented, and those
+relatively lean implementations could serve as examples.
diff --git a/Documentation/dev-tools/sparse.rst b/Documentation/dev-tools/sparse.rst
index 78aa00a..e3e8b27 100644
--- a/Documentation/dev-tools/sparse.rst
+++ b/Documentation/dev-tools/sparse.rst
@@ -101,5 +101,31 @@ recompiled, or use "make C=2" to run sparse on the files whether

they need to

Post by Knut Omang
be recompiled or not. The latter is a fast way to check the whole tree if you
have already built it.
-The optional make variable CF can be used to pass arguments to sparse. The
-build system passes -Wbitwise to sparse automatically.
+The "make C={1,2}" form of kernel make indirectly calls sparse via "runchecks",
+which dependent on configuration and command line options may dispatch calls to
+other checkers in addition to sparse. Details on how this works is covered
+in Documentation/dev-tools/runchecks.rst .
+
+The optional make variable CF can be used to pass arguments to runchecks for dispatch
+to sparse. If sparse is the only tool enabled, any option not recognized by
+runchecks will be forwarded to sparse. If more than one tool is active, you must
+add the parameters you want sparse to get as a comma separated list prefixed by
+``--to-sparse:``. If you want sparse to be the only checker run, and you want
+
+ make C=2 CF="--run:sparse --color"
+
+This will cause sparse to be called for all files which are supported by a valid
+runchecks configuration (again see Documentation/dev-tools/runchecks.rst for
+details). If you want to run sparse on all files and ignore any missing
+configuration files(s), just add ``-n`` to the list of options passed to
+runchecks. This will cause runchecks to call sparse with all errors enabled for
+all files even if no valid configuration is found in the tree for the source files.
+
+By default "runchecks" is set to enable all sparse errors, but you can
+configure what checks to be applied by sparse on a per file or per subsystem
+basis. With the above invocation, make will fail and stop on the first file
+encountered with sparse errors or warnings in it. If you want to continue
+
+ make C=2 CF="--run:sparse --color -w"
diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
index ac2363e..260e688 100644
--- a/Documentation/kbuild/kbuild.txt
+++ b/Documentation/kbuild/kbuild.txt
@@ -103,10 +103,13 @@ CROSS_COMPILE is also used for ccache in some setups.
CF
--------------------------------------------------
-Additional options for sparse.
+Additional options for runchecks, the generic checker runner.
- make CF=-Wbitwise C=2
+ make C=2 CF="--run:sparse --color -w"
+
+to run the sparse tool only, and to use colored output and continue on warnings
+or errors.
INSTALL_PATH
--------------------------------------------------
--
git-series 0.9.1

[Knut Omang]

Dear Knut,
Does this document need any more syntax checking and further review besides
the places which were indicated by Julia Lawall?

Post by Knut Omang
+Two classes of configuration is available.

+Two classes … are available. …

Post by Knut Omang
+checker is does not provide any suppression. Check type namespaces are

+checker does not …

Post by Knut Omang
+The checker implementations of the ``typedef`` command also allows

+The checker implementations … allow

Post by Knut Omang
+allow builds with ``C=2`` to pass even for subsystems that has not yet done

… subsystems that have not …
Regards,
Markus

I hope not,..
Believe it or not, I do know the grammar rules although it might not
look that way here :) Always good to get a native english
speaker to review though.

I'll take these in for v3 as well and carefully review myself
for any further errors of the same sorts..

Thanks,
Knut

[Julia Lawall]

Post by Knut Omang

Dear Knut,
Does this document need any more syntax checking and further review besides
the places which were indicated by Julia Lawall?

Post by Knut Omang
+Two classes of configuration is available.

+Two classes 
 are available. 


Post by Knut Omang
+checker is does not provide any suppression. Check type namespaces are

+checker does not 


Post by Knut Omang
+The checker implementations of the ``typedef`` command also allows

+The checker implementations 
 allow

Post by Knut Omang
+allow builds with ``C=2`` to pass even for subsystems that has not yet done


 subsystems that have not 

Regards,
Markus

I hope not,..
Believe it or not, I do know the grammar rules although it might not
look that way here :) Always good to get a native english
speaker to review though.
I'll take these in for v3 as well and carefully review myself
for any further errors of the same sorts..

It's a lot of text. Anyone can make mistakes :) Thanks for the work.

julia

Post by Knut Omang
Thanks,
Knut
_______________________________________________
Cocci mailing list
https://systeme.lip6.fr/mailman/listinfo/cocci

[Knut Omang]

Post by Knut Omang
I'll take these in for v3 as well

Thanks.

Post by Knut Omang
and carefully review myself for any further errors of the same sorts..

I found it surprising that so many update candidates were left over
still in the second version of this text (while tags were already present
from three reviewers).
I assume that other wording adjustments could eventually be considered
at a few places.

I am afraid most of it came due to the changes I made to go from supporting just
checkpatch to support many checkers from v1 to v2, so I have to take the sole
responsibility...

Thanks,
Knut

Regards,
Markus

[Knut Omang]

Post by Knut Omang

I assume that other wording adjustments could eventually be considered
at a few places.

I am afraid most of it came due to the changes I made to go from supporting just
checkpatch to support many checkers from v1 to v2, so I have to take the sole
responsibility...

Thanks for your clarification.
You showed a promising start for another software evolution.
My own understanding level might be also still incomplete in this case.
Do you plan any extensions for higher level descriptions?

I am itching to implement more features :-)
I already have some ideas and I am happy to take more, I just want the basis to get in
first, so I am sure I don't waste my time..

Thanks,
Knut

Regards,
Markus

[Knut Omang]

Post by Knut Omang
I'll take these in for v3 as well and carefully review myself
for any further errors of the same sorts..

I suggest to consider a few additional wording adjustments.

Will fix.

* Do you care for the handling of orphaned words in paragraphs?

Will look over it, thanks.

* Was any consensus achieved already for corresponding names?
x Could a longer variable name like “CHECKING_FLAGS” be easier to remember
than the proposed “CF”?

I kept CF as is already established as the flag to use for parameters with C=

x Will the naming style matter (if you compare the approaches “checkpatch”
and “get_maintainer”)?

No consensus made on the naming, just that I tried using verbatim names from sparse, eg
lowercase-dash vs UPPERCASE_UNDERSCORE as in checkpatch and found the runchecks.cfg and
reporting lines more easy to read using the uppercase variants, and that it looks more
nice to have a uniform output across checkers.

I think that the visual output of a test is important, as even we developers tend to be
more happy with (and willing to execute) programs that produce nice output vs ones with
ugly, cluttery, inconsistent output :-)

Thanks,
Knut

Regards,
Markus

[Knut Omang]

Hi Marcus,

Thanks for all the useful enhancement feedback, I plan to look through these
in detail when I prepare for v3 - right now family has to get priority.

My initial thoughts from a quick glance is that these are good points that will improve
the docs and functionality, I also have a long list of ideas myself, but I am a bit
reluctant to put significantly more work into this until I have some guarantee that any of
it will be accepted. Maybe we can save some of it for an update patch later?

Thanks,
Knut

Post by Knut Omang
+The ``typedef`` command adds ``NAME`` and associates it with the given
+regular expression. This expression is used to match against standard error
+output from the checker and ``NAME`` can be used as a new named check that
+runchecks understands and that can be used with checker supported names
+below to selectively suppress that particular set of warning or error
+messages.

I would like to clarify further software development concerns.
https://github.com/torvalds/linux/compare/master...knuto:runchecks#commit_comments_bucke
t
1. Would an other identifier be more appropriate for this use case?
2. Which flavour of regular expressions would you like to use here?
3. Is the “named check” a data filter?
4. How do you think about to share any more experiences from previous
applications of similar approaches?
5. I imagine that corresponding regexes can become complicated.
Would you like to develop a pattern library then?
6. It can happen that messages which are provided by special programs are
so complex that scripts for higher level programming languages would
be applied.
7. How would you like to iterate through several messages from an error log?
8. Will an include statement be needed so that configuration scripts can be
better combined?
9. Does the suggested script language need a structured format description
as extended Backus-Naur form (ISO/IEC 14977)?
Regards,
Markus

[Knut Omang]

Post by Knut Omang
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

[Knut Omang]

Post by Knut Omang

Post by Knut Omang
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.

Post by Knut Omang
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

[Stephen Hemminger]

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

Post by Knut Omang
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).
# comments
checker <name>
typedef NAME regex
run <list of checkers or "all"
line_len <n>
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.
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.
-----------------
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
too, to make fixing issues in the code, the goal of this whole exercise,
much more pleasant IMHO :-)
Thanks,
Knut
runchecks: Generalize make C={1,2} to support multiple checkers
Documentation: Add doc for runchecks, a checker runner
checkpatch: Improve --fix-inplace for TABSTOP
rds: Add runchecks.cfg for net/rds
RDMA/core: Add runchecks.cfg for drivers/infiniband/core
Documentation/dev-tools/coccinelle.rst | 12 +-
Documentation/dev-tools/index.rst | 1 +-
Documentation/dev-tools/runchecks.rst | 215 ++++++++-
Documentation/dev-tools/sparse.rst | 30 +-
Documentation/kbuild/kbuild.txt | 9 +-
Makefile | 23 +-
drivers/infiniband/core/runchecks.cfg | 83 +++-
net/rds/runchecks.cfg | 76 +++-
scripts/Makefile.build | 4 +-
scripts/checkpatch.pl | 2 +-
scripts/runchecks | 734 ++++++++++++++++++++++++++-
scripts/runchecks.cfg | 63 ++-
scripts/runchecks_help.txt | 43 ++-
13 files changed, 1274 insertions(+), 21 deletions(-)
create mode 100644 Documentation/dev-tools/runchecks.rst
create mode 100644 drivers/infiniband/core/runchecks.cfg
create mode 100644 net/rds/runchecks.cfg
create mode 100755 scripts/runchecks
create mode 100644 scripts/runchecks.cfg
create mode 100644 scripts/runchecks_help.txt
base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215

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.

[Joe Perches]

Post by Stephen Hemminger
On Sat, 16 Dec 2017 15:42:25 +0100

Post by Knut Omang
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.

[]

Post by Stephen Hemminger
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

[Knut Omang]

Post by Stephen Hemminger
On Sat, 16 Dec 2017 15:42:25 +0100

Post by Knut Omang
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).
# comments
checker <name>
typedef NAME regex
run <list of checkers or "all"
line_len <n>
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.
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.
-----------------
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
too, to make fixing issues in the code, the goal of this whole exercise,
much more pleasant IMHO :-)
Thanks,
Knut
runchecks: Generalize make C={1,2} to support multiple checkers
Documentation: Add doc for runchecks, a checker runner
checkpatch: Improve --fix-inplace for TABSTOP
rds: Add runchecks.cfg for net/rds
RDMA/core: Add runchecks.cfg for drivers/infiniband/core
Documentation/dev-tools/coccinelle.rst | 12 +-
Documentation/dev-tools/index.rst | 1 +-
Documentation/dev-tools/runchecks.rst | 215 ++++++++-
Documentation/dev-tools/sparse.rst | 30 +-
Documentation/kbuild/kbuild.txt | 9 +-
Makefile | 23 +-
drivers/infiniband/core/runchecks.cfg | 83 +++-
net/rds/runchecks.cfg | 76 +++-
scripts/Makefile.build | 4 +-
scripts/checkpatch.pl | 2 +-
scripts/runchecks | 734 ++++++++++++++++++++++++++-
scripts/runchecks.cfg | 63 ++-
scripts/runchecks_help.txt | 43 ++-
13 files changed, 1274 insertions(+), 21 deletions(-)
create mode 100644 Documentation/dev-tools/runchecks.rst
create mode 100644 drivers/infiniband/core/runchecks.cfg
create mode 100644 net/rds/runchecks.cfg
create mode 100755 scripts/runchecks
create mode 100644 scripts/runchecks.cfg
create mode 100644 scripts/runchecks_help.txt
base-commit: ae64f9bd1d3621b5e60d7363bc20afb46aede215

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.

Post by Stephen Hemminger
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.

Thanks,
Knut

[Joe Perches]

Post by Knut Omang

Post by Stephen Hemminger
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=<TYPE> \
$(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

[Knut Omang]

Post by Joe Perches

Post by Knut Omang

Post by Stephen Hemminger
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=<TYPE> \
$(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

Post by Joe Perches
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

[Joe Perches]

Post by Knut Omang

Post by Joe Perches
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=<TYPE> \
$(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

[Knut Omang]

Post by Joe Perches

Post by Knut Omang

Post by Joe Perches
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=<TYPE> \
$(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.

Post by Joe Perches
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

[Jason Gunthorpe]

Post by Knut Omang
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

[Joe Perches]

Post by Joe Perches

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=<TYPE> \
$(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.

[Bart Van Assche]

Post by Joe Perches

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=<TYPE> \
$(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.

[Knut Omang]

Post by Bart Van Assche

Post by Joe Perches

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=<TYPE> \
$(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

[Leon Romanovsky]

Post by Knut Omang

Post by Bart Van Assche

Post by Joe Perches

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=<TYPE> \
$(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

[Knut Omang]

Post by Knut Omang

Post by Stephen Hemminger
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...

Post by Knut Omang

Post by Stephen Hemminger
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