Re: [PATCH v2 5/5] RDMA/core: Add runchecks.cfg for drivers/infiniband/core

[Knut Omang <knut.omang@oracle.com>]

On Mon, 2017-12-18 at 16:04 +0200, Leon Romanovsky wrote:
> On Mon, Dec 18, 2017 at 01:36:26PM +0100, Knut Omang wrote:
> > On Mon, 2017-12-18 at 10:02 +0200, Leon Romanovsky wrote:
> > > On Sat, Dec 16, 2017 at 03:42:30PM +0100, Knut Omang wrote:
> > > > Add a runchecks.cfg to drivers/infiniband/core
> > > > to start "reining in" future checker errors,
> > > > and making it easier to selectively clean up existing
> > > > issues.
> > > >
> > > > This runchecks.cfg lets make C=2 M=drivers/infiniband/core
> > > > pass with all errors/warnings suppressed
> > > >
> > > > See Documentation/dev-tools/runchecks.rst for
> > > > motivation and details.
> > > >
> > > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
> > > > Reviewed-by: Åsmund Østvold <asmund.ostvold@oracle.com>
> > > > ---
> > > >  drivers/infiniband/core/runchecks.cfg | 83 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 83 insertions(+)
> > > >  create mode 100644 drivers/infiniband/core/runchecks.cfg
> > > >
> > > > diff --git a/drivers/infiniband/core/runchecks.cfg
> b/drivers/infiniband/core/runchecks.cfg
> > > > new file mode 100644
> > > > index 0000000..75ca57c
> > > > --- /dev/null
> > > > +++ b/drivers/infiniband/core/runchecks.cfg
> > > > @@ -0,0 +1,83 @@
> > > > +#
> > > > +# checker suppression lists for drivers/infiniband/core
> > > > +#
> > > > +# see Documentation/dev-tools/runchecks.rst
> > > > +#
> > > > +
> > > > +checker checkpatch
> > > > +##################
> > > > +
> > > > +# Accept somewhat longer lines:
> > > > +line_len 110
> > > > +
> > > > +# Uncategorized:
> > > > +#
> > > > +except TRAILING_STATEMENTS packer.c
> > > > +except NON_OCTAL_PERMISSIONS rw.c
> > > > +except STATIC_CONST_CHAR_ARRAY sysfs.c
> > > > +except SYMBOLIC_PERMS sysfs.c
> > > > +except NEEDLESS_IF sysfs.c
> > > > +except NAKED_SSCANF device.c
> > > > +except ALLOC_WITH_MULTIPLY fmr_pool.c iwpm_util.c cma.c
> > > > +except MULTIPLE_ASSIGNMENTS rw.c verbs.c
> > > > +except ALLOC_SIZEOF_STRUCT sysfs.c iwpm_util.c iwpm_msg.c cma.c cm.c iwcm.c
> > > > +except LOGICAL_CONTINUATIONS mad.c iwpm_util.c user_mad.c
> > > > +
> > > > +# Important to fix/go through from a quality perspective:
> > > > +#
> > > > +except AVOID_BUG rw.c mad.c cm.c iwcm.c cma.c
> > > > +except UNCOMMENTED_DEFINITION ucma.c fmr_pool.c multicast.c mad_rmpp.c
> > > > +except UNCOMMENTED_DEFINITION ucm.c umem.c cma.c user_mad.c cm.c
> > > > +except ASSIGN_IN_IF mad.c cma.c uverbs_ioctl_merge.c
> > > > +except SUSPECT_CODE_INDENT iwpm_util.c cma.c uverbs_ioctl.c user_mad.c
> uverbs_cmd.c
> > > > +except COMPLEX_MACRO uverbs_ioctl_merge.c
> > > > +except BOOL_COMPARISON roce_gid_mgmt.c
> > > > +
> > > > +# Code simplification:
> > > > +#
> > > > +except UNNECESSARY_ELSE cache.c mad.c mad_rmpp.c
> > > > +except UNNECESSARY_PARENTHESES cma.c sa_query.c mad.c ucma.c mad_rmpp.c umem.c
> > > > +except UNNECESSARY_PARENTHESES user_mad.c uverbs_cmd.c uverbs_marshall.c cm.c
> > > > +except EMBEDDED_FUNCTION_NAME mad.c umem.c cma.c ucma.c user_mad.c
> > > > +except RETURN_VOID sysfs.c sa_query.c mad.c cma.c ucm.c uverbs_main.c umem.c
> > > > +except CONSTANT_COMPARISON smi.c
> > > > +except OOM_MESSAGE iwpm_util.c
> > > > +
> > > > +# Style and readability:
> > > > +#
> > > > +except OPEN_BRACE cache.c mad.c umem_odp.c umem_rbtree.c cm.c
> > > > +except MULTILINE_DEREFERENCE mad.c cm.c cma.c uverbs_main.c
> > > > +except LONG_LINE cma.c
> > > > +except PARENTHESIS_ALIGNMENT umem.c cm.c
> > > > +except FUNCTION_ARGUMENTS verbs.c uverbs_cmd.c sa_query.c sysfs.c
> > > > +
> > > > +# Candidates to leave as exceptions (don't fix):
> > > > +except SPACING umem_rbtree.c
> > > > +except MACRO_ARG_REUSE ud_header.c sa_query.c uverbs_ioctl_merge.c
> > > > +
> > > > +# These are in most of the source files, ignore for all files:
> > > > +#
> > > > +pervasive BLOCK_COMMENT_STYLE ENOSYS BRACES OPEN_ENDED_LINE
> > > > +
> > > > +# These are easily autocorrected by checkpatch with --fix-inplace:
> > > > +#
> > > > +pervasive SIZEOF_PARENTHESIS SPACING LINE_SPACING SPACE_BEFORE_TAB TABSTOP
> > > > +pervasive TRAILING_WHITESPACE POINTER_LOCATION INITIALISED_STATIC
> > > > +pervasive CODE_INDENT ELSE_AFTER_BRACE SYMBOLIC_PERMS LEADING_SPACE
> > > > +pervasive PARENTHESIS_ALIGNMENT COMPARISON_TO_NULL TYPO_SPELLING
> > > > +
> > > > +checker sparse
> > > > +##############
> > > > +
> > > > +except SHADOW sysfs.c fmr_pool.c user_mad.c uverbs_main.c ucm.c ucma.c
> > > > +except RETURN_VOID roce_gid_mgmt.c
> > > > +except TYPESIGN ucm.c ucma.c
> > > > +
> > > > +# From linux/device.h:
> > > > +except RETURN_VOID ucm.c
> > > > +
> > > > +checker checkdoc
> > > > +################
> > > > +
> > > > +except PARAM_DESC verbs.c device.c fmr_pool.c cache.c sa_query.c
> > > > +except X_PARAM verbs.c fmr_pool.c cache.c
> > >
> > > I missed most of the patches from this series and previous version too,
> > > but in regards to IB. I don't want to see static checkers exceptions for
> > > core code. It does make a lot of sense for drivers which can be unmaintained.
> >
> > And I couldn't agree more. But some aid in assessing the scope of the task
> > and tracking progress/prevent regressions would be good, wouldn't it?
> 
> In current form, it doesn't fully prevent regressions, for example the
> line "except TYPESIGN ucm.c ucma.c" means that new code with such errors
> to the files ucm.c/ucma.c will be missed too.

