[PATCH v2 0/6] Test and linter fixups

[PATCH v2 0/6] Test and linter fixups

[David Gibson]

Before starting to convert testcases to use tunbridge, I wanted to add
linting for the Python test scripts.  While doing that I discovered a
new crop of cppcheck and clang-tidy false positives, and a Makefile
bug.

Here's a batch of fixes.

I didn't manage to get through the whole testsuite with this.  I keep
getting hangs on rampstream_out, which I *think* are unrelated.

v2:
 * Actually understood why exetool was being deleted, and fixed it
   properly.

David Gibson (6):
  test: Add linting of Python test scripts
  clang-tidy: Suppress redundant expression warning
  cppcheck: Suppress the suppression of a suppression
  cppcheck: Suppress a buggy cppcheck warning
  cppcheck: Suppress variable scope warnings in dhcpv6()
  test: Don't delete exetool on make clean

 dhcpv6.c                      |  8 ++++++++
 linux_dep.h                   |  2 +-
 tcp.c                         |  5 +++++
 test/Makefile                 | 19 +++++++++++++++----
 test/build/build.py           |  5 +++--
 test/build/static_checkers.sh |  6 +++++-
 vhost_user.c                  |  1 +
 7 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.51.0

[PATCH v2 1/6] test: Add linting of Python test scripts

[David Gibson]

We currently have one test moved to the new exeter based framwork written
in Python.  We plan to add many more, so add linting (flake8) and type
checking (mypy) of those scripts.  This can be invoked manually with
"make flake8" or "make mypy" in test/, and is also added to the static
checkers test set.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/Makefile                 | 14 +++++++++++++-
 test/build/build.py           |  5 +++--
 test/build/static_checkers.sh |  6 +++++-
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/test/Makefile b/test/Makefile
index 49388276..61ccb075 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -8,6 +8,8 @@
 BATS = bats -j $(shell nproc)
 EXETOOL = exeter/exetool/exetool
 WGET = wget -c
+FLAKE8 = flake8
+MYPY = mypy --strict
 
 DEBIAN_IMGS = debian-8.11.0-openstack-amd64.qcow2 \
 	debian-10-nocloud-amd64.qcow2 \
@@ -65,11 +67,15 @@ LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
 ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
 
 EXETER_PYPATH = exeter/py3
+EXETER_PYTHON = build/build.py
 EXETER_BATS = smoke/smoke.sh.bats \
-	build/build.py.bats build/static_checkers.sh.bats
+	$(EXETER_PYTHON:%=%.bats) build/static_checkers.sh.bats
 BATS_FILES = $(EXETER_BATS) \
 	podman/test/system/505-networking-pasta.bats
 
+# Python test code (for linters)
+PYPKGS = $(EXETER_PYTHON)
+
 CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
 
 assets: $(ASSETS)
@@ -128,6 +134,12 @@ medium.bin:
 big.bin:
 	dd if=/dev/urandom bs=1M count=10 of=$@
 
+flake8: pull-exeter
+	PYTHONPATH=$(EXETER_PYPATH) $(FLAKE8) $(PYPKGS)
+
+mypy: pull-exeter
+	PYTHONPATH=$(EXETER_PYPATH) $(MYPY) $(PYPKGS)
+
 $(EXETER_BATS): %.bats: % $(EXETOOL)
 	PYTHONPATH=$(EXETER_PYPATH) $(EXETOOL) bats -- $< > $@
 
diff --git a/test/build/build.py b/test/build/build.py
index e49287c9..e3de8305 100755
--- a/test/build/build.py
+++ b/test/build/build.py
@@ -18,11 +18,12 @@ import os
 from pathlib import Path
 import subprocess
 import tempfile
-from typing import Iterable, Iterator
+from typing import Iterator
 
 import exeter
 
-def sh(cmd):
+
+def sh(cmd: str) -> None:
     """Run given command in a shell"""
     subprocess.run(cmd, shell=True)
 
diff --git a/test/build/static_checkers.sh b/test/build/static_checkers.sh
index 42806e79..228b99ae 100755
--- a/test/build/static_checkers.sh
+++ b/test/build/static_checkers.sh
@@ -21,6 +21,10 @@ exeter_set_description cppcheck "passt sources pass cppcheck"
 exeter_register clang_tidy make -C .. clang-tidy
 exeter_set_description clang_tidy "passt sources pass clang-tidy"
 
-exeter_main "$@"
+exeter_register flake8 make flake8
+exeter_set_description flake8 "passt tests in Python pass flake8"
 
+exeter_register mypy make mypy
+exeter_set_description mypy "passt tests in Python pass mypy --strict"
 
+exeter_main "$@"
-- 
2.51.0

[PATCH v2 2/6] clang-tidy: Suppress redundant expression warning

[David Gibson]

clang-tidy 20.1.8 doesn't like (VHOST_USER_MAX_VQS / 2), because it expands
to (2 / 2).  But in the context of the #define, this makes logical sense,
so suppress the warning.

