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

[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

[LTP] [RFC PATCH 1/4] Add scripts to remove TEST in library

[Richard Palethorpe]

---
 .../coccinelle/libltp-test-macro-vars.cocci   |  54 +++++++
 scripts/coccinelle/libltp-test-macro.cocci    | 137 ++++++++++++++++++
 2 files changed, 191 insertions(+)
 create mode 100644 scripts/coccinelle/libltp-test-macro-vars.cocci
 create mode 100644 scripts/coccinelle/libltp-test-macro.cocci

diff --git a/scripts/coccinelle/libltp-test-macro-vars.cocci b/scripts/coccinelle/libltp-test-macro-vars.cocci
new file mode 100644
index 000000000..679ce80fe
--- /dev/null
+++ b/scripts/coccinelle/libltp-test-macro-vars.cocci
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2021 SUSE LLC  <rpalethorpe@suse.com>
+
+// The TEST macro should not be used in the library because it sets
+// TST_RET and TST_ERR which are global variables. The test author
+// only expects these to be changed if *they* call TEST directly.
+
+// Set with -D fix
+virtual fix
+// Set with -D report
+virtual report
+
+// Find all positions where TEST's variables are used
+@ find_use exists @
+identifier tst_var =~ "TST_(ERR|RET)";
+position p;
+expression E;
+@@
+
+(
+ tst_var@p
+|
+ TTERRNO@p | E
+)
+
+@initialize:python depends on report@
+@@
+
+import json
+
+errors = []
+
+@script:python depends on report@
+p << find_use.p;
+@@
+
+msg = {
+    'msg': "TEST macro variable use in library",
+    'file': p[0].file,
+    'line': p[0].line,
+    'col': p[0].column,
+}
+
+errors.append(msg)
+
+@finalize:python depends on report@
+@@
+
+msgs = {
+     'check_script': 'coccinelle/libltp-test-macro-vars.cocci',
+     'errors': errors,
+}
+
+print(json.dumps(msgs, indent=2))
diff --git a/scripts/coccinelle/libltp-test-macro.cocci b/scripts/coccinelle/libltp-test-macro.cocci
new file mode 100644
index 000000000..960b5d325
--- /dev/null
+++ b/scripts/coccinelle/libltp-test-macro.cocci
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2021 SUSE LLC  <rpalethorpe@suse.com>
+
+// The TEST macro should not be used in the library because it sets
+// TST_RET and TST_ERR which are global variables. The test author
+// only expects these to be changed if *they* call TEST directly.
+
+// Set with -D fix
+virtual fix
+// Set with -D report
+virtual report
+
+// Find all positions where TEST is _used_.
+@ find_use exists @
+position p;
+@@
+
+ TEST@p(...);
+
+// Print the position of the TEST usage
+@initialize:python depends on report@
+@@
+
+import json
+
+errors = []
+
+@script:python depends on report@
+p << find_use.p;
+@@
+
+msg = {
+    'msg': "TEST macro use in library",
+    'file': p[0].file,
+    'line': p[0].line,
+    'col': p[0].column,
+}
+
+errors.append(msg)
+
+@finalize:python depends on report@
+@@
+
+msgs = {
+     'check_script': 'coccinelle/libltp-test-macro.cocci',
+     'errors': errors,
+}
+
+print(json.dumps(msgs, indent=2))
+
+// Below are rules which will create a patch to replace TEST usage
+// It assumes we can use the ret var without conflicts
+
+// Fix all references to the variables TEST modifies when they occur in a
+// function where TEST was used.
+@ depends on fix && find_use @
+@@
+
+(
+- TST_RET
++ ret
+|
+- TST_ERR
++ errno
+|
+- TTERRNO
++ TERRNO
+)
+
+// Replace TEST in all functions where it occurs only at the start. It
+// is slightly complicated by adding a newline if a statement appears
+// on the line after TEST(). It is not clear to me what the rules are
+// for matching whitespace as it has no semantic meaning, but this
+// appears to work.
+@ depends on fix @
+identifier fn;
+expression tested_expr;
+statement st;
+@@
+
+  fn (...)
+  {
+- 	TEST(tested_expr);
++	const long ret = tested_expr;
+(
++
+	st
+|
+
+)
+	... when != TEST(...)
+  }
+
+// Replace TEST in all functions where it occurs at the start
+// Functions where it *only* occurs at the start were handled above
+@ depends on fix @
+identifier fn;
+expression tested_expr;
+statement st;
+@@
+
+  fn (...)
+  {
+- 	TEST(tested_expr);
++	long ret = tested_expr;
+(
++
+	st
+|
+
+)
+	...
+  }
+
+// Add ret var@the start of a function where TEST occurs and there
+// is not already a ret declaration
+@ depends on fix exists @
+identifier fn;
+@@
+
+  fn (...)
+  {
++	long ret;
+	... when != long ret;
+
+	TEST(...)
+	...
+  }
+
+// Replace any remaining occurances of TEST
+@ depends on fix @
+expression tested_expr;
+@@
+
+- 	TEST(tested_expr);
++	ret = tested_expr;
+
-- 
2.31.1

[LTP] [RFC PATCH 2/4] Add script to run Coccinelle checks

[Richard Palethorpe]

---
 scripts/coccinelle/libltp_checks.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100755 scripts/coccinelle/libltp_checks.sh

diff --git a/scripts/coccinelle/libltp_checks.sh b/scripts/coccinelle/libltp_checks.sh
new file mode 100755
index 000000000..6fdaa7ae8
--- /dev/null
+++ b/scripts/coccinelle/libltp_checks.sh
@@ -0,0 +1,29 @@
+#!/bin/sh -eu
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2021 SUSE LLC  <rpalethorpe@suse.com>
+
+# Run the Coccinelle checks for the library. Running the fixes
+# requires passing -D fix instead of -D report.
+
+if [[ ! -d lib || ! -d scripts/coccinelle ]]; then
+    echo "$0: Can't find lib or scripts directories. Run me from top src dir"
+    exit 1
+fi
+
+echo Python args ${COCCI_PYTHON:=--python python3} >&2
+
+libltp_spatch() {
+    spatch $COCCI_PYTHON --dir lib \
+	   --ignore lib/parse_opts.c \
+	   --ignore lib/newlib_tests \
+	   --ignore lib/tests \
+	   --very-quiet \
+	   --use-gitgrep \
+	   -D report \
+	   --include-headers \
+	   $*
+}
+
+libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro.cocci
+libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro-vars.cocci \
+	      --ignore lib/tst_test.c
-- 
2.31.1

[LTP] [RFC PATCH 3/4] API: Mostly automatic removal of TEST() usage by Coccinelle

[Richard Palethorpe]

Only manual change is to set ret = -1 due to prevent compiler warning
in cgroup read.
---
 lib/tst_af_alg.c             | 46 ++++++++++++++++++++----------------
 lib/tst_cgroup.c             | 13 +++++-----
 lib/tst_crypto.c             | 20 +++++++++-------
 lib/tst_supported_fs_types.c | 10 ++++----
 4 files changed, 50 insertions(+), 39 deletions(-)

diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c
index d3895a83d..05caa6301 100644
--- a/lib/tst_af_alg.c
+++ b/lib/tst_af_alg.c
@@ -13,25 +13,28 @@
 
 int tst_alg_create(void)
 {
-	TEST(socket(AF_ALG, SOCK_SEQPACKET, 0));
-	if (TST_RET >= 0)
-		return TST_RET;
-	if (TST_ERR == EAFNOSUPPORT)
+	const long ret = socket(AF_ALG, SOCK_SEQPACKET, 0);
+
+	if (ret >= 0)
+		return ret;
+	if (errno == EAFNOSUPPORT)
 		tst_brk(TCONF, "kernel doesn't support AF_ALG");
-	tst_brk(TBROK | TTERRNO, "unexpected error creating AF_ALG socket");
+	tst_brk(TBROK | TERRNO, "unexpected error creating AF_ALG socket");
 	return -1;
 }
 
 void tst_alg_bind_addr(int algfd, const struct sockaddr_alg *addr)
 {
-	TEST(bind(algfd, (const struct sockaddr *)addr, sizeof(*addr)));
-	if (TST_RET == 0)
+	const long ret = bind(algfd, (const struct sockaddr *)addr,
+			      sizeof(*addr));
+
+	if (ret == 0)
 		return;
-	if (TST_ERR == ENOENT) {
+	if (errno == ENOENT) {
 		tst_brk(TCONF, "kernel doesn't support %s algorithm '%s'",
 			addr->salg_type, addr->salg_name);
 	}
-	tst_brk(TBROK | TTERRNO,
+	tst_brk(TBROK | TERRNO,
 		"unexpected error binding AF_ALG socket to %s algorithm '%s'",
 		addr->salg_type, addr->salg_name);
 }
@@ -63,6 +66,7 @@ void tst_alg_bind(int algfd, const char *algtype, const char *algname)
 
 bool tst_have_alg(const char *algtype, const char *algname)
 {
+	long ret;
 	int algfd;
 	struct sockaddr_alg addr;
 	bool have_alg = true;
@@ -71,10 +75,10 @@ bool tst_have_alg(const char *algtype, const char *algname)
 
 	init_sockaddr_alg(&addr, algtype, algname);
 
-	TEST(bind(algfd, (const struct sockaddr *)&addr, sizeof(addr)));
-	if (TST_RET != 0) {
-		if (TST_ERR != ENOENT) {
-			tst_brk(TBROK | TTERRNO,
+	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
+	if (ret != 0) {
+		if (errno != ENOENT) {
+			tst_brk(TBROK | TERRNO,
 				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
 				algtype, algname);
 		}
@@ -96,6 +100,7 @@ void tst_require_alg(const char *algtype, const char *algname)
 
 void tst_alg_setkey(int algfd, const uint8_t *key, unsigned int keylen)
 {
+	long ret;
 	uint8_t *keybuf = NULL;
 	unsigned int i;
 
@@ -106,9 +111,9 @@ void tst_alg_setkey(int algfd, const uint8_t *key, unsigned int keylen)
 			keybuf[i] = rand();
 		key = keybuf;
 	}
-	TEST(setsockopt(algfd, SOL_ALG, ALG_SET_KEY, key, keylen));
-	if (TST_RET != 0) {
-		tst_brk(TBROK | TTERRNO,
+	ret = setsockopt(algfd, SOL_ALG, ALG_SET_KEY, key, keylen);
+	if (ret != 0) {
+		tst_brk(TBROK | TERRNO,
 			"unexpected error setting key (len=%u)", keylen);
 	}
 	free(keybuf);
@@ -116,12 +121,13 @@ void tst_alg_setkey(int algfd, const uint8_t *key, unsigned int keylen)
 
 int tst_alg_accept(int algfd)
 {
-	TEST(accept(algfd, NULL, NULL));
-	if (TST_RET < 0) {
-		tst_brk(TBROK | TTERRNO,
+	const long ret = accept(algfd, NULL, NULL);
+
+	if (ret < 0) {
+		tst_brk(TBROK | TERRNO,
 			"unexpected error accept()ing AF_ALG request socket");
 	}
-	return TST_RET;
+	return ret;
 }
 
 int tst_alg_setup(const char *algtype, const char *algname,
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1e036d3c3..5d13e2829 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1037,6 +1037,7 @@ ssize_t safe_cgroup_read(const char *const file, const int lineno,
 			 const char *const file_name,
 			 char *const out, const size_t len)
 {
+	long ret = -1;
 	const struct cgroup_file *const cfile =
 		cgroup_file_find(file, lineno, file_name);
 	struct cgroup_dir *const *dir;
@@ -1051,9 +1052,9 @@ ssize_t safe_cgroup_read(const char *const file, const int lineno,
 		if (prev_len)
 			memcpy(prev_buf, out, prev_len);
 
-		TEST(safe_file_readat(file, lineno,
-				      (*dir)->dir_fd, alias, out, len));
-		if (TST_RET < 0)
+		ret = safe_file_readat(file, lineno, (*dir)->dir_fd, alias,
+				       out, len);
+		if (ret < 0)
 			continue;
 
 		if (prev_len && memcmp(out, prev_buf, prev_len)) {
@@ -1063,12 +1064,12 @@ ssize_t safe_cgroup_read(const char *const file, const int lineno,
 			break;
 		}
 
-		prev_len = MIN(sizeof(prev_buf), (size_t)TST_RET);
+		prev_len = MIN(sizeof(prev_buf), (size_t) ret);
 	}
 
-	out[MAX(TST_RET, 0)] = '\0';
+	out[MAX(ret, 0)] = '\0';
 
-	return TST_RET;
+	return ret;
 }
 
 void safe_cgroup_printf(const char *const file, const int lineno,
diff --git a/lib/tst_crypto.c b/lib/tst_crypto.c
index 685e0871e..c01632c2a 100644
--- a/lib/tst_crypto.c
+++ b/lib/tst_crypto.c
@@ -14,16 +14,17 @@
 
 void tst_crypto_open(struct tst_crypto_session *ses)
 {
-	TEST(socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CRYPTO));
-	if (TST_RET < 0 && TST_ERR == EPROTONOSUPPORT)
-		tst_brk(TCONF | TTERRNO, "NETLINK_CRYPTO is probably disabled");
+	const long ret = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CRYPTO);
 
-	if (TST_RET < 0) {
-		tst_brk(TBROK | TTERRNO,
+	if (ret < 0 && errno == EPROTONOSUPPORT)
+		tst_brk(TCONF | TERRNO, "NETLINK_CRYPTO is probably disabled");
+
+	if (ret < 0) {
+		tst_brk(TBROK | TERRNO,
 			"socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CRYPTO)");
 	}
 
-	ses->fd = TST_RET;
+	ses->fd = ret;
 	ses->seq_num = 0;
 }
 
@@ -83,6 +84,7 @@ int tst_crypto_add_alg(struct tst_crypto_session *ses,
 int tst_crypto_del_alg(struct tst_crypto_session *ses,
 		       const struct crypto_user_alg *alg)
 {
+	long ret;
 	unsigned int i = 0;
 	struct nlmsghdr nh = {
 		.nlmsg_len = sizeof(struct nlmsghdr) + sizeof(*alg),
@@ -96,8 +98,8 @@ int tst_crypto_del_alg(struct tst_crypto_session *ses,
 
 		SAFE_NETLINK_SEND(ses->fd, &nh, alg);
 
-		TEST(tst_crypto_recv_ack(ses));
-		if (TST_RET != -EBUSY || i >= ses->retries)
+		ret = tst_crypto_recv_ack(ses);
+		if (ret != -EBUSY || i >= ses->retries)
 			break;
 
 		if (usleep(1) && errno != EINTR)
@@ -106,5 +108,5 @@ int tst_crypto_del_alg(struct tst_crypto_session *ses,
 		++i;
 	}
 
-	return TST_RET;
+	return ret;
 }
diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 592a526ae..d0f745490 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -167,14 +167,16 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)
 
 int tst_check_quota_support(const char *device, int format, char *quotafile)
 {
-	TEST(quotactl(QCMD(Q_QUOTAON, USRQUOTA), device, format, quotafile));
+	const long ret = quotactl(QCMD(Q_QUOTAON, USRQUOTA), device, format,
+				  quotafile);
 
 	/* Not supported */
-	if (TST_RET == -1 && TST_ERR == ESRCH)
+
+	if (ret == -1 && errno == ESRCH)
 		return 0;
 
 	/* Broken */
-	if (TST_RET)
+	if (ret)
 		return -1;
 
 	quotactl(QCMD(Q_QUOTAOFF, USRQUOTA), device, 0, 0);
@@ -192,5 +194,5 @@ void tst_require_quota_support_(const char *file, const int lineno,
 	}
 
 	if (status < 0)
-		tst_brk_(file, lineno, TBROK|TTERRNO, "FS quotas are broken");
+		tst_brk_(file, lineno, TBROK|TERRNO, "FS quotas are broken");
 }
-- 
2.31.1

[LTP] [RFC PATCH 4/4] API: Removal of TST_ERR usage

[Richard Palethorpe]

Use errno instead to stop Coccinelle test from failing.
---
 lib/tst_netdevice.c | 10 +++++-----
 lib/tst_rtnetlink.c |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/tst_netdevice.c b/lib/tst_netdevice.c
index d098173d5..cc2766be8 100644
--- a/lib/tst_netdevice.c
+++ b/lib/tst_netdevice.c
@@ -149,7 +149,7 @@ int tst_create_veth_pair(const char *file, const int lineno,
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to create veth interfaces %s+%s", ifname1,
 			ifname2);
 	}
@@ -183,7 +183,7 @@ int tst_remove_netdev(const char *file, const int lineno, const char *ifname)
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to remove netdevice %s", ifname);
 	}
 
@@ -232,7 +232,7 @@ static int modify_address(const char *file, const int lineno,
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to modify %s network address", ifname);
 	}
 
@@ -301,7 +301,7 @@ static int change_ns(const char *file, const int lineno, const char *ifname,
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to move %s to another namespace", ifname);
 	}
 
@@ -392,7 +392,7 @@ static int modify_route(const char *file, const int lineno, unsigned int action,
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to modify network route");
 	}
 
diff --git a/lib/tst_rtnetlink.c b/lib/tst_rtnetlink.c
index 1ecda3a9f..d93f7e18d 100644
--- a/lib/tst_rtnetlink.c
+++ b/lib/tst_rtnetlink.c
@@ -380,7 +380,7 @@ int tst_rtnl_check_acks(const char *file, const int lineno,
 		}
 
 		if (res->err->error) {
-			TST_ERR = -res->err->error;
+			errno = -res->err->error;
 			return 0;
 		}
 	}
@@ -394,7 +394,7 @@ int tst_rtnl_send_validate(const char *file, const int lineno,
 	struct tst_rtnl_message *response;
 	int ret;
 
-	TST_ERR = 0;
+	errno = 0;
 
 	if (tst_rtnl_send(file, lineno, ctx) <= 0)
 		return 0;
-- 
2.31.1

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

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Richard Palethorpe]

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?

-- 
Thank you,
Richard.