[LTP] [RFC PATCH 0/4] Auto review and Coccinelle

[Richard Palethorpe]

Hello,

tldr; It looks like we can do a lot with Coccinelle. Just need to
      figure out how to apply it. clang-tidy is in second place.

This patch set implements some checks for a basic LTP library rule and
contains the auto generated fix. The rule being: never use TEST,
TST_RET or TST_ERR in the library. So that the test author can assume
these never change unless they use TEST().

I think this is a good rule, atlhough I did not know it was a rule
until Cyril pointed it out in my CGroups patch. Then I fixed it in one
place, but still left another usage. The patch set was merged with the
error in.

The only way this error would be discovered is with further manual
code review or if a test author found it. As the safe_cgroup_*
functions are likely to always pass, this could have resulted in nasty
false negatives if a test used TEST then called SAFE_CGROUP_READ
before checking the results.

Of course errno has a similar problem, but then that is why we have
TST_ERR. If people feel it is necessary we could introduced TEST_LIB()
and associated variables.

Alot of review comments are just pointing out LTP library usage errors
or even basic C coding errors. I believe a large chunk of these errors
can be automatically detected. At least theoretically, in practice the
tools are a problem.

I have identified and spent some time with the following tools:

Coccinelle (spatch)
clang --analyze
clang-tidy
gcc -fanalyzer
gcc plugin API
sparse
smatch
check_patch.pl

Clang analyzer, GCC analyzer and Smatch are doing full state (value
range) tracking for variables. They are the most powerful tools here,
but they all have different issues. People should try using gcc and
clang in their personal workflows. However these do not represent low
hanging fruit IMO.

smatch is based on sparse, both are used on the kernel, but I ran into
all kinds of issues on my system when using these on the LTP. sparse
is simpler to understand than gcc, but then gcc works everywhere.

The same is mostly true of clang-tidy which we can probably just use
with a custom configuration to find some generic C errors. The plugin
interface looks easier to use than GCC's.

We should probably automate check_patch.pl somehow, but extending it
doesn't seem like a good idea.

Finally Coccinelle allows quite advanced analyses and updating without
huge effort. It doesn't appear to track variable states, although it
allows some scripting which may allow limited context
tracking. However that is not low hanging fruit. For checking stuff
like "tst_reap_children() is used instead of wait or SAFE_WAIT", it
should work fine.

I'm not sure how to integrate it with the build system. We may just
want to do something similar to the kernel. Also I guess we want to
have a way of checking patches sent to the mailing list.

Note that I haven't tested these changes by running all the
tests. Only that they compile!

Richard Palethorpe (4):
  Add scripts to remove TEST in library
  Add script to run Coccinelle checks
  API: Mostly automatic removal of TEST() usage by Coccinelle
  API: Removal of TST_ERR usage

 lib/tst_af_alg.c                              |  46 +++---
 lib/tst_cgroup.c                              |  13 +-
 lib/tst_crypto.c                              |  20 +--
 lib/tst_netdevice.c                           |  10 +-
 lib/tst_rtnetlink.c                           |   4 +-
 lib/tst_supported_fs_types.c                  |  10 +-
 .../coccinelle/libltp-test-macro-vars.cocci   |  54 +++++++
 scripts/coccinelle/libltp-test-macro.cocci    | 137 ++++++++++++++++++
 scripts/coccinelle/libltp_checks.sh           |  29 ++++
 9 files changed, 277 insertions(+), 46 deletions(-)
 create mode 100644 scripts/coccinelle/libltp-test-macro-vars.cocci
 create mode 100644 scripts/coccinelle/libltp-test-macro.cocci
 create mode 100755 scripts/coccinelle/libltp_checks.sh

-- 
2.31.1