[LTP] [PATCH v2 0/8] Sparse based checker and rule proposal

[LTP] [PATCH v2 0/8] Sparse based checker and rule proposal

[Richard Palethorpe]

Hello,

So it turns out that it is quite easy to implement the TST_RET/ERR
check in Sparse. It compiles the code into an IR which is just the
right abstraction for the test. It is also possible to inspect the AST
with Sparse. It appears more difficult to inspect the AST before
macros are expanded.

Sparse is not packaged as a shared library, but has few/no
dependencies and is easy to compile. It seems like it was meant to be
vendored. Including it as a git module seems reasonable to me. Please
see tools/sparse/README.md.

There are still a lot of errors/noise when running 'make check' on the
entire tree. These are mainly caused by old function definitions and
such. They need to be fixed before the tool can be used properly.

Also I have tried to document the rule and created a list of rules. So
this can also be taken as a formal proposal for the rule itself.

Thanks,

V2:
* Automatically download and build sparse.
* Only build sparse if "make check" is run. It is filtered from "make all".
* Move libtsc.h out of the realtime tests dir. Note that checking of metldown.c
  now fails because it uses a GCC builtin sparse does not recognize.

As mentioned above, there are various errors during checking that need
fixing. For the most part these are legit errors (usually old style
function definitions or redefining of symbols). With stuff like
metldown I am tempted to filter it, but OTOH it looks relatively
straight forward to add a builtin to Sparse upstream. I just need time
to do it. First though I would like to get "make check" working on the
library, so we can put that in CI.

Richard Palethorpe (8):
  Add Sparse based checker and TST_RET/ERR check
  Add 'make check' to the build system
  doc: Add rules and recommendations list
  doc: Remind authors and maintainers to run make check
  doc: Document TEST macro and state TST_RET/ERR rule LTP-002
  Reference LTP-002 rule in Cocci scripts
  API: Move libtsc.h from realtime tests include to tst_tsc.h
  API/tst_tsc: Add guards and remove some boilerplate

 .gitmodules                                   |   3 +
 Makefile                                      |   8 +
 doc/c-test-api.txt                            |  47 ++++++
 doc/library-api-writing-guidelines.txt        |  14 ++
 doc/maintainer-patch-review-checklist.txt     |   2 +-
 doc/rules.tsv                                 |   3 +
 doc/test-writing-guidelines.txt               |   6 +
 include/mk/env_post.mk                        |   8 +
 include/mk/generic_leaf_target.inc            |   5 +-
 include/mk/generic_trunk_target.inc           |   7 +-
 include/mk/lib.mk                             |   3 +
 include/mk/module.mk                          |   2 +
 include/mk/rules.mk                           |   9 ++
 include/mk/sparse.mk                          |   9 ++
 include/mk/testcases.mk                       |   1 +
 .../include/libtsc.h => include/tst_tsc.h     |  35 +----
 .../coccinelle/libltp-test-macro-vars.cocci   |   6 +-
 scripts/coccinelle/libltp-test-macro.cocci    |   4 +-
 testcases/cve/Makefile                        |   2 -
 testcases/cve/meltdown.c                      |   2 +-
 testcases/open_posix_testsuite/Makefile       |   4 +
 .../func/async_handler/async_handler_tsc.c    |   3 +-
 .../func/measurement/preempt_timing.c         |   3 +-
 .../realtime/func/measurement/rdtsc-latency.c |   3 +-
 tools/Makefile                                |   2 +
 tools/sparse/.gitignore                       |   1 +
 tools/sparse/Makefile                         |  27 ++++
 tools/sparse/README.md                        |  38 +++++
 tools/sparse/main.c                           | 148 ++++++++++++++++++
 tools/sparse/sparse-src                       |   1 +
 30 files changed, 362 insertions(+), 44 deletions(-)
 create mode 100644 doc/rules.tsv
 create mode 100644 include/mk/sparse.mk
 rename testcases/realtime/include/libtsc.h => include/tst_tsc.h (53%)
 create mode 100644 tools/sparse/.gitignore
 create mode 100644 tools/sparse/Makefile
 create mode 100644 tools/sparse/README.md
 create mode 100644 tools/sparse/main.c
 create mode 160000 tools/sparse/sparse-src

-- 
2.31.1

[LTP] [PATCH v2 1/8] Add Sparse based checker and TST_RET/ERR check

[Richard Palethorpe]

Vendors in Sparse as a git module. Then uses it to check for stores to
TST_RET/ERR within the library.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 .gitmodules             |   3 +
 tools/Makefile          |   2 +
 tools/sparse/.gitignore |   1 +
 tools/sparse/Makefile   |  27 ++++++++
 tools/sparse/README.md  |  38 +++++++++++
 tools/sparse/main.c     | 148 ++++++++++++++++++++++++++++++++++++++++
 tools/sparse/sparse-src |   1 +
 7 files changed, 220 insertions(+)
 create mode 100644 tools/sparse/.gitignore
 create mode 100644 tools/sparse/Makefile
 create mode 100644 tools/sparse/README.md
 create mode 100644 tools/sparse/main.c
 create mode 160000 tools/sparse/sparse-src

diff --git a/.gitmodules b/.gitmodules
index 1c9e9c38a..a3c34af4b 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "testcases/kernel/mce-test"]
 	path = testcases/kernel/mce-test
 	url = git://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/mce-test.git
+[submodule "tools/sparse/sparse-src"]
+	path = tools/sparse/sparse-src
+	url = git://git.kernel.org/pub/scm/devel/sparse/sparse.git
diff --git a/tools/Makefile b/tools/Makefile
index def1c6fa9..adbf4fe70 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -28,4 +28,6 @@ INSTALL_TARGETS		:= *.awk *.pl *.sh html_report_header.txt
 
 INSTALL_DIR		:= bin
 
+FILTER_OUT_DIRS		+= sparse
+
 include $(top_srcdir)/include/mk/generic_trunk_target.mk
