[LTP] [PATCH v4 1/3] Add Coccinelle helper scripts for reference

[LTP] [PATCH v4 1/3] Add Coccinelle helper scripts for reference

[Richard Palethorpe]

Check-in a couple of semantic patches used for removing the TEST macro
from the library. Also include a shell script to run them with a
working set of arguments.

These are only intended to help someone develop their own refactoring
or check scripts. Not for running automatically.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V4:
* Break rtnetlink/netdevice into separate patch and save errno

V3:
* Make proper shell script to run spatch
* Fix some issues people would likely encounter with semantic patches

V2:
* Simplify the Cocci scripts
* Simplify the patchset and combine it with the separate CGroups patch
* Testing & sign-off

 .../coccinelle/libltp-test-macro-vars.cocci   |  19 ++++
 scripts/coccinelle/libltp-test-macro.cocci    | 107 ++++++++++++++++++
 scripts/coccinelle/run-spatch.sh              | 106 +++++++++++++++++
 3 files changed, 232 insertions(+)
 create mode 100644 scripts/coccinelle/libltp-test-macro-vars.cocci
 create mode 100644 scripts/coccinelle/libltp-test-macro.cocci
 create mode 100755 scripts/coccinelle/run-spatch.sh

diff --git a/scripts/coccinelle/libltp-test-macro-vars.cocci b/scripts/coccinelle/libltp-test-macro-vars.cocci
new file mode 100644
index 000000000..ed5459a48
--- /dev/null
+++ b/scripts/coccinelle/libltp-test-macro-vars.cocci
@@ -0,0 +1,19 @@
+// 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.
+
+// Find all positions where TEST's variables are used
+@ find_use exists @
+expression E;
+@@
+
+(
+* TST_ERR
+|
+* TST_RET
+|
+* TTERRNO | E
+)
diff --git a/scripts/coccinelle/libltp-test-macro.cocci b/scripts/coccinelle/libltp-test-macro.cocci
new file mode 100644
index 000000000..7563d23aa
--- /dev/null
+++ b/scripts/coccinelle/libltp-test-macro.cocci
@@ -0,0 +1,107 @@
+// 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
+
+// Find all positions where TEST is _used_.
+@ depends on !fix exists @
+@@
+
+* TEST(...);
+
+// 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 exists @
+@@
+
+ TEST(...)
+
+ ...
+
+(
+- 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 at 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 occurrences of TEST
+@ depends on fix @
+expression tested_expr;
+@@
+
+- 	TEST(tested_expr);
++	ret = tested_expr;
+
diff --git a/scripts/coccinelle/run-spatch.sh b/scripts/coccinelle/run-spatch.sh
new file mode 100755
index 000000000..e8e6f47d8
--- /dev/null
+++ b/scripts/coccinelle/run-spatch.sh
@@ -0,0 +1,106 @@
+#!/bin/sh -eu
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2021 SUSE LLC  <rpalethorpe@suse.com>
+
+# Helper for running spatch Coccinelle scripts on the LTP source tree
+
+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
+
+do_fix=no
+
+# Run a script on the lib dir
+libltp_spatch() {
+    echo libltp_spatch $*
+
+    if [ $do_fix = yes ]; then
+	spatch --dir lib \
+	       --ignore lib/parse_opts.c \
+	       --ignore lib/newlib_tests \
+	       --ignore lib/tests \
+	       --use-gitgrep \
+	       --in-place \
+	       -D fix \
+	       --include-headers \
+	       $*
+    else
+	spatch --dir lib \
+	       --ignore lib/parse_opts.c \
+	       --ignore lib/newlib_tests \
+	       --ignore lib/tests \
+	       --use-gitgrep \
+	       --include-headers \
+	       $*
+    fi
+}
+
+tests_spatch() {
+        echo tests_spatch $*
+
+        if [ $do_fix = yes ]; then
+	    spatch --dir testcases \
+		   --use-gitgrep \
+		   --in-place \
+		   -D fix \
+		   --include-headers \
+		   $*
+	else
+	    spatch --dir testcases \
+		   --use-gitgrep \
+		   --include-headers \
+		   $*
+	fi
+}
+
+usage()
+{
+    cat <<EOF
+Usage:
+$0 [ -f ] <patch basename> [ <patch basename> [...] ]
+$0 -h
+
+Options:
+-f	Apply the semantic patch in-place to fix the code
+-h	You are reading it
+
+If run without -f then the semantic patch will only print locations
+where it matches or show a diff.
+
+EOF
+}
+
+while getopts "fh" opt; do
+    case $opt in
+	f) do_fix=yes;;
+	h|?) usage; exit $([ $opt = h ]);;
+    esac
+done
+
+shift $(($OPTIND - 1))
+
+if [ $# -eq 0 ]; then
+    echo -e "Missing semantic patch name \n"
+    usage; exit 1
+fi
+
+if [ $do_fix = yes ] && [ -n "$(git ls-files -m -d)" ]; then
+    echo "At least stage your current changes!"
+    exit 1
+fi
+
+for spatch_file in $*; do
+    case $spatch_file in
+	libltp-test-macro)
+	    libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro.cocci;;
+	libltp-test-macro-vars)
+	    libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro-vars.cocci \
+			  --ignore lib/tst_test.c;;
+	*)
+	    tests_spatch --sp-file scripts/coccinelle/$spatch_file.cocci;;
+    esac
+done
+
+
+
-- 
2.31.1

