| Message ID | 20210524144745.10887-1-rpalethorpe@suse.com |
|---|---|
| Headers | show |
| Series | Auto review and Coccinelle | expand |
Hi! > 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. I guess that having it in travis as a post commit check would be better than nothing. Pre commit hook would be ideal but requiring coccinelle installed for LTP development would increase the bar for contribution too much I guess.
Hi! > > I guess that having it in travis as a post commit check would be better > > than nothing. > > > > Pre commit hook would be ideal but requiring coccinelle installed for > > LTP development would increase the bar for contribution too much I > > guess. > > I fear this defeats my primary goal of giving very quick feedback > without involving patch submission. This makes me think of clang-tidy > (clang-tools?) again. It will probably be more difficult to write LTP > specific checks, but I guess every desktop Linux distro less than 10 > years old has Clang? As far as I can tell clang is generally present on modern distributions while coccinelle tends to be problematic on some distributions. It's a great tool but it seems that there is a shortage of maintainers that can maintain ocaml packages. > I don't think there is much else I can do than try writing the same > check in clang as well. See how that goes... If that works well enough I would vote to a switch to clang-tidy. > Anyway, we could copy the kernel to some extent. Make it so running > > make coccicheck > > or > > make clang-tidy > > or more generic > > make check > > Will recursively run the checks on the files under the current > directory? Sounds like a good plan.
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> 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. > > I guess that having it in travis as a post commit check would be better > than nothing. > > Pre commit hook would be ideal but requiring coccinelle installed for > LTP development would increase the bar for contribution too much I > guess. I fear this defeats my primary goal of giving very quick feedback without involving patch submission. This makes me think of clang-tidy (clang-tools?) again. It will probably be more difficult to write LTP specific checks, but I guess every desktop Linux distro less than 10 years old has Clang? I don't think there is much else I can do than try writing the same check in clang as well. See how that goes... Anyway, we could copy the kernel to some extent. Make it so running make coccicheck or make clang-tidy or more generic make check Will recursively run the checks on the files under the current directory?
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