diff --git a/tools/sparse/.gitignore b/tools/sparse/.gitignore
new file mode 100644
index 000000000..ba2906d06
--- /dev/null
+++ b/tools/sparse/.gitignore
@@ -0,0 +1 @@
+main
diff --git a/tools/sparse/Makefile b/tools/sparse/Makefile
new file mode 100644
index 000000000..b2e608d59
--- /dev/null
+++ b/tools/sparse/Makefile
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
+
+top_srcdir		?= ../..
+
+include $(top_srcdir)/include/mk/env_pre.mk
+include $(top_srcdir)/include/mk/functions.mk
+
+SPARSE_SRC	?= sparse-src
+
+$(SPARSE_SRC)/Makefile:
+ifeq ($(SPARSE_SRC),sparse-src)
+	git submodule update --init
+else
+	$(error "Can't find $(SPARSE_SRC)/Makefile")
+endif
+
+$(SPARSE_SRC)/libsparse.a: $(SPARSE_SRC)/Makefile
+	$(MAKE) -C $(SPARSE_SRC) libsparse.a
+
+HOST_MAKE_TARGETS	:= main
+MAKE_DEPS		+= $(SPARSE_SRC)/libsparse.a
+HOST_CFLAGS		+= -I$(SPARSE_SRC)
+HOST_LDLIBS		+= $(SPARSE_SRC)/libsparse.a
+
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/tools/sparse/README.md b/tools/sparse/README.md
new file mode 100644
index 000000000..7c605d5ce
--- /dev/null
+++ b/tools/sparse/README.md
@@ -0,0 +1,38 @@
+# Sparse based linting
+
+This tool checks LTP test and library code for common problems.
+
+## Usage
+
+It is integrated with the LTP build system. Just run `make check` or
+`make check-a_test01`, where `a_test01` is an arbitrary test
+executable or object file.
+
+## Building
+
+The bad news is you must get and build Sparse[^1]. The good news is
+that this only takes a minute and the build system does it for
+you. Just try running `make check` as described above.
+
+However if you want to reuse an existing Sparse checkout. Then you can
+do the following. Where `$SRC_PATH` is the path to the Sparse
+directory.
+
+```sh
+$ cd tools/sparse
+$ make SPARSE_SRC=$SRC_PATH
+```
+You can also manually fetch it via the git submodule
+
+```sh
+$ cd tools/sparse
+$ git submodule update --init
+```
+
+### Clang
+
+Note that while it is possible to build Sparse with Clang. This may
+cause some issues. Namely `GCC_BASE` is set to the Clang resource
+directory. This contains some headers Sparse can not parse.
+
+[1]: Many distributions have a Sparse package. This only contains some executables. There is no shared library
diff --git a/tools/sparse/main.c b/tools/sparse/main.c
new file mode 100644
index 000000000..58f9a549c
--- /dev/null
+++ b/tools/sparse/main.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 SUSE LLC <rpalethorpe@suse.com>
+ *
+ * Sparse allows us to perform checks on the AST (struct symbol) or on
+ * a linearized representation. In the latter case we are given a set
+ * of entry points (functions) containing basic blocks of
+ * instructions.
+ *
+ * The basic blocks contain byte code in SSA form. This is similar to
+ * the the intermediate representation most compilers use during
+ * optimisation.
+ */
+#include <stdarg.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <ctype.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#include "lib.h"
+#include "allocate.h"
+#include "opcode.h"
+#include "token.h"
+#include "parse.h"
+#include "symbol.h"
+#include "expression.h"
+#include "linearize.h"
+
+/* The rules for test, library and tool code are different */
+enum ltp_tu_kind {
+	LTP_LIB,
+	LTP_OTHER,
+};
+
+static enum ltp_tu_kind tu_kind = LTP_OTHER;
+
+/* Check for LTP-002
+ *
+ * Inspects the destination symbol of each store instruction. If it is
+ * TST_RET or TST_ERR then emit a warning.
+ */
+static void check_lib_sets_TEST_vars(const struct instruction *insn)
+{
+	static struct ident *TST_RES_id, *TST_ERR_id;
+
+	if (!TST_RES_id) {
+		TST_RES_id = built_in_ident("TST_RET");
+		TST_ERR_id = built_in_ident("TST_ERR");
+	}
+
+	if (insn->opcode != OP_STORE)
+		return;
+	if (insn->src->ident != TST_RES_id &&
+	    insn->src->ident != TST_ERR_id)
+		return;
+
+	warning(insn->pos,
+		"LTP-002: Library should not write to TST_RET or TST_ERR");
+}
+
+static void do_basicblock_checks(struct basic_block *bb)
+{
+	struct instruction *insn;
+
+	FOR_EACH_PTR(bb->insns, insn) {
+		if (!bb_reachable(insn->bb))
+			continue;
+
+		if (tu_kind == LTP_LIB)
+			check_lib_sets_TEST_vars(insn);
+	} END_FOR_EACH_PTR(insn);
+}
+
+static void do_entrypoint_checks(struct entrypoint *ep)
+{
+	struct basic_block *bb;
+
+	FOR_EACH_PTR(ep->bbs, bb) {
+		do_basicblock_checks(bb);
+	} END_FOR_EACH_PTR(bb);
+}
+
+/* Compile the AST into a graph of basicblocks */
+static void process_symbols(struct symbol_list *list)
+{
+	struct symbol *sym;
+
+	FOR_EACH_PTR(list, sym) {
+		struct entrypoint *ep;
+
+		expand_symbol(sym);
+		ep = linearize_symbol(sym);
+		if (!ep || !ep->entry)
+			continue;
+
+		do_entrypoint_checks(ep);
+
+		if (dbg_entry)
+			show_entry(ep);
+	} END_FOR_EACH_PTR(sym);
+}
+
+static void collect_info_from_args(const int argc, char *const *const argv)
+{
+	int i;
+
+	for (i = 0; i < argc; i++) {
+		if (!strcmp("-DLTPLIB", argv[i])) {
+			tu_kind = LTP_LIB;
+		}
+	}
+}
+
+int main(int argc, char **argv)
+{
+	struct string_list *filelist = NULL;
+	char *file;
+
+	Waddress_space = 0;
+	Wbitwise = 0;
+	Wcast_truncate = 0;
+	Wcontext = 0;
+	Wdecl = 0;
+	Wexternal_function_has_definition = 0;
+	Wflexible_array_array = 0;
+	Wimplicit_int = 0;
+	Wint_to_pointer_cast = 0;
+	Wmemcpy_max_count = 0;
+	Wnon_pointer_null = 0;
+	Wone_bit_signed_bitfield = 0;
+	Woverride_init = 0;
+	Wpointer_to_int_cast = 0;
+	Wvla = 0;
+
+	do_output = 0;
+
+	collect_info_from_args(argc, argv);
+
+	process_symbols(sparse_initialize(argc, argv, &filelist));
+	FOR_EACH_PTR(filelist, file) {
+		process_symbols(sparse(file));
+	} END_FOR_EACH_PTR(file);
+
+	report_stats();
+	return 0;
+}
diff --git a/tools/sparse/sparse-src b/tools/sparse/sparse-src
new file mode 160000
index 000000000..8af243292
--- /dev/null
+++ b/tools/sparse/sparse-src
@@ -0,0 +1 @@
+Subproject commit 8af2432923486c753ab52cae70b94ee684121080
-- 
2.31.1

[LTP] [PATCH v2 2/8] Add 'make check' to the build system

[Richard Palethorpe]

Allows one to run 'make check' or 'make check-$TARGET'. Which will
execute the CHECK tool with that target's CC arguments and
CHECK_FLAGS. By default the check tool is tools/sparse/main.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 Makefile                                | 8 ++++++++
 include/mk/env_post.mk                  | 8 ++++++++
 include/mk/generic_leaf_target.inc      | 5 ++++-
 include/mk/generic_trunk_target.inc     | 7 ++++++-
 include/mk/lib.mk                       | 3 +++
 include/mk/module.mk                    | 2 ++
 include/mk/rules.mk                     | 9 +++++++++
 include/mk/sparse.mk                    | 9 +++++++++
 include/mk/testcases.mk                 | 1 +
 testcases/open_posix_testsuite/Makefile | 4 ++++
 10 files changed, 54 insertions(+), 2 deletions(-)
 create mode 100644 include/mk/sparse.mk