[LTP] [PATCH v4 2/3] API: Remove TEST macro usage from library

[Richard Palethorpe]

The test author is guaranteed that the LTP library will not change the
TST_RET and TST_ERR global variables. That way the test author can use
the TEST macro then call other library functions before using TST_RET,
TST_ERR or TTERRNO.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 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 6d94ea41c..2b99c81a4 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1073,6 +1073,7 @@ ssize_t safe_cgroup_read(const char *const file, const int lineno,
 	const char *alias;
 	size_t prev_len = 0;
 	char prev_buf[BUFSIZ];
+	ssize_t read_ret = 0;
 
 	for_each_dir(cg, cfile->ctrl_indx, dir) {
 		if (!(alias = cgroup_file_alias(cfile, *dir)))
@@ -1081,9 +1082,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)
+		read_ret = safe_file_readat(file, lineno,
+					    (*dir)->dir_fd, alias, out, len);
+		if (read_ret < 0)
 			continue;
 
 		if (prev_len && memcmp(out, prev_buf, prev_len)) {
@@ -1093,12 +1094,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)read_ret);
 	}
 
-	out[MAX(TST_RET, 0)] = '\0';
+	out[MAX(read_ret, 0)] = '\0';
 
-	return TST_RET;
+	return read_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] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Richard Palethorpe]

The test author is guaranteed that the library will not set TST_ERR
except via the TEST macro and similar.

Currently the rtnetlink & netdevice API returns 0 on fail and 1 on
success. Either TST_ERR or errno is used to store the error
number. The commit stays with this scheme except that we only use
errno. This means that we have to temporarily save errno when it would
be overwritten by a less important operation.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

Possibly the API should be redesigned so that we are not juggling
errno. We could return the errno instead of just a boolean. However I
then think this should be done for all network functions for
consistency. Even if that is done, I still don't like it.

In most cases though, the issue is caused by tst_brk_ not
returning. Instead we could print the error immediately, but defer
cleanup. E.g. have tst_brk_defer_ which prints the error and sets a
flag, then call tst_sync_ which exits if the brk flag is set.