Yes, that's true.

> > This file + enabling a build bot to run make C=2 allows you to prevent
> > any checker error types from appearing anywhere in core apart from for
> > the exception files and starting from this point of.
> 
> We are talking about the same and our goal is the same too - to see it is merged.
> I'm just saying that for the IB part, cleanup of IB/core is needed prior
> to merging IB related runcheck.cfg file, so it will include drivers only.

appreciated, 
my take on it is that this initial runchecks.cfg file is more 
of a plan for improvements than an exception list, and that the file can be used
to track changes and for you to indicate importance or state of different 
checks.

> > > Also, I agree with other reviewers, there is no excuse for adding
> > > checkpatch specifics per-subsystem/folder, the differences are better
> > > to be treated in checkpatch.pl itself.
> >
> > Hopefully my intention should be clearer if you look at the documentation:
> >
> > https://lkml.org/lkml/2017/12/16/135
> >
> > The best is if no exceptions are needed, but until then let's at least get
> > automatic testing so that you and other reviewers can focus on the more important
> > issues?
> 
> Hiding simply mean that such errors will never be fixed

I agree that's a potential way to misuse this tool.

It depends on how we view/use the runchecks.cfg file - after all it is a documentation of
issues that exists, and can be used to indicate why it has not been fixed yet or to point 
contributors to where the biggest pain points are.

Right now important detections that may hide potential bugs easily 
drown in whitespace or sizeof issues. I found it very useful just to 
do the exercise of creating these suppression lists. 

I first started out fixing issues while the checkers discovered them, 
but after a while I ended up taking a step back and starting over 
again by first creating the "todo list" in the form of a runchecks.cfg file that 
narrowly suppresses everything, then starting to fix issues type by type while
narrowing the exceptions list in the runchecks.cfg file 
this gives IMHO cleaner and more focused commits, and every step would 
pass the 'make C=2' test with no errors.

Thanks,
Knut

> Thanks
> 
> >
> > Thanks,
> > Knut
> >
> > >
> > > Thanks
> > >
> > > > --
> > > > git-series 0.9.1