diff --git a/Makefile b/Makefile
index 56812d77b..3b0ba330d 100644
--- a/Makefile
+++ b/Makefile
@@ -79,6 +79,7 @@ BOOTSTRAP_TARGETS	:= $(sort $(COMMON_TARGETS) $(CLEAN_TARGETS) $(INSTALL_TARGETS
 CLEAN_TARGETS		:= $(addsuffix -clean,$(CLEAN_TARGETS))
 INSTALL_TARGETS		:= $(addsuffix -install,$(INSTALL_TARGETS))
 MAKE_TARGETS		:= $(addsuffix -all,$(filter-out lib,$(COMMON_TARGETS)))
+CHECK_TARGETS		:= $(addsuffix -check,testcases lib)
 
 # There's no reason why we should run `all' twice. Otherwise we're just wasting
 # 3+ mins of useful CPU cycles on a modern machine, and even more time on an
@@ -108,6 +109,10 @@ $(MAKE_TARGETS) include-all lib-all libs-all:
 	$(MAKE) -C "$(subst -all,,$@)" \
 		-f "$(abs_top_srcdir)/$(subst -all,,$@)/Makefile" all
 
+$(CHECK_TARGETS): tools-all
+	$(MAKE) -C "$(subst -check,,$@)" \
+		-f "$(abs_top_srcdir)/$(subst -check,,$@)/Makefile" check
+
 # Let's not conflict with ac-clean, maintainer-clean, etc, so.
 $(filter-out include-clean,$(CLEAN_TARGETS))::
 	-$(MAKE) -C "$(subst -clean,,$@)" \
@@ -189,6 +194,9 @@ INSTALL_TARGETS		+= $(addprefix $(DESTDIR)/$(bindir)/,$(BINDIR_INSTALL_SCRIPTS))
 
 $(INSTALL_TARGETS): $(INSTALL_DIR) $(DESTDIR)/$(bindir)
 
+.PHONY: check
+check: $(CHECK_TARGETS)
+
 ## Install
 install: $(INSTALL_TARGETS)
 
diff --git a/include/mk/env_post.mk b/include/mk/env_post.mk
index 1d22f9c53..7f4948037 100644
--- a/include/mk/env_post.mk
+++ b/include/mk/env_post.mk
@@ -89,6 +89,14 @@ $(error You must define $$(prefix) before executing install)
 endif # END $(filter-out install,$(MAKECMDGOALS)),$(MAKECMDGOALS)
 endif
 
+CHECK_TARGETS			?= $(addprefix check-,$(notdir $(patsubst %.c,%,$(sort $(wildcard $(abs_srcdir)/*.c)))))
+CHECK_TARGETS			:= $(filter-out $(addprefix check-, $(FILTER_OUT_MAKE_TARGETS)), $(CHECK_TARGETS))
+CHECK				?= $(abs_top_srcdir)/tools/sparse/main
+
+ifeq ($(CHECK),$(abs_top_srcdir)/tools/sparse/main)
+CHECK_DEPS			+= $(CHECK)
+endif
+
 include $(top_srcdir)/include/mk/rules.mk
 
 endif
diff --git a/include/mk/generic_leaf_target.inc b/include/mk/generic_leaf_target.inc
index 64953f89a..aa092a5a3 100644
--- a/include/mk/generic_leaf_target.inc
+++ b/include/mk/generic_leaf_target.inc
@@ -92,7 +92,7 @@
 # INSTALL_DIR			:= $(libdir)
 #
 
-.PHONY: all clean install
+.PHONY: all clean install check
 
 ifneq ($(strip $(MAKE_TARGETS)),)
 $(MAKE_TARGETS) += $(HOST_MAKE_TARGETS)
@@ -109,4 +109,7 @@ $(INSTALL_FILES): | $(INSTALL_DEPS)
 
 install: $(INSTALL_FILES)
 
+$(CHECK_TARGETS): | $(CHECK_DEPS)
+check: $(CHECK_TARGETS)
+
 # vim: syntax=make
diff --git a/include/mk/generic_trunk_target.inc b/include/mk/generic_trunk_target.inc
index fc59f944f..32a108fbf 100644
--- a/include/mk/generic_trunk_target.inc
+++ b/include/mk/generic_trunk_target.inc
@@ -48,7 +48,7 @@
 
 include $(top_srcdir)/include/mk/functions.mk
 
-RECURSIVE_TARGETS		?= all install
+RECURSIVE_TARGETS		?= all install check
 
 $(eval $(get_make_dirs))
 
@@ -68,6 +68,9 @@ $(INSTALL_FILES): | $(INSTALL_DEPS)
 
 trunk-install: $(INSTALL_FILES)
 
+$(CHECK_TARGETS): | $(CHECK_DEPS)
+trunk-check: $(CHECK_TARGETS)
+
 # Avoid creating duplicate .PHONY references to all, clean, and install. IIRC,
 # I've seen some indeterministic behavior when one does this in the past with
 # GNU Make...
@@ -108,4 +111,6 @@ else
 endif
 endif
 
+check: trunk-check
+
 # vim: syntax=make
diff --git a/include/mk/lib.mk b/include/mk/lib.mk
index f9b6c0aff..3bf63bf9e 100644
--- a/include/mk/lib.mk
+++ b/include/mk/lib.mk
@@ -26,6 +26,7 @@
 # Makefile to include for libraries.
 
 include $(top_srcdir)/include/mk/env_pre.mk
+include $(top_srcdir)/include/mk/sparse.mk
 
 INSTALL_DIR	:= $(libdir)
 
@@ -57,6 +58,8 @@ LIBSRCS		:= $(filter-out $(FILTER_OUT_LIBSRCS),$(LIBSRCS))
 
 LIBOBJS		:= $(LIBSRCS:.c=.o)
 
+CHECK_TARGETS	:= $(addprefix check-,$(notdir $(LIBSRCS:.c=)))
+
 $(LIB): $(notdir $(LIBOBJS))
 	@if [ -z "$(strip $^)" ] ; then \
 		echo "Cowardly refusing to create empty archive"; \
diff --git a/include/mk/module.mk b/include/mk/module.mk
index 6c8814b96..3bb7350f1 100644
--- a/include/mk/module.mk
+++ b/include/mk/module.mk
@@ -47,6 +47,8 @@ endif
 
 CLEAN_TARGETS += .dep_modules *.mod built-in.a
 
+CHECK_TARGETS := $(filter-out %.ko, $(CHECK_TARGETS))
+
 MODULE_SOURCES := $(patsubst %.ko,%.c,$(filter %.ko, $(MAKE_TARGETS)))
 
 # Ignoring the exit status of commands is done to be forward compatible with
diff --git a/include/mk/rules.mk b/include/mk/rules.mk
index c8f4bbbbe..2a04b2b67 100644
--- a/include/mk/rules.mk
+++ b/include/mk/rules.mk
@@ -37,3 +37,12 @@ else
 	@$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $^ $(LTPLDLIBS) $(LDLIBS) -o $@
 	@echo CC $(target_rel_dir)$@
 endif
+
+.PHONY: $(CHECK_TARGETS)
+$(CHECK_TARGETS): check-%: %.c
+ifdef VERBOSE
+	$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $<
+else
+	@$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $<
+	@echo CHECK $(target_rel_dir)$<
+endif
diff --git a/include/mk/sparse.mk b/include/mk/sparse.mk
new file mode 100644
index 000000000..66711d107
--- /dev/null
+++ b/include/mk/sparse.mk
@@ -0,0 +1,9 @@
+# Rules to make sparse tool(s) for inclusion in lib and testcases Makefiles
+
+SPARSE_DIR:= $(abs_top_builddir)/tools/sparse
+
+$(SPARSE_DIR)/main: $(SPARSE_DIR)
+	$(MAKE) -C "$^" all
+
+$(SPARSE_DIR): %:
+	mkdir -p "$@"
diff --git a/include/mk/testcases.mk b/include/mk/testcases.mk
index 1c81773d0..444020f16 100644
--- a/include/mk/testcases.mk
+++ b/include/mk/testcases.mk
@@ -22,6 +22,7 @@
 
 include $(top_srcdir)/include/mk/env_pre.mk
 include $(top_srcdir)/include/mk/functions.mk
+include $(top_srcdir)/include/mk/sparse.mk
 
 APICMDS_DIR	:= $(abs_top_builddir)/tools/apicmds
 
diff --git a/testcases/open_posix_testsuite/Makefile b/testcases/open_posix_testsuite/Makefile
index 205ecdc00..fea6db14b 100644
--- a/testcases/open_posix_testsuite/Makefile
+++ b/testcases/open_posix_testsuite/Makefile
@@ -104,3 +104,7 @@ $(CRITICAL_MAKEFILE): \
 	$(top_srcdir)/LDFLAGS			\
 	$(top_srcdir)/LDLIBS
 	@$(MAKE) generate-makefiles
+
+.PHONY: check
+check:
+	@echo "Checker not yet supported by Open POSIX testsuite"
-- 
2.31.1

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Richard Palethorpe]

Create a list of rules which are feasible to check automatically. The
file is a table of tab separated values. It's intended to be machine
readable.

For now there is just an ID column, with IDs that look similar to CWE
IDs or CVEs. For now these can just be used with 'git grep'. Also
there is a description column.

Clearly this is not an exhaustive list. It just contains a library
rule already stated in the guide and the issue Sparse checks for.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 doc/rules.tsv                   | 3 +++
 doc/test-writing-guidelines.txt | 5 +++++
 2 files changed, 8 insertions(+)
 create mode 100644 doc/rules.tsv

diff --git a/doc/rules.tsv b/doc/rules.tsv
new file mode 100644
index 000000000..d4081ce0f
--- /dev/null
+++ b/doc/rules.tsv
@@ -0,0 +1,3 @@
+ID	DESCRIPTION
+LTP-001	Library source files have tst_ prefix
+LTP-002	TST_RET and TST_ERR are never modified by test library functions
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index d57e52655..74e8ad7ee 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -10,6 +10,11 @@ NOTE: See also
       https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
       https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].
 
+Rules and recommendations which are "machine checkable" should be
+tagged with an ID like +LTP-XXX+. There will be a corresponding entry
+in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
+will display these IDs as a reference.
+
 1. Guide to clean and understandable code
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.31.1

[LTP] [PATCH v2 4/8] doc: Remind authors and maintainers to run make check

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 doc/maintainer-patch-review-checklist.txt | 2 +-
 doc/test-writing-guidelines.txt           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
index f6682b574..970779acc 100644
--- a/doc/maintainer-patch-review-checklist.txt
+++ b/doc/maintainer-patch-review-checklist.txt
@@ -39,7 +39,7 @@ New test should
 * Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
 * Test binaries are added into corresponding '.gitignore' files
 * Check coding style with
-  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
+  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl] and `make check`
   (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
 * Docparse documentation
 * If a test is a regression test it should include tags
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 74e8ad7ee..e3268852a 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -308,6 +308,7 @@ NOTE: See also
 3. The runtest entires are in place
 4. Test binaries are added into corresponding '.gitignore' files
 5. Patches apply over the latest git
+6. 'make check' does not emit any warnings
 
 6.1 About .gitignore files
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.31.1

[LTP] [PATCH v2 5/8] doc: Document TEST macro and state TST_RET/ERR rule LTP-002

[Richard Palethorpe]

There are cases where these variables can be safely used by the
library. However it is a difficult problem to identify these cases
automatically.

Identifying any library code which uses them is relatively easy. In
fact it is easier to automate it than by code review. Because it is
such a boring thing to repeatedly look for and comment on.

If a test library function needs these variables it can recreate its
own private copies.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 doc/c-test-api.txt                     | 47 ++++++++++++++++++++++++++
 doc/library-api-writing-guidelines.txt | 14 ++++++++
 2 files changed, 61 insertions(+)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 45784195b..b47728f60 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -767,6 +767,53 @@ LTP_ALIGN(x, a)
 
 Aligns the x to be next multiple of a. The a must be power of 2.
 
+[source,c]
+-------------------------------------------------------------------------------
+TEST(socket(AF_INET, SOCK_RAW, 1));
+if (TST_RET > -1) {
+	tst_res(TFAIL, "Created raw socket");
+	SAFE_CLOSE(TST_RET);
+} else if (TST_ERR != EPERM) {
+	tst_res(TFAIL | TTERRNO,
+		"Failed to create socket for wrong reason");
+} else {
+	tst_res(TPASS | TTERRNO, "Didn't create raw socket");
+}
+-------------------------------------------------------------------------------
+
+The +TEST+ macro sets +TST_RET+ to its argument's return value and
++TST_ERR+ to +errno+. The +TTERNO+ flag can be used to print the error
+number's symbolic value.
+
+No LTP library function or macro, except those in 'tst_test_macros.h',
+will write to these variables (rule 'LTP-002'). So their values will
+not be changed unexpectedly.
+
+[source,c]
+-------------------------------------------------------------------------------
+TST_EXP_POSITIVE(wait(&status));
+
+if (!TST_PASS)
+	return;
+-------------------------------------------------------------------------------
+
+If the return value of 'wait' is positive. This macro will print a
+pass result and set +TST_PASS+ appropriately. If the return value is
+zero or negative, then it will print fail.
+
+This and similar macros take optional variadic arguments. These begin
+with a format string and then appropriate values to be formatted.
+
+[source,c]
+-------------------------------------------------------------------------------
+TST_EXP_PASS(chown("a/file", uid, gid), "chown(%s,%d,%d)",
+	     "a/file", uid, gid);
+-------------------------------------------------------------------------------
+
+Expects +chown+ to return 0 and emits a pass or a fail. The arguments
+to +chown+ will be printed in either case. There are many similar
+macros, please see 'tst_test_macros.h'.
+
 1.13 Filesystem type detection and skiplist
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/doc/library-api-writing-guidelines.txt b/doc/library-api-writing-guidelines.txt
index a4393fc54..2819d4177 100644
--- a/doc/library-api-writing-guidelines.txt
+++ b/doc/library-api-writing-guidelines.txt
@@ -21,10 +21,24 @@ Don't forget to update docs when you change the API.
 2. C API
 --------
 
+2.1 LTP-001: Sources have tst_ prefix
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 API source code is in headers `include/*.h`, `include/lapi/*.h` (backward
 compatibility for old kernel and libc) and C sources in `lib/*.c`. Files have
 'tst_' prefix.
 
+2.2 LTP-002: TST_RET and TST_ERR are not modified
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The test author is guaranteed that the test API will not modify these
+variables. This prevents silent errors where the return value and
+errno are overwritten before the test has chance to check them.
+
+The macros which are clearly intended to update these variables. That
+is +TEST+ and those in 'tst_test_macros.h'. Are of course allowed to
+update these variables.
+
 3. Shell API
 ------------
 
-- 
2.31.1

[LTP] [PATCH v2 6/8] Reference LTP-002 rule in Cocci scripts

[Richard Palethorpe]

Avoid duplication of rule description

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 scripts/coccinelle/libltp-test-macro-vars.cocci | 6 +-----
 scripts/coccinelle/libltp-test-macro.cocci      | 4 +---
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/scripts/coccinelle/libltp-test-macro-vars.cocci b/scripts/coccinelle/libltp-test-macro-vars.cocci
index ed5459a48..e0fe4e2a7 100644
--- a/scripts/coccinelle/libltp-test-macro-vars.cocci
+++ b/scripts/coccinelle/libltp-test-macro-vars.cocci
@@ -1,11 +1,7 @@
 // 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 violations of LTP-002
 @ find_use exists @
 expression E;
 @@
diff --git a/scripts/coccinelle/libltp-test-macro.cocci b/scripts/coccinelle/libltp-test-macro.cocci
index 7563d23aa..86c55c6c4 100644
--- a/scripts/coccinelle/libltp-test-macro.cocci
+++ b/scripts/coccinelle/libltp-test-macro.cocci
@@ -1,9 +1,7 @@
 // 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 and fix violations of rule LTP-002
 
 // Set with -D fix
 virtual fix
-- 
2.31.1

[LTP] [PATCH v2 7/8] API: Move libtsc.h from realtime tests include to tst_tsc.h

[Richard Palethorpe]

Allow the meltdown test to use this file without modifying the
CFLAGS. This avoids having to add the include also to the CHECK_FLAGS.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/realtime/include/libtsc.h => include/tst_tsc.h  | 0
 testcases/cve/Makefile                                    | 2 --
 testcases/cve/meltdown.c                                  | 2 +-
 testcases/realtime/func/async_handler/async_handler_tsc.c | 3 ++-
 testcases/realtime/func/measurement/preempt_timing.c      | 3 ++-
 testcases/realtime/func/measurement/rdtsc-latency.c       | 3 ++-
 6 files changed, 7 insertions(+), 6 deletions(-)
 rename testcases/realtime/include/libtsc.h => include/tst_tsc.h (100%)

diff --git a/testcases/realtime/include/libtsc.h b/include/tst_tsc.h
similarity index 100%
rename from testcases/realtime/include/libtsc.h
rename to include/tst_tsc.h
diff --git a/testcases/cve/Makefile b/testcases/cve/Makefile
index ddf8b6fe1..c5308794d 100644
--- a/testcases/cve/Makefile
+++ b/testcases/cve/Makefile
@@ -18,8 +18,6 @@ ifneq ($(ANDROID),1)
 cve-2014-0196:  LDLIBS += -lutil
 endif
 
-meltdown: CFLAGS += -I$(abs_srcdir)/../realtime/include
-
 ifneq (,$(filter $(HOST_CPU),x86 x86_64))
 meltdown: CFLAGS += -msse2
 endif
diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
index a387b3205..5a984aba3 100644
--- a/testcases/cve/meltdown.c
+++ b/testcases/cve/meltdown.c
@@ -19,7 +19,7 @@
 
 #include <emmintrin.h>
 
-#include "libtsc.h"
+#include "tst_tsc.h"
 
 #define TARGET_OFFSET	9
 #define TARGET_SIZE	(1 << TARGET_OFFSET)
diff --git a/testcases/realtime/func/async_handler/async_handler_tsc.c b/testcases/realtime/func/async_handler/async_handler_tsc.c
index 7da4324a5..73d4ee5c6 100644
--- a/testcases/realtime/func/async_handler/async_handler_tsc.c
+++ b/testcases/realtime/func/async_handler/async_handler_tsc.c
@@ -46,7 +46,8 @@
 #include <pthread.h>
 #include <librttest.h>
 #include <libstats.h>
-#include <libtsc.h>
+
+#include "tst_tsc.h"
 
 #define HANDLER_PRIO 98
 #define SIGNAL_PRIO 99
diff --git a/testcases/realtime/func/measurement/preempt_timing.c b/testcases/realtime/func/measurement/preempt_timing.c
index 8b5333480..b84d54692 100644
--- a/testcases/realtime/func/measurement/preempt_timing.c
+++ b/testcases/realtime/func/measurement/preempt_timing.c
@@ -52,7 +52,8 @@
 #include <sys/mman.h>
 #include <stdint.h>
 #include <librttest.h>
-#include <libtsc.h>
+
+#include "tst_tsc.h"
 
 #define ITERATIONS 1000000ULL
 #define INTERVALS 10
diff --git a/testcases/realtime/func/measurement/rdtsc-latency.c b/testcases/realtime/func/measurement/rdtsc-latency.c
index d6ab89ff0..3829947bc 100644
--- a/testcases/realtime/func/measurement/rdtsc-latency.c
+++ b/testcases/realtime/func/measurement/rdtsc-latency.c
@@ -44,7 +44,8 @@
 #include <errno.h>
 #include <stdint.h>
 #include <librttest.h>
-#include <libtsc.h>
+
+#include "tst_tsc.h"
 
 #define ITERATIONS 1000000
 
-- 
2.31.1

[LTP] [PATCH v2 8/8] API/tst_tsc: Add guards and remove some boilerplate

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_tsc.h | 35 ++++++++---------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/include/tst_tsc.h b/include/tst_tsc.h
index 9ad5fd659..3f49a6ca7 100644
--- a/include/tst_tsc.h
+++ b/include/tst_tsc.h
@@ -1,28 +1,6 @@
-/******************************************************************************
- *
- *   Copyright ? International Business Machines  Corp., 2006-2008
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- *
- * NAME
- *       libtsc.h
- *
- * DESCRIPTION
- *
- * USAGE:
- *       To be included in some testcases.
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright ? International Business Machines  Corp., 2006-2008
  *
  * AUTHOR
  *        Darren Hart <dvhltc@us.ibm.com>
@@ -30,8 +8,10 @@
  *
  * HISTORY
  *      It directly comes from the librttest.h (see its HISTORY).
- *
- *****************************************************************************/
+ */
+
+#ifndef TST_TSC_H
+#define TST_TSC_H
 
 #undef TSC_UNSUPPORTED
 
@@ -70,3 +50,4 @@
 #define TSC_UNSUPPORTED
 #endif
 
+#endif
-- 
2.31.1

[LTP] [PATCH v2 1/8] Add Sparse based checker and TST_RET/ERR check

[Petr Vorel]

Hi Richie,

> Vendors in Sparse as a git module. Then uses it to check for stores to
> TST_RET/ERR within the library.

I think submodules are a bit problematic, thus I'd prefer avoiding them.
But whole thing is integrated nicely, thus:

Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> diff --git a/tools/sparse/README.md b/tools/sparse/README.md
...
> +# Sparse based linting
> +
> +Note that while it is possible to build Sparse with Clang. This may
> +cause some issues. Namely `GCC_BASE` is set to the Clang resource
> +directory. This contains some headers Sparse can not parse.
> +
> +[1]: Many distributions have a Sparse package. This only contains some executables. There is no shared library
nit: missing dot here.

> diff --git a/tools/sparse/main.c b/tools/sparse/main.c
> new file mode 100644
> index 000000000..58f9a549c
> --- /dev/null
> +++ b/tools/sparse/main.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 SUSE LLC <rpalethorpe@suse.com>
> + *
> + * Sparse allows us to perform checks on the AST (struct symbol) or on
> + * a linearized representation. In the latter case we are given a set
> + * of entry points (functions) containing basic blocks of
> + * instructions.
> + *
> + * The basic blocks contain byte code in SSA form. This is similar to
> + * the the intermediate representation most compilers use during
nit: repeated the

> + * optimisation.
> + */

The rest LGTM.

Kind regards,
Petr

[LTP] [PATCH v2 2/8] Add 'make check' to the build system

[Petr Vorel]

Hi Richie,

> Allows one to run 'make check' or 'make check-$TARGET'. Which will
> execute the CHECK tool with that target's CC arguments and
> CHECK_FLAGS. By default the check tool is tools/sparse/main.

LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Only build sparse if "make check" is run. It is filtered from "make all".
+1

Kind regards,
Petr

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Petr Vorel]

Hi Richie,

obviously correct.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +++ b/doc/test-writing-guidelines.txt
> @@ -10,6 +10,11 @@ NOTE: See also
>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

> +Rules and recommendations which are "machine checkable" should be
> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
nit: I'd make 'doc/rules.tsv' as link (for lazy people like me):
https://github.com/linux-test-project/ltp/tree/master/doc/rules.tsv[doc/rules.tsv]

Kind regards,
Petr

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Petr Vorel]

Hi Richie,

> +++ b/doc/test-writing-guidelines.txt
> @@ -10,6 +10,11 @@ NOTE: See also
>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

> +Rules and recommendations which are "machine checkable" should be
> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
> +will display these IDs as a reference.
> +
Actually text is on the top. I suppose, you probably planned to put this into "2.1 C coding style".

Kind regards,
Petr

>  1. Guide to clean and understandable code
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[LTP] [PATCH v2 4/8] doc: Remind authors and maintainers to run make check

[Petr Vorel]

Hi Richie,

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  doc/maintainer-patch-review-checklist.txt | 2 +-
>  doc/test-writing-guidelines.txt           | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

> diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
> index f6682b574..970779acc 100644
> --- a/doc/maintainer-patch-review-checklist.txt
> +++ b/doc/maintainer-patch-review-checklist.txt
> @@ -39,7 +39,7 @@ New test should
>  * Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
>  * Test binaries are added into corresponding '.gitignore' files
>  * Check coding style with
> -  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl] and `make check`
>    (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
+1

"131" would mean section 1.3.1. If you put it into "2.1 C coding style" the link
should be #21-c-coding-style.

Kind regards,
Petr

>  * Docparse documentation
>  * If a test is a regression test it should include tags
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index 74e8ad7ee..e3268852a 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -308,6 +308,7 @@ NOTE: See also
>  3. The runtest entires are in place
>  4. Test binaries are added into corresponding '.gitignore' files
>  5. Patches apply over the latest git
> +6. 'make check' does not emit any warnings
+1

Kind regards,
Petr

[LTP] [PATCH v2 5/8] doc: Document TEST macro and state TST_RET/ERR rule LTP-002

[Petr Vorel]

Hi Richie,

generally LGTM, with comments below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> There are cases where these variables can be safely used by the
> library. However it is a difficult problem to identify these cases
> automatically.

> Identifying any library code which uses them is relatively easy. In
> fact it is easier to automate it than by code review. Because it is
> such a boring thing to repeatedly look for and comment on.

> If a test library function needs these variables it can recreate its
> own private copies.

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  doc/c-test-api.txt                     | 47 ++++++++++++++++++++++++++
>  doc/library-api-writing-guidelines.txt | 14 ++++++++
>  2 files changed, 61 insertions(+)

> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 45784195b..b47728f60 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -767,6 +767,53 @@ LTP_ALIGN(x, a)

>  Aligns the x to be next multiple of a. The a must be power of 2.

> +[source,c]
> +-------------------------------------------------------------------------------
> +TEST(socket(AF_INET, SOCK_RAW, 1));
> +if (TST_RET > -1) {
> +	tst_res(TFAIL, "Created raw socket");
> +	SAFE_CLOSE(TST_RET);
> +} else if (TST_ERR != EPERM) {
> +	tst_res(TFAIL | TTERRNO,
> +		"Failed to create socket for wrong reason");
> +} else {
> +	tst_res(TPASS | TTERRNO, "Didn't create raw socket");
> +}
> +-------------------------------------------------------------------------------
> +
> +The +TEST+ macro sets +TST_RET+ to its argument's return value and
> ++TST_ERR+ to +errno+. The +TTERNO+ flag can be used to print the error
> +number's symbolic value.
Nice examples, but we already talk about TEST() in "1.2 Basic test interface".
Should we put it there? If not, I'd add links to both places to the other
(separate effort).
Also I suppose whole thing could be simplified with TST_EXP_FAIL2().

> +
> +No LTP library function or macro, except those in 'tst_test_macros.h',
> +will write to these variables (rule 'LTP-002'). So their values will
> +not be changed unexpectedly.
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +TST_EXP_POSITIVE(wait(&status));
> +
> +if (!TST_PASS)
> +	return;
IMHO This is really for "1.2 Basic test interface".

> +-------------------------------------------------------------------------------
> +
> +If the return value of 'wait' is positive. This macro will print a
> +pass result and set +TST_PASS+ appropriately. If the return value is
> +zero or negative, then it will print fail.
> +
> +This and similar macros take optional variadic arguments. These begin
> +with a format string and then appropriate values to be formatted.
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +TST_EXP_PASS(chown("a/file", uid, gid), "chown(%s,%d,%d)",
> +	     "a/file", uid, gid);
> +-------------------------------------------------------------------------------
> +
> +Expects +chown+ to return 0 and emits a pass or a fail. The arguments
> +to +chown+ will be printed in either case. There are many similar
> +macros, please see 'tst_test_macros.h'.
And this as well.

...

> diff --git a/doc/library-api-writing-guidelines.txt b/doc/library-api-writing-guidelines.txt
> index a4393fc54..2819d4177 100644
> --- a/doc/library-api-writing-guidelines.txt
> +++ b/doc/library-api-writing-guidelines.txt
> @@ -21,10 +21,24 @@ Don't forget to update docs when you change the API.
>  2. C API
>  --------

> +2.1 LTP-001: Sources have tst_ prefix
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+1

> +
>  API source code is in headers `include/*.h`, `include/lapi/*.h` (backward
>  compatibility for old kernel and libc) and C sources in `lib/*.c`. Files have
>  'tst_' prefix.

> +2.2 LTP-002: TST_RET and TST_ERR are not modified
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The test author is guaranteed that the test API will not modify these
> +variables. This prevents silent errors where the return value and
> +errno are overwritten before the test has chance to check them.
> +
> +The macros which are clearly intended to update these variables. That
> +is +TEST+ and those in 'tst_test_macros.h'. Are of course allowed to
nit: Maybe +TEST()+ (it's a "function", unlike "variables" TST_RET TST_ERR)?

> +update these variables.

Kind regards,
Petr

[LTP] [PATCH v2 6/8] Reference LTP-002 rule in Cocci scripts

[Petr Vorel]

Hi Riichie,

> Avoid duplication of rule description
obviously correct.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH v2 7/8] API: Move libtsc.h from realtime tests include to tst_tsc.h

[Petr Vorel]

Hi Richie,

> Allow the meltdown test to use this file without modifying the
> CFLAGS. This avoids having to add the include also to the CHECK_FLAGS.
+1

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH v2 1/8] Add Sparse based checker and TST_RET/ERR check

[Joerg Vehlow]

Hi,


On 7/14/2021 9:11 AM, Richard Palethorpe via ltp wrote:
> Vendors in Sparse as a git module. Then uses it to check for stores to
> TST_RET/ERR within the library.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>   .gitmodules             |   3 +
>   tools/Makefile          |   2 +
>   tools/sparse/.gitignore |   1 +
>   tools/sparse/Makefile   |  27 ++++++++
>   tools/sparse/README.md  |  38 +++++++++++
>   tools/sparse/main.c     | 148 ++++++++++++++++++++++++++++++++++++++++
I think the name could be a bit better... sparse_main or ltp_checker or 
something, because a binary just called main could be confusing.

Otherwise good basis for additional checks and fixing the warnings 
emitted by sparse will allow use to raise the overall warning level.
Hopefully at some point at least "-Wall -Werror" is possible.

Joerg

[LTP] [PATCH v2 8/8] API/tst_tsc: Add guards and remove some boilerplate

[Petr Vorel]

Hi Richie,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH v2 0/8] Sparse based checker and rule proposal

[Petr Vorel]

Hi Richie,

thanks for this work! I'm looking forward enabling it in CI after we merge it.

Kind regards,
Petr

[LTP] [PATCH v2 1/8] Add Sparse based checker and TST_RET/ERR check

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi,
>
>
> On 7/14/2021 9:11 AM, Richard Palethorpe via ltp wrote:
>> Vendors in Sparse as a git module. Then uses it to check for stores to
>> TST_RET/ERR within the library.
>>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>   .gitmodules             |   3 +
>>   tools/Makefile          |   2 +
>>   tools/sparse/.gitignore |   1 +
>>   tools/sparse/Makefile   |  27 ++++++++
>>   tools/sparse/README.md  |  38 +++++++++++
>>   tools/sparse/main.c     | 148 ++++++++++++++++++++++++++++++++++++++++
> I think the name could be a bit better... sparse_main or ltp_checker
> or something, because a binary just called main could be confusing.

Sure or maybe sparse-ltp?

That would fit with upstream IMO.

>
> Otherwise good basis for additional checks and fixing the warnings
> emitted by sparse will allow use to raise the overall warning level.
> Hopefully at some point at least "-Wall -Werror" is possible.
>
> Joerg

Yes, I would really like to see -Wall enabled by default. Also Sparse
can maybe help highlight wierd crap only GCC and Clang accept.

-- 
Thank you,
Richard.

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
> obviously correct.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
>> +++ b/doc/test-writing-guidelines.txt
>> @@ -10,6 +10,11 @@ NOTE: See also
>>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
>>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].
>
>> +Rules and recommendations which are "machine checkable" should be
>> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
>> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
> nit: I'd make 'doc/rules.tsv' as link (for lazy people like me):
> https://github.com/linux-test-project/ltp/tree/master/doc/rules.tsv[doc/rules.tsv]

+1

Although I was wondering if it could be formatted and displayed inline somehow?

>
> Kind regards,
> Petr


-- 
Thank you,
Richard.

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
>> +++ b/doc/test-writing-guidelines.txt
>> @@ -10,6 +10,11 @@ NOTE: See also
>>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
>>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].
>
>> +Rules and recommendations which are "machine checkable" should be
>> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
>> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
>> +will display these IDs as a reference.
>> +
> Actually text is on the top. I suppose, you probably planned to put
> this into "2.1 C coding style".

I'm not sure where to put it. The coding style is mostly about syntax
and formatting. The rules file can state anything machine checkable, so
that can include shell, directory structure, what functions are used
etc.

>
> Kind regards,
> Petr
>
>>  1. Guide to clean and understandable code
>>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


-- 
Thank you,
Richard.

[LTP] [PATCH v2 4/8] doc: Remind authors and maintainers to run make check

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>  doc/maintainer-patch-review-checklist.txt | 2 +-
>>  doc/test-writing-guidelines.txt           | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>
>> diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
>> index f6682b574..970779acc 100644
>> --- a/doc/maintainer-patch-review-checklist.txt
>> +++ b/doc/maintainer-patch-review-checklist.txt
>> @@ -39,7 +39,7 @@ New test should
>>  * Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
>>  * Test binaries are added into corresponding '.gitignore' files
>>  * Check coding style with
>> -  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
>> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl] and `make check`
>>    (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
> +1
>
> "131" would mean section 1.3.1. If you put it into "2.1 C coding style" the link
> should be #21-c-coding-style.

You may want to do a quick-fix commit for that, because it is not part
of my patch. Although I can add it ofcourse.

>
> Kind regards,
> Petr
>
>>  * Docparse documentation
>>  * If a test is a regression test it should include tags
>> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
>> index 74e8ad7ee..e3268852a 100644
>> --- a/doc/test-writing-guidelines.txt
>> +++ b/doc/test-writing-guidelines.txt
>> @@ -308,6 +308,7 @@ NOTE: See also
>>  3. The runtest entires are in place
>>  4. Test binaries are added into corresponding '.gitignore' files
>>  5. Patches apply over the latest git
>> +6. 'make check' does not emit any warnings
> +1
>
> Kind regards,
> Petr


-- 
Thank you,
Richard.

[LTP] [PATCH v2 5/8] doc: Document TEST macro and state TST_RET/ERR rule LTP-002

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
> generally LGTM, with comments below.
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
>> There are cases where these variables can be safely used by the
>> library. However it is a difficult problem to identify these cases
>> automatically.
>
>> Identifying any library code which uses them is relatively easy. In
>> fact it is easier to automate it than by code review. Because it is
>> such a boring thing to repeatedly look for and comment on.
>
>> If a test library function needs these variables it can recreate its
>> own private copies.
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>  doc/c-test-api.txt                     | 47 ++++++++++++++++++++++++++
>>  doc/library-api-writing-guidelines.txt | 14 ++++++++
>>  2 files changed, 61 insertions(+)
>
>> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
>> index 45784195b..b47728f60 100644
>> --- a/doc/c-test-api.txt
>> +++ b/doc/c-test-api.txt
>> @@ -767,6 +767,53 @@ LTP_ALIGN(x, a)
>
>>  Aligns the x to be next multiple of a. The a must be power of 2.
>
>> +[source,c]
>> +-------------------------------------------------------------------------------
>> +TEST(socket(AF_INET, SOCK_RAW, 1));
>> +if (TST_RET > -1) {
>> +	tst_res(TFAIL, "Created raw socket");
>> +	SAFE_CLOSE(TST_RET);
>> +} else if (TST_ERR != EPERM) {
>> +	tst_res(TFAIL | TTERRNO,
>> +		"Failed to create socket for wrong reason");
>> +} else {
>> +	tst_res(TPASS | TTERRNO, "Didn't create raw socket");
>> +}
>> +-------------------------------------------------------------------------------
>> +
>> +The +TEST+ macro sets +TST_RET+ to its argument's return value and
>> ++TST_ERR+ to +errno+. The +TTERNO+ flag can be used to print the error
>> +number's symbolic value.
> Nice examples, but we already talk about TEST() in "1.2 Basic test interface".
> Should we put it there? If not, I'd add links to both places to the other
> (separate effort).
> Also I suppose whole thing could be simplified with TST_EXP_FAIL2().

Yes, I suppose that I missed that originally. So it needs unifying and
some wording changes.

>
>> +
>> +No LTP library function or macro, except those in 'tst_test_macros.h',
>> +will write to these variables (rule 'LTP-002'). So their values will
>> +not be changed unexpectedly.
>> +
>> +[source,c]
>> +-------------------------------------------------------------------------------
>> +TST_EXP_POSITIVE(wait(&status));
>> +
>> +if (!TST_PASS)
>> +	return;
> IMHO This is really for "1.2 Basic test interface".
>
>> +-------------------------------------------------------------------------------
>> +
>> +If the return value of 'wait' is positive. This macro will print a
>> +pass result and set +TST_PASS+ appropriately. If the return value is
>> +zero or negative, then it will print fail.
>> +
>> +This and similar macros take optional variadic arguments. These begin
>> +with a format string and then appropriate values to be formatted.
>> +
>> +[source,c]
>> +-------------------------------------------------------------------------------
>> +TST_EXP_PASS(chown("a/file", uid, gid), "chown(%s,%d,%d)",
>> +	     "a/file", uid, gid);
>> +-------------------------------------------------------------------------------
>> +
>> +Expects +chown+ to return 0 and emits a pass or a fail. The arguments
>> +to +chown+ will be printed in either case. There are many similar
>> +macros, please see 'tst_test_macros.h'.
> And this as well.
>
> ...
>
>> diff --git a/doc/library-api-writing-guidelines.txt b/doc/library-api-writing-guidelines.txt
>> index a4393fc54..2819d4177 100644
>> --- a/doc/library-api-writing-guidelines.txt
>> +++ b/doc/library-api-writing-guidelines.txt
>> @@ -21,10 +21,24 @@ Don't forget to update docs when you change the API.
>>  2. C API
>>  --------
>
>> +2.1 LTP-001: Sources have tst_ prefix
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +1
>
>> +
>>  API source code is in headers `include/*.h`, `include/lapi/*.h` (backward
>>  compatibility for old kernel and libc) and C sources in `lib/*.c`. Files have
>>  'tst_' prefix.
>
>> +2.2 LTP-002: TST_RET and TST_ERR are not modified
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +The test author is guaranteed that the test API will not modify these
>> +variables. This prevents silent errors where the return value and
>> +errno are overwritten before the test has chance to check them.
>> +
>> +The macros which are clearly intended to update these variables. That
>> +is +TEST+ and those in 'tst_test_macros.h'. Are of course allowed to
> nit: Maybe +TEST()+ (it's a "function", unlike "variables" TST_RET
> TST_ERR)?

+1

>
>> +update these variables.
>
> Kind regards,
> Petr


-- 
Thank you,
Richard.

[LTP] [PATCH v2 0/8] Sparse based checker and rule proposal

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
> thanks for this work! I'm looking forward enabling it in CI after we
> merge it.

Thanks. Perhaps only in the lib directory for now though. Also it will
only really be checking the checker itself doesn't crash or fail to
build. There are some errors which need fixing in the library before we
can enforce the checks in CI.

>
> Kind regards,
> Petr


-- 
Thank you,
Richard.

[LTP] [PATCH v2 4/8] doc: Remind authors and maintainers to run make check

[Petr Vorel]

> Hello Petr,

> Petr Vorel <pvorel@suse.cz> writes:

> > Hi Richie,

> >> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> >> ---
> >>  doc/maintainer-patch-review-checklist.txt | 2 +-
> >>  doc/test-writing-guidelines.txt           | 1 +
> >>  2 files changed, 2 insertions(+), 1 deletion(-)

> >> diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
> >> index f6682b574..970779acc 100644
> >> --- a/doc/maintainer-patch-review-checklist.txt
> >> +++ b/doc/maintainer-patch-review-checklist.txt
> >> @@ -39,7 +39,7 @@ New test should
> >>  * Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
> >>  * Test binaries are added into corresponding '.gitignore' files
> >>  * Check coding style with
> >> -  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> >> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl] and `make check`
> >>    (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
> > +1

> > "131" would mean section 1.3.1. If you put it into "2.1 C coding style" the link
> > should be #21-c-coding-style.

> You may want to do a quick-fix commit for that, because it is not part
> of my patch. Although I can add it ofcourse.
I'm sorry, I overlooked that it does not have ^+. Thanks for info!

Kind regards,
Petr

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Petr Vorel]