In any case these are quite big changes. So I have just submitted this
errno patch.

 lib/tst_netdevice.c | 30 ++++++++++++++++++++----------
 lib/tst_rtnetlink.c |  9 +++++----
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/lib/tst_netdevice.c b/lib/tst_netdevice.c
index d098173d5..214a6f500 100644
--- a/lib/tst_netdevice.c
+++ b/lib/tst_netdevice.c
@@ -101,7 +101,7 @@ int tst_netdev_set_state(const char *file, const int lineno,
 int tst_create_veth_pair(const char *file, const int lineno,
 	const char *ifname1, const char *ifname2)
 {
-	int ret;
+	int ret, save_errno;
 	struct ifinfomsg info = { .ifi_family = AF_UNSPEC };
 	struct tst_rtnl_context *ctx;
 	struct tst_rtnl_attr_list peerinfo[] = {
@@ -146,10 +146,12 @@ int tst_create_veth_pair(const char *file, const int lineno,
 	}
 
 	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	save_errno = errno;
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		errno = save_errno;
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to create veth interfaces %s+%s", ifname1,
 			ifname2);
 	}
@@ -161,7 +163,7 @@ int tst_remove_netdev(const char *file, const int lineno, const char *ifname)
 {
 	struct ifinfomsg info = { .ifi_family = AF_UNSPEC };
 	struct tst_rtnl_context *ctx;
-	int ret;
+	int ret, save_errno;
 
 	if (strlen(ifname) >= IFNAMSIZ) {
 		tst_brk_(file, lineno, TBROK,
@@ -180,10 +182,12 @@ int tst_remove_netdev(const char *file, const int lineno, const char *ifname)
 	}
 
 	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	save_errno = errno;
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		errno = save_errno;
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to remove netdevice %s", ifname);
 	}
 
@@ -196,7 +200,7 @@ static int modify_address(const char *file, const int lineno,
 	size_t addrlen, uint32_t addr_flags)
 {
 	struct tst_rtnl_context *ctx;
-	int index, ret;
+	int index, ret, save_errno;
 	struct ifaddrmsg info = {
 		.ifa_family = family,
 		.ifa_prefixlen = prefix
@@ -229,10 +233,12 @@ static int modify_address(const char *file, const int lineno,
 	}
 
 	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	save_errno = errno;
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		errno = save_errno;
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to modify %s network address", ifname);
 	}
 
@@ -276,7 +282,7 @@ static int change_ns(const char *file, const int lineno, const char *ifname,
 {
 	struct ifinfomsg info = { .ifi_family = AF_UNSPEC };
 	struct tst_rtnl_context *ctx;
-	int ret;
+	int ret, save_errno;
 
 	if (strlen(ifname) >= IFNAMSIZ) {
 		tst_brk_(file, lineno, TBROK,
@@ -298,10 +304,12 @@ static int change_ns(const char *file, const int lineno, const char *ifname,
 	}
 
 	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	save_errno = errno;
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		errno = save_errno;
+		tst_brk_(file, lineno, TBROK | TERRNO,
 			"Failed to move %s to another namespace", ifname);
 	}
 
@@ -327,7 +335,7 @@ static int modify_route(const char *file, const int lineno, unsigned int action,
 	const void *gateway, size_t gatewaylen)
 {
 	struct tst_rtnl_context *ctx;
-	int ret;
+	int ret, save_errno;
 	int32_t index;
 	struct rtmsg info = {
 		.rtm_family = family,
@@ -389,10 +397,12 @@ static int modify_route(const char *file, const int lineno, unsigned int action,
 	}
 
 	ret = tst_rtnl_send_validate(file, lineno, ctx);
+	save_errno = errno;
 	tst_rtnl_destroy_context(file, lineno, ctx);
 
 	if (!ret) {
-		tst_brk_(file, lineno, TBROK | TTERRNO,
+		errno = save_errno;
+		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..627695c80 100644
--- a/lib/tst_rtnetlink.c
+++ b/lib/tst_rtnetlink.c
@@ -376,11 +376,12 @@ int tst_rtnl_check_acks(const char *file, const int lineno,
 			tst_brk_(file, lineno, TBROK,
 				"No ACK found for Netlink message %u",
 				msg->nlmsg_seq);
+			errno = ENOMSG;
 			return 0;
 		}
 
 		if (res->err->error) {
-			TST_ERR = -res->err->error;
+			errno = -res->err->error;
 			return 0;
 		}
 	}
@@ -392,9 +393,7 @@ int tst_rtnl_send_validate(const char *file, const int lineno,
 	struct tst_rtnl_context *ctx)
 {
 	struct tst_rtnl_message *response;
-	int ret;
-
-	TST_ERR = 0;
+	int ret, save_errno;
 
 	if (tst_rtnl_send(file, lineno, ctx) <= 0)
 		return 0;
@@ -406,7 +405,9 @@ int tst_rtnl_send_validate(const char *file, const int lineno,
 		return 0;
 
 	ret = tst_rtnl_check_acks(file, lineno, ctx, response);
+	save_errno = errno;
 	tst_rtnl_free_message(response);
+	errno = save_errno;
 
 	return ret;
 }
-- 
2.31.1

[LTP] [PATCH v4 1/3] Add Coccinelle helper scripts for reference

[Cyril Hrubis]

Hi!
Applied, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 2/3] API: Remove TEST macro usage from library

[Cyril Hrubis]

Hi!
Applied thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Cyril Hrubis]

Hi!
> The test author is guaranteed that the library will not set TST_ERR
> except via the TEST macro and similar.
> 
> Currently the rtnetlink & netdevice API returns 0 on fail and 1 on
> success. Either TST_ERR or errno is used to store the error
> number. The commit stays with this scheme except that we only use
> errno. This means that we have to temporarily save errno when it would
> be overwritten by a less important operation.

I do not like this fix. If nothing else it's fragile and would make any
patch review for these libraries much harder.

I would prefer having these functions modified to return the errors
instead even if I have to do the work.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 2/3] API: Remove TEST macro usage from library

[Li Wang]

Hi Richard,


On Tue, Jun 15, 2021 at 3:41 PM Richard Palethorpe via ltp
<ltp@lists.linux.it> wrote:
>
> The test author is guaranteed that the LTP library will not change the
> TST_RET and TST_ERR global variables. That way the test author can use
> the TEST macro then call other library functions before using TST_RET,
> TST_ERR or TTERRNO.

I guess the inline function in the header files also should be
considered, right?

lapi/clone.h:           TEST(syscall(__NR_clone3, NULL, 0));
lapi/fsmount.h:         TEST(syscall(__NR_fsopen, NULL, 0));
lapi/init_module.h:               TEST(syscall(__NR_finit_module, 0, "", 0));
lapi/io_uring.h:                TEST(syscall(__NR_io_uring_setup, NULL, 0));
lapi/name_to_handle_at.h:       TEST(name_to_handle_at(dfd, pathname,
&fh, &mount_id, 0));
lapi/openat2.h:         TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));


--
Regards,
Li Wang

[LTP] [PATCH v4 1/3] Add Coccinelle helper scripts for reference

[Li Wang]

Hi Richard,

> +for spatch_file in $*; do
> +    case $spatch_file in
> +       libltp-test-macro)
> +           libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro.cocci;;
> +       libltp-test-macro-vars)
> +           libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro-vars.cocci \
> +                         --ignore lib/tst_test.c;;
> +       *)
> +           tests_spatch --sp-file scripts/coccinelle/$spatch_file.cocci;;

Why here use the coccinelle/ path and add suffix with the spatch_file?
Wouldn't this below more simple to us:

   scripts/coccinelle/$spatch_file.cocci  ==> $spatch_file

> +    esac
> +done




--
Regards,
Li Wang

[LTP] [PATCH v4 1/3] Add Coccinelle helper scripts for reference

[Li Wang]

Li Wang <liwang@redhat.com> wrote:

> > +for spatch_file in $*; do
> > +    case $spatch_file in
> > +       libltp-test-macro)
> > +           libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro.cocci;;
> > +       libltp-test-macro-vars)
> > +           libltp_spatch --sp-file scripts/coccinelle/libltp-test-macro-vars.cocci \
> > +                         --ignore lib/tst_test.c;;
> > +       *)
> > +           tests_spatch --sp-file scripts/coccinelle/$spatch_file.cocci;;
>
> Why here use the coccinelle/ path and add suffix with the spatch_file?