I'm not sure why it isn't firing on the debug() line just below.  Possibly
it only complains once per expression per function, so we only have to
suppress it once?

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 vhost_user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vhost_user.c b/vhost_user.c
index fa343a86..223332d5 100644
--- a/vhost_user.c
+++ b/vhost_user.c
@@ -939,6 +939,7 @@ static bool vu_get_queue_num_exec(struct vu_dev *vdev,
 {
 	(void)vdev;
 
+	/* NOLINTNEXTLINE(misc-redundant-expression) */
 	vmsg_set_reply_u64(vmsg, VHOST_USER_MAX_VQS / 2);
 
 	debug("VHOST_USER_MAX_VQS  %u", VHOST_USER_MAX_VQS / 2);
-- 
2.51.0

[PATCH v2 3/6] cppcheck: Suppress the suppression of a suppression

[David Gibson]

Apparently cppcheck 2.18.3 no longer trips the funcArgNamesDifferent
warning on our (re-)definition of close_range().  So instead we get an
unmatchedSuppression warning.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 linux_dep.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux_dep.h b/linux_dep.h
index 1d9e1669..89e590c8 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -142,7 +142,7 @@ struct tcp_info_linux {
 #endif
 
 __attribute__ ((weak))
-/* cppcheck-suppress funcArgNamesDifferent */
+/* cppcheck-suppress [funcArgNamesDifferent,unmatchedSuppression] */
 int close_range(unsigned int first, unsigned int last, int flags) {
 	return syscall(SYS_close_range, first, last, flags);
 }
-- 
2.51.0

[PATCH v2 4/6] cppcheck: Suppress a buggy cppcheck warning

[David Gibson]

Another cppcheck package in Fedora, another bogus false positive.  This
one seems to not realise that a variable has been initialised by
getsockopt() under a complicated set of circumstances.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tcp.c b/tcp.c
index 48b1ef29..7da41797 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1030,6 +1030,11 @@ int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn,
 					return 0;
 			}
 
+			/* This trips a cppcheck bug in some versions, including
+			 * cppcheck 2.18.3.
+			 * https://sourceforge.net/p/cppcheck/discussion/general/thread/fecde59085/
+			 */
+			/* cppcheck-suppress [uninitvar,unmatchedSuppression] */
 			conn->seq_ack_to_tap = tinfo->tcpi_bytes_acked +
 				conn->seq_init_from_tap;
 
-- 
2.51.0

[PATCH v2 5/6] cppcheck: Suppress variable scope warnings in dhcpv6()

[David Gibson]

At least some cppcheck versions (2.18.3 for me) complain that the _storage
variables in dhcpv6() could be reduced in scope.  That's not actually the
case, because although we don't reference the variables, we may touch
their memory via pointers after the blocks in question.  There's no
reasonable way for cppcheck to determine that, though, so suppress its
warnings.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 dhcpv6.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/dhcpv6.c b/dhcpv6.c
index c1a27aba..e4df0db5 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -550,10 +550,18 @@ int dhcpv6(struct ctx *c, struct iov_tail *data,
 {
 	const struct opt_server_id *server_id = NULL;
 	const struct opt_hdr *client_id = NULL;
+	/* The _storage variables can't be local to the blocks they're used in,
+	 * because IOV_*_HEADER() may return pointers to them which are
+	 * dereferenced afterwards. Since we don't have Rust-like lifetime
+	 * tracking, cppcheck can't reasonably determine that, so we must
+	 * suppress its warnings. */
+	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
 	struct opt_server_id server_id_storage;
 	struct iov_tail opt, client_id_base;
 	const struct opt_ia_na *ia = NULL;
+	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
 	struct opt_hdr client_id_storage;
+	/* cppcheck-suppress [variableScope,unmatchedSuppression] */
 	struct opt_ia_na ia_storage;
 	const struct in6_addr *src;
 	struct msg_hdr mh_storage;
-- 
2.51.0

[PATCH v2 6/6] test: Don't delete exetool on make clean

[David Gibson]

exetool is listed in $(LOCAL_ASSETS), which seems like it makes sense,
until you realise that $(LOCAL_ASSETS) are deleted on make clean, and we
can't rebuild exetool after deleting it (since it's just part of the
exeter tree we download).  Move it to $(DOWNLOAD_ASSETS) instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/test/Makefile b/test/Makefile
index 61ccb075..60fd12a7 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -54,15 +54,14 @@ UBUNTU_NEW_IMGS = xenial-server-cloudimg-powerpc-disk1.img \
 	jammy-server-cloudimg-s390x.img
 UBUNTU_IMGS = $(UBUNTU_OLD_IMGS) $(UBUNTU_NEW_IMGS)
 
-DOWNLOAD_ASSETS = exeter mbuto podman \
+DOWNLOAD_ASSETS = $(EXETOOL) mbuto podman \
 	$(DEBIAN_IMGS) $(FEDORA_IMGS) $(OPENSUSE_IMGS) $(UBUNTU_IMGS)
 TESTDATA_ASSETS = small.bin big.bin medium.bin \
 	rampstream
 LOCAL_ASSETS = mbuto.img mbuto.mem.img podman/bin/podman QEMU_EFI.fd \
 	$(DEBIAN_IMGS:%=prepared-%) $(FEDORA_IMGS:%=prepared-%) \
 	$(UBUNTU_NEW_IMGS:%=prepared-%) \
-	nstool guest-key guest-key.pub $(EXETOOL) \
-	$(TESTDATA_ASSETS)
+	nstool guest-key guest-key.pub $(TESTDATA_ASSETS)
 
 ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
 
-- 
2.51.0