Hi Richie,

> Hello Petr,

> Petr Vorel <pvorel@suse.cz> writes:

> > Hi Richie,

> >> +++ b/doc/test-writing-guidelines.txt
> >> @@ -10,6 +10,11 @@ NOTE: See also
> >>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
> >>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

> >> +Rules and recommendations which are "machine checkable" should be
> >> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
> >> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
> >> +will display these IDs as a reference.
> >> +
> > Actually text is on the top. I suppose, you probably planned to put
> > this into "2.1 C coding style".

> I'm not sure where to put it. The coding style is mostly about syntax
> and formatting. The rules file can state anything machine checkable, so
> that can include shell, directory structure, what functions are used
> etc.

Not sure myself myself. But my note was that you put it below NOTE,
above "1. Guide to clean and understandable code" which looks strange to me.

But not a big deal with it, just a nit.

Kind regards,
Petr

> > Kind regards,
> > Petr

> >>  1. Guide to clean and understandable code
> >>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[LTP] [PATCH v2 1/8] Add Sparse based checker and TST_RET/ERR check

[Cyril Hrubis]

Hi!
Looks good now, if I'm reading it correctly the 'make check' will get
fetch the submodule sources and build everything.

With the main.c file name changed to something reasonable:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/8] doc: Add rules and recommendations list

[Petr Vorel]

Hi Richie,

> Hello Petr,

> Petr Vorel <pvorel@suse.cz> writes:

> > Hi Richie,

> > obviously correct.
> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> >> +++ b/doc/test-writing-guidelines.txt
> >> @@ -10,6 +10,11 @@ NOTE: See also
> >>        https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API],
> >>        https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

> >> +Rules and recommendations which are "machine checkable" should be
> >> +tagged with an ID like +LTP-XXX+. There will be a corresponding entry
> >> +in 'doc/rules.tsv'. When you run 'make check' or 'make check-test' it
> > nit: I'd make 'doc/rules.tsv' as link (for lazy people like me):
> > https://github.com/linux-test-project/ltp/tree/master/doc/rules.tsv[doc/rules.tsv]

> +1

> Although I was wondering if it could be formatted and displayed inline somehow?

I'm not aware of it :(.

Kind regards,
Petr


> > Kind regards,
> > Petr