Ah, it seems to preserve for more self define spatch files, I misread
the usage just now.
Sorry for making noise, I'm definitely a newbie on coccinelle.

-- 
Regards,
Li Wang

[LTP] [PATCH v4 2/3] API: Remove TEST macro usage from library

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
>
> On Tue, Jun 15, 2021 at 3:41 PM Richard Palethorpe via ltp
> <ltp@lists.linux.it> wrote:
>>
>> The test author is guaranteed that the LTP library will not change the
>> TST_RET and TST_ERR global variables. That way the test author can use
>> the TEST macro then call other library functions before using TST_RET,
>> TST_ERR or TTERRNO.
>
> I guess the inline function in the header files also should be
> considered, right?
>
> lapi/clone.h:           TEST(syscall(__NR_clone3, NULL, 0));
> lapi/fsmount.h:         TEST(syscall(__NR_fsopen, NULL, 0));
> lapi/init_module.h:               TEST(syscall(__NR_finit_module, 0, "", 0));
> lapi/io_uring.h:                TEST(syscall(__NR_io_uring_setup, NULL, 0));
> lapi/name_to_handle_at.h:       TEST(name_to_handle_at(dfd, pathname,
> &fh, &mount_id, 0));
> lapi/openat2.h:         TEST(syscall(__NR_openat2, -1, NULL, NULL, 0));

