[v2,4/5] rds: Add runchecks.cfg for net/rds

Commit Message

Knut Omang Dec. 16, 2017, 2:42 p.m. UTC

Add a runchecks.cfg to net/rds 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=net/rds
pass with all errors/warnings suppressed

See Documentation/dev-tools/runchecks.rst for
motivation and details.

Signed-off-by: Knut Omang <[e-mail]>
Reviewed-by: Håkon Bugge <[e-mail]>
Reviewed-by: Åsmund Østvold <[e-mail]>
---
 net/rds/runchecks.cfg | 76 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+)
 create mode 100644 net/rds/runchecks.cfg

Comments

Stephen Hemminger Dec. 16, 2017, 5:45 p.m. UTC | #1

On Sat, 16 Dec 2017 15:42:29 +0100
Knut Omang <[e-mail]> wrote:

> +
> +# Important to fix from a quality perspective:
> +#
> +except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c info.c loop.c message.c
> +except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
> +except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
> +except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
> +except UNNECESSARY_ELSE bind.c ib_cm.c
> +except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
> +except MACRO_ARG_REUSE rds.h
> +except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
> +except UNCOMMENTED_DEFINITION ib_cm.c
> +
> +# Code simplification:
> +#
> +except ALLOC_WITH_MULTIPLY ib.c
> +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
> +except UNNECESSARY_ELSE ib_fmr.c
> +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> +except PRINTK_RATELIMITED ib_frmr.c
> +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> +
> +# Style and readability:
> +#
> +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> +except OOM_MESSAGE ib.c tcp.c
> +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> +except OPEN_ENDED_LINE recv.c ib_recv.c
> +
> +# Candidates to leave as exceptions (don't fix):
> +except MULTIPLE_ASSIGNMENTS ib_send.c
> +except LONG_LINE_STRING connection.c
> +except OPEN_BRACE connection.c
> +

Why start letting subsystems have a free-pass?
Also this would mean that new patches to IB would continue the bad habits.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Knut Omang Dec. 17, 2017, 1:46 a.m. UTC | #2

On Sat, 2017-12-16 at 09:45 -0800, Stephen Hemminger wrote:
> On Sat, 16 Dec 2017 15:42:29 +0100
> Knut Omang <[e-mail]> wrote:
> 
> > +
> > +# Important to fix from a quality perspective:
> > +#
> > +except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c
> info.c loop.c message.c
> > +except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
> > +except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
> > +except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
> > +except UNNECESSARY_ELSE bind.c ib_cm.c
> > +except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
> > +except MACRO_ARG_REUSE rds.h
> > +except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
> > +except UNCOMMENTED_DEFINITION ib_cm.c
> > +
> > +# Code simplification:
> > +#
> > +except ALLOC_WITH_MULTIPLY ib.c
> > +except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c
> transport.c
> > +except UNNECESSARY_ELSE ib_fmr.c
> > +except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
> > +except PRINTK_RATELIMITED ib_frmr.c
> > +except EMBEDDED_FUNCTION_NAME ib_rdma.c
> > +
> > +# Style and readability:
> > +#
> > +except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
> > +except OOM_MESSAGE ib.c tcp.c
> > +except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
> > +except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
> > +except OPEN_ENDED_LINE recv.c ib_recv.c
> > +
> > +# Candidates to leave as exceptions (don't fix):
> > +except MULTIPLE_ASSIGNMENTS ib_send.c
> > +except LONG_LINE_STRING connection.c
> > +except OPEN_BRACE connection.c
> > +
> 
> Why start letting subsystems have a free-pass?

It's not a free pass, on the contrary - it's a way to enable the build bots/CI systems to
prevent regressions!

Right now, no automatic system can be set up to run checkpatch on almost any subsystem in
the kernel because there are so many warnings. That means that regressions happens all
over the place, even on source files and for types of checks that there currently are no
issues. Also reviewers have to spend time correcting whitespace issues which automation
can really handle much better!

Now, let's assume that we get the build bots to run their builds with make C=2 (which my
patches here allow, since it produces 0 warnings by design and by default)

Once this patch is in, errors of any kind of any of the types that are *not* excepted by
this file will break the build and generate a warning report, forcing the committer to fix
the errors right away. To me that's a big improvement from today.