Yes, I guess we need to explicitly scan the headers. Thanks!

-- 
Thank you,
Richard.

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> The test author is guaranteed that the library will not set TST_ERR
>> except via the TEST macro and similar.
>> 
>> Currently the rtnetlink & netdevice API returns 0 on fail and 1 on
>> success. Either TST_ERR or errno is used to store the error
>> number. The commit stays with this scheme except that we only use
>> errno. This means that we have to temporarily save errno when it would
>> be overwritten by a less important operation.
>
> I do not like this fix. If nothing else it's fragile and would make any
> patch review for these libraries much harder.
>
> I would prefer having these functions modified to return the errors
> instead even if I have to do the work.

I think there are some other issues here as well. Like the macros are
not prefixed with SAFE_. A lot of the functions are not used or they are
just used internally, but are not marked as static.

I guess there are more tests on the way that use these
functions. However it is difficult to update the API, if you don't know
how it will be used. I like to have a concrete usecase for what I am
working on, especially if it's not code I have a lot of vision for.

I'd prefer to remove the stuff which is unused before making significant
changes to the library. Also mark internal functions as static and
remove the headers.

Also, it seems the problem functions are actually internal and not used
by the test. If they are marked static, then I don't see any problem
with them being inconsistent with the public API.

This may create more work for Martin, but any sweeping changes made by
myself or Cyril will probably break any WIP tests and need to be
changed.

-- 
Thank you,
Richard.

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Cyril Hrubis]

Hi!
> > The test author is guaranteed that the library will not set TST_ERR
> > except via the TEST macro and similar.
> 
> Hi, who decided to guarantee this and where is the guarantee documented?

That is the whole point of the API, keep TST_RET and TST_ERR until
explicitly changed by either chaning them directly or via the TEST()
macro.

> I was planning to document that RTNL_SEND_VALIDATE() and
> RTNL_CHECK_ACKS() will pass error codes found in rtnetlink ACK messages
> through TST_ERR.
> 
> On 17. 06. 21 10:40, Richard Palethorpe wrote:
> > Hello,
> > 
> > Cyril Hrubis <chrubis@suse.cz> writes:
> >> I do not like this fix. If nothing else it's fragile and would make any
> >> patch review for these libraries much harder.
> >>
> >> I would prefer having these functions modified to return the errors
> >> instead even if I have to do the work.
> 
> Changing the return value to always return errno will be a major PITA
> because 95% of error handling happens after some safe_syscall() which
> clobbers errno by calling tst_brk() after failure. And if you only want
> to return error codes from rtnetlink ACK messages, then there's the
> problem what to return when a safe_syscall() fails during cleanup().

For the current code it would be as easy as:

diff --git a/lib/tst_netdevice.c b/lib/tst_netdevice.c
index d098173d5..b4b10944d 100644
--- a/lib/tst_netdevice.c
+++ b/lib/tst_netdevice.c
@@ -148,10 +148,10 @@ int tst_create_veth_pair(const char *file, const int lineno,
        ret = tst_rtnl_send_validate(file, lineno, ctx);
        tst_rtnl_destroy_context(file, lineno, ctx);

-       if (!ret) {
-               tst_brk_(file, lineno, TBROK | TTERRNO,
-                       "Failed to create veth interfaces %s+%s", ifname1,
-                       ifname2);
+       if (ret) {
+               tst_brk_(file, lineno, TBROK,
+                       "Failed to create veth interfaces %s+%s: %s", ifname1,
+                       ifname2, tst_strerrno(ret));
        }

        return ret;
@@ -182,9 +182,9 @@ int tst_remove_netdev(const char *file, const int lineno, const char *ifname)
        ret = tst_rtnl_send_validate(file, lineno, ctx);
        tst_rtnl_destroy_context(file, lineno, ctx);

-       if (!ret) {
+       if (ret) {
                tst_brk_(file, lineno, TBROK | TTERRNO,
-                       "Failed to remove netdevice %s", ifname);
+                       "Failed to remove netdevice %s: %s", ifname, tst_strerrno(ret));
        }

        return ret;
@@ -231,9 +231,9 @@ static int modify_address(const char *file, const int lineno,
        ret = tst_rtnl_send_validate(file, lineno, ctx);
        tst_rtnl_destroy_context(file, lineno, ctx);

-       if (!ret) {
+       if (ret) {
                tst_brk_(file, lineno, TBROK | TTERRNO,
-                       "Failed to modify %s network address", ifname);
+                       "Failed to modify %s network address: %s", ifname, tst_strerrno(ret));
        }

        return ret;
@@ -300,9 +300,9 @@ static int change_ns(const char *file, const int lineno, const char *ifname,
        ret = tst_rtnl_send_validate(file, lineno, ctx);
        tst_rtnl_destroy_context(file, lineno, ctx);

-       if (!ret) {
+       if (ret) {
                tst_brk_(file, lineno, TBROK | TTERRNO,
-                       "Failed to move %s to another namespace", ifname);
+                       "Failed to move %s to another namespace: %s", ifname, tst_strerrno(ret));
        }

        return ret;
@@ -391,9 +391,9 @@ static int modify_route(const char *file, const int lineno, unsigned int action,
        ret = tst_rtnl_send_validate(file, lineno, ctx);
        tst_rtnl_destroy_context(file, lineno, ctx);

-       if (!ret) {
+       if (ret) {
                tst_brk_(file, lineno, TBROK | TTERRNO,
-                       "Failed to modify network route");
+                       "Failed to modify network route", tst_strerrno(ret));
        }

        return ret;
diff --git a/lib/tst_rtnetlink.c b/lib/tst_rtnetlink.c
index 1ecda3a9f..c5f1aa0dc 100644
--- a/lib/tst_rtnetlink.c
+++ b/lib/tst_rtnetlink.c
@@ -376,16 +376,14 @@ int tst_rtnl_check_acks(const char *file, const int lineno,
                        tst_brk_(file, lineno, TBROK,
                                "No ACK found for Netlink message %u",
                                msg->nlmsg_seq);
-                       return 0;
+                       return EPROTO;
                }

-               if (res->err->error) {
-                       TST_ERR = -res->err->error;
-                       return 0;
-               }
+               if (res->err->error)
+                       return res->err->error;
        }

-       return 1;
+       return 0;
 }

 int tst_rtnl_send_validate(const char *file, const int lineno,
@@ -394,8 +392,6 @@ int tst_rtnl_send_validate(const char *file, const int lineno,
        struct tst_rtnl_message *response;
        int ret;

-       TST_ERR = 0;
-
        if (tst_rtnl_send(file, lineno, ctx) <= 0)
                return 0;

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Martin Doucha]

On 15. 06. 21 9:40, Richard Palethorpe wrote:
> The test author is guaranteed that the library will not set TST_ERR
> except via the TEST macro and similar.

Hi, who decided to guarantee this and where is the guarantee documented?

I was planning to document that RTNL_SEND_VALIDATE() and
RTNL_CHECK_ACKS() will pass error codes found in rtnetlink ACK messages
through TST_ERR.

On 17. 06. 21 10:40, Richard Palethorpe wrote:
> Hello,
> 
> Cyril Hrubis <chrubis@suse.cz> writes:
>> I do not like this fix. If nothing else it's fragile and would make any
>> patch review for these libraries much harder.
>>
>> I would prefer having these functions modified to return the errors
>> instead even if I have to do the work.

Changing the return value to always return errno will be a major PITA
because 95% of error handling happens after some safe_syscall() which
clobbers errno by calling tst_brk() after failure. And if you only want
to return error codes from rtnetlink ACK messages, then there's the
problem what to return when a safe_syscall() fails during cleanup().

> I think there are some other issues here as well. Like the macros are
> not prefixed with SAFE_. A lot of the functions are not used or they are
> just used internally, but are not marked as static.

All the functions which are intended as internal-only are already static
in both tst_netlink.c and tst_netdevice.c. The macros are not prefixed
with SAFE_ because they're not simple syscall wrappers. They're prefixed
with RTNL_ and NETDEV_ instead.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Cyril Hrubis]

Hi!
> >>> The test author is guaranteed that the library will not set TST_ERR
> >>> except via the TEST macro and similar.
> >>
> >> Hi, who decided to guarantee this and where is the guarantee documented?
> 
> Guaranteeing that TST_RET and TST_ERR will not be modified makes sense
> for SAFE_SYSCALL()s because they will be used extensively throughout
> test code. But the case is not so clear for primarily setup()/cleanup()
> functions like the af_alg, rtnetlink and netdevice libraries. And note
> that the rtnetlink library allows you to check ACKs manually without
> calling the two functions which will modify TST_ERR.
> 
> So again, where is the extent of this guarantee documented? I haven't
> found any mention of it in the doc/ dir and Richie didn't add any docs
> changes in his patchset either. Documenting this is necessary for both
> test writers and library maintainers.

I guess that it should be added to
doc/library-api-writing-guidelines.txt but that is orthogonal to the
attempt to fix the the library code itself.

> >> Changing the return value to always return errno will be a major PITA
> >> because 95% of error handling happens after some safe_syscall() which
> >> clobbers errno by calling tst_brk() after failure. And if you only want
> >> to return error codes from rtnetlink ACK messages, then there's the
> >> problem what to return when a safe_syscall() fails during cleanup().
> > 
> > For the current code it would be as easy as:
> 
> That code will only result in RTNL_SEND_VALIDATE() always returning 0
> regardless of success or failure, except when tst_brk() terminates the
> whole program.

Ah right, we have to return non-zero if the tst_rtnl_send() or
tst_rtnl_rect() failed. I would be nice to propagete the errno properly,
which we do not do at the moment, but that looks like a lot of effort so
we may as well hardcode something non-zero there as well.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Martin Doucha]

On 22. 06. 21 15:40, Cyril Hrubis wrote:
> Hi!
>>> The test author is guaranteed that the library will not set TST_ERR
>>> except via the TEST macro and similar.
>>
>> Hi, who decided to guarantee this and where is the guarantee documented?

Guaranteeing that TST_RET and TST_ERR will not be modified makes sense
for SAFE_SYSCALL()s because they will be used extensively throughout
test code. But the case is not so clear for primarily setup()/cleanup()
functions like the af_alg, rtnetlink and netdevice libraries. And note
that the rtnetlink library allows you to check ACKs manually without
calling the two functions which will modify TST_ERR.

So again, where is the extent of this guarantee documented? I haven't
found any mention of it in the doc/ dir and Richie didn't add any docs
changes in his patchset either. Documenting this is necessary for both
test writers and library maintainers.

>> Changing the return value to always return errno will be a major PITA
>> because 95% of error handling happens after some safe_syscall() which
>> clobbers errno by calling tst_brk() after failure. And if you only want
>> to return error codes from rtnetlink ACK messages, then there's the
>> problem what to return when a safe_syscall() fails during cleanup().
> 
> For the current code it would be as easy as:

That code will only result in RTNL_SEND_VALIDATE() always returning 0
regardless of success or failure, except when tst_brk() terminates the
whole program.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH v4 3/3] API: Remove TST_ERR usage from rtnetlink/netdevice

[Richard Palethorpe]

Hello Martin,

Martin Doucha <mdoucha@suse.cz> writes:

> On 22. 06. 21 15:40, Cyril Hrubis wrote:
>> Hi!
>>>> The test author is guaranteed that the library will not set TST_ERR
>>>> except via the TEST macro and similar.
>>>
>>> Hi, who decided to guarantee this and where is the guarantee documented?
>
> Guaranteeing that TST_RET and TST_ERR will not be modified makes sense
> for SAFE_SYSCALL()s because they will be used extensively throughout
> test code. But the case is not so clear for primarily setup()/cleanup()
> functions like the af_alg, rtnetlink and netdevice libraries. And note
> that the rtnetlink library allows you to check ACKs manually without
> calling the two functions which will modify TST_ERR.

Cyril said not to use it (on my CGroups patch set) and when I thought
about it, it made a lot of sense. I don't want to discuss it again here,
we can do that on a doc RFC.

>
> So again, where is the extent of this guarantee documented? I haven't
> found any mention of it in the doc/ dir and Richie didn't add any docs
> changes in his patchset either. Documenting this is necessary for both
> test writers and library maintainers.

Yes, it needs documenting, we should have done that once the discussions
concluded. I think this highlights a process issue. I don't really know
where to put such a rule. I can just arbitrarily put it in the API docs
somewhere, but that is suboptimal IMO.

As well as explanatory API documentation I think we need a simple
machine readable document which lists rules like this (e.g. CSV file
with ID and description columns). Then we can have a process for
deciding on rules like this. Where someone makes an RFC patch to update
the rule.

I also think we need an automated way of enforcing (some of) these rules
and making test authors aware of them. Which is why I'm trying to create
a SA tool specifically for LTP. In the SA error message we can list the
ID of the rule it violates.

I can add this to the Sparse patch set with the TEST macro and
TST_RET,TST_ERR rules.

The problem with documentation is that people do not read it once their
code works. Also it is easy to miss or forget about something like
TST_ERR being set.

>
>>> Changing the return value to always return errno will be a major PITA
>>> because 95% of error handling happens after some safe_syscall() which
>>> clobbers errno by calling tst_brk() after failure. And if you only want
>>> to return error codes from rtnetlink ACK messages, then there's the
>>> problem what to return when a safe_syscall() fails during cleanup().
>> 
>> For the current code it would be as easy as:
>
> That code will only result in RTNL_SEND_VALIDATE() always returning 0
> regardless of success or failure, except when tst_brk() terminates the
> whole program.


-- 
Thank you,
Richard.