> Also this would mean that new patches to IB would continue the bad habits

That's **only for the excepted types and files, and a temporary situation 
until we can fix the rest of the issues.

See my additional patch set here as an example of how I see us attack this piecemeal:

https://github.com/knuto/linux/tree/runchecks

I'll post that set as soon as patch #1/2 here is in.

I hope this clarifies!

Thanks,
Knut

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/rds/runchecks.cfg b/net/rds/runchecks.cfg
new file mode 100644
index 0000000..2a02701
--- /dev/null
+++ b/net/rds/runchecks.cfg
@@ -0,0 +1,76 @@ 
+#
+# checker suppression lists for net/rds
+#
+# see Documentation/dev-tools/runchecks.rst
+#
+
+checker checkpatch
+##################
+
+# Accept somewhat longer lines:
+line_len 110
+
+# Important to fix from a quality perspective:
+#
+except AVOID_BUG connection.c ib.c ib_cm.c ib_rdma.c ib_recv.c ib_ring.c ib_send.c info.c loop.c message.c
+except AVOID_BUG rdma.c recv.c send.c stats.c tcp_recv.c transport.c
+except MEMORY_BARRIER ib_recv.c send.c tcp_send.c
+except WAITQUEUE_ACTIVE cong.c ib_rdma.c ib_ring.c ib_send.c
+except UNNECESSARY_ELSE bind.c ib_cm.c
+except MACRO_ARG_PRECEDENCE connection.c ib.h rds.h
+except MACRO_ARG_REUSE rds.h
+except ALLOC_SIZEOF_STRUCT cong.c ib.c ib_cm.c loop.c message.c rdma.c
+except UNCOMMENTED_DEFINITION ib_cm.c
+
+# Code simplification:
+#
+except ALLOC_WITH_MULTIPLY ib.c
+except PREFER_PR_LEVEL ib_cm.c ib_recv.c ib_send.c rdma_transport.c threads.c transport.c
+except UNNECESSARY_ELSE ib_fmr.c
+except UNNECESSARY_PARENTHESES ib_rdma.c rdma.c recv.c send.c
+except PRINTK_RATELIMITED ib_frmr.c
+except EMBEDDED_FUNCTION_NAME ib_rdma.c
+
+# Style and readability:
+#
+except BRACES ib_cm.c ib_rdma.c ib_recv.c send.c transport.c
+except OOM_MESSAGE ib.c tcp.c
+except LONG_LINE_STRING ib.c ib_recv.c ib_send.c
+except FUNCTION_ARGUMENTS ib.h ib_mr.h rds.h tcp.h
+except OPEN_ENDED_LINE recv.c ib_recv.c
+
+# Candidates to leave as exceptions (don't fix):
+except MULTIPLE_ASSIGNMENTS ib_send.c
+except LONG_LINE_STRING connection.c
+except OPEN_BRACE connection.c
+
+# These are in most of the source files, ignore for all files:
+#
+pervasive NETWORKING_BLOCK_COMMENT_STYLE BLOCK_COMMENT_STYLE
+
+# These are easily autocorrected by checkpatch with --fix-inplace:
+# Just ignore here - fixed in a separate commit:
+#
+pervasive PREFER_KERNEL_TYPES LINE_SPACING SPACING SPACE_BEFORE_TAB SPLIT_STRING
+pervasive BIT_MACRO SIZEOF_PARENTHESIS LOGICAL_CONTINUATIONS GLOBAL_INITIALISERS
+pervasive ALLOC_WITH_MULTIPLY TABSTOP
+
+# These were easy to fix manually while getting make C=2 to pass.
+# Fixed in a separate commit:
+#
+pervasive SUSPECT_CODE_INDENT PARENTHESIS_ALIGNMENT BRACES PRINTF_L COMPARISON_TO_NULL
+pervasive LOGICAL_CONTINUATIONS
+
+
+checker sparse
+##############
+
+except VLA connection.c
+except COND_ADDRESS_ARRAY connection.c
+except PTR_SUBTRACTION_BLOWS ib_recv.c
+except TYPESIGN ib_frmr.c
+
+# This is coming from linux/dma-mapping.h:
+except RETURN_VOID ib_cm.c
+
+pervasive BITWISE SHADOW