[PATCH 00/13] Improvements to static checker invocation

[PATCH 00/13] Improvements to static checker invocation

[David Gibson]

While working on pesto, I ran into a number of awkward errors with the
static checkers.  This series reworks the invocation of the checkers
in a way that will let us deal with that.  As a bonus, it also gives
us static checking for passt-repair and qrap.  It also makes a few
other cleanups to the Makefile that seemed natural along the way.

David Gibson (13):
  Makefile: Use make variables for static checker configuration
  cppcheck: Split out essential defines into a BASE_CPPFLAGS variable
  Makefile: Remove preprocessor flags from $(FLAGS)
  Makefile: Remove non-standard $(FLAGS) variable
  Makefile: Make conditional definition of $(BIN) clearer
  Makefile: Use common binary compilation rule
  Makefile: Remove unhelpful $(HEADERS) variable
  Makefile: Add header dependencies for secondary binaries
  Makefile: Split static checker targets
  passt-repair: Split out inotify handling to its own function
  passt-repair: Simplify construction of Unix path from inotify
  passt-repair: Run static checkers
  qrap: Run static checkers

 Makefile       | 106 ++++++++++++++++++------------
 linux_dep.h    |   2 +-
 passt-repair.c | 171 +++++++++++++++++++++++++++----------------------
 qrap.c         |  42 +++++++-----
 4 files changed, 188 insertions(+), 133 deletions(-)

-- 
2.53.0

[PATCH 01/13] Makefile: Use make variables for static checker configuration

[David Gibson]

Our cppcheck and clang-tidy rules don't really follow normal Makefile
conventions.  Usually any commands other than the very basics have their
binary specified in a variable so it can be overridden on the command line
if they're in an unusual location.  Implement that for $(CPPCHECK) and
$(CLANG_TIDY)

Likewise flags to tools usually have their own Make variable.  Do the same
with $(CLANG_TIDY_FLAGS) and $(CPPCHECK_FLAGS).  Note that these only have
the options specifically for the static checker, not compiler flags which
we are also supplying to the static checker - those are derived from
FLAGS / CFLAGS / CPPFLAGS as before.

As part of that we change the probing for --check-level=exhaustive from
being run as part of the cppcheck target, to being run when we build the
CPPCHECK_FLAGS variable.  That doesn't make any real difference now, but
will make things nicer if we need multiple cppcheck targets in future (e.g.
for passt-repair).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index f697c12b..17e70d22 100644
--- a/Makefile
+++ b/Makefile
@@ -174,21 +174,27 @@ docs: README.md
 		done < README.md;					\
 	) > README.plain.md
 
+CLANG_TIDY = clang-tidy
+CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
+
 clang-tidy: $(PASST_SRCS)
-	clang-tidy $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
-	           -DCLANG_TIDY_58992
+	$(CLANG_TIDY) $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
+		$(CLANG_TIDY_FLAGS)
 
-cppcheck: $(PASST_SRCS) $(HEADERS)
-	if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \
-		CPPCHECK_EXHAUSTIVE="--check-level=exhaustive";		\
-	else								\
-		CPPCHECK_EXHAUSTIVE=;					\
-	fi;								\
-	cppcheck --std=c11 --error-exitcode=1 --enable=all --force	\
+CPPCHECK = cppcheck
+CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	--inconclusive --library=posix --quiet				\
-	$${CPPCHECK_EXHAUSTIVE}						\
 	--inline-suppr							\
-	--suppress=missingIncludeSystem \
+	$(shell if $(CPPCHECK) --quiet --check-level=exhaustive /dev/null; then \
+		echo "--check-level=exhaustive";			\
+	else								\
+		echo "";						\
+	fi)								\
+	--suppress=missingIncludeSystem					\
 	--suppress=unusedStructMember					\
-	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936	\
-	$^
+	 -D CPPCHECK_6936
+
+cppcheck: $(PASST_SRCS) $(HEADERS)
+	$(CPPCHECK) $(CPPCHECK_FLAGS) 					\
+		$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) $^		\
+		$^
-- 
2.53.0

[PATCH 02/13] cppcheck: Split out essential defines into a BASE_CPPFLAGS variable

[David Gibson]

Our cppcheck target need certain flags from the compiler so that they it
can analyse the code correctly.  Currently we extract these rather
awkwardly from FLAGS / CFLAGS / CPPFLAGS.  But this means we inhibit one
of cppcheck's features: by default it will attempt to analyse paths for all
combinations of compile time options, not just a single one.

Analysing *all* paths doesn't work for us because many of the -D options we
use are essential to compile at all, so unless we supply those to cppcheck,
overriding the default behaviour we get many spurious errors.  At the
moment, however, we give cppcheck *all* our -D options, including
conditional / configurable ones, not just the essential ones.

All cppcheck really needs here is those essential -D options.  Split those
into a separate variable, and use that directly rather than the clunky
$(filter) expression.

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

diff --git a/Makefile b/Makefile
index 17e70d22..0de98375 100644
--- a/Makefile
+++ b/Makefile
@@ -30,11 +30,15 @@ ifeq ($(shell $(CC) -O2 -dM -E - < /dev/null 2>&1 | grep ' _FORTIFY_SOURCE ' > /
 FORTIFY_FLAG := -D_FORTIFY_SOURCE=2
 endif
 
+# Require preprocessor flags we can't build without
+BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE \
+	-DPAGE_SIZE=$(shell getconf PAGE_SIZE) \
+	-DVERSION=\"$(VERSION)\"
+
 FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security
-FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE
+FLAGS += -pedantic -std=c11
 FLAGS +=  $(FORTIFY_FLAG) -O2 -pie -fPIE
-FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
-FLAGS += -DVERSION=\"$(VERSION)\"
+FLAGS += $(BASE_CPPFLAGS)
 FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
 
 PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
@@ -195,6 +199,4 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	 -D CPPCHECK_6936
 
 cppcheck: $(PASST_SRCS) $(HEADERS)
-	$(CPPCHECK) $(CPPCHECK_FLAGS) 					\
-		$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) $^		\
-		$^
+	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
-- 
2.53.0

[PATCH 03/13] Makefile: Remove preprocessor flags from $(FLAGS)

[David Gibson]

FLAGS contains both compiler and preprocessor flags, which isn't great
make practice.  Move preprocessor flags out of that variable and into the
more conventionally named $(CPPFLAGS).  Use both $(BASE_CPPFLAGS) and
$(CPPFLAGS) in most places, so we get both the essential flags and the
conditional ones (which could be overridden on the command line).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 0de98375..e89e5556 100644
--- a/Makefile
+++ b/Makefile
@@ -34,12 +34,10 @@ endif
 BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE \
 	-DPAGE_SIZE=$(shell getconf PAGE_SIZE) \
 	-DVERSION=\"$(VERSION)\"
+CPPFLAGS := $(FORTIFY_FLAG) -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
 
 FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security
-FLAGS += -pedantic -std=c11
-FLAGS +=  $(FORTIFY_FLAG) -O2 -pie -fPIE
-FLAGS += $(BASE_CPPFLAGS)
-FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
+FLAGS += -pedantic -std=c11 -O2 -pie -fPIE
 
 PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
 	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
@@ -64,7 +62,7 @@ HEADERS = $(PASST_HEADERS) seccomp.h
 
 C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
 ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
-	FLAGS += -DHAS_GETRANDOM
+	CPPFLAGS += -DHAS_GETRANDOM
 endif
 
 ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
@@ -87,7 +85,8 @@ endif
 
 all: $(BIN) $(MANPAGES) docs
 
-static: FLAGS += -static -DGLIBC_NO_STATIC_NSS
+static: FLAGS += -static
+static: CPPFLAGS += -DGLIBC_NO_STATIC_NSS
 static: clean all
 
 seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
@@ -97,11 +96,11 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
 	@ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS)
 
 passt: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
+	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
 
 passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
 passt.avx2: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(CPPFLAGS) \
+	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) \
 		$(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
 
 passt.avx2: passt
@@ -110,16 +109,17 @@ pasta.avx2 pasta.1 pasta: pasta%: passt%
 	ln -sf $< $@
 
 qrap: $(QRAP_SRCS) passt.h
-	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
+	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
 
 passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h
-	$(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
+	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
 
 valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
 			    rt_sigreturn getpid gettid kill clock_gettime \
 			    mmap|mmap2 munmap open unlink gettimeofday futex \
 			    statx readlink
-valgrind: FLAGS += -g -DVALGRIND
+valgrind: FLAGS += -g
+valgrind: CPPFLAGS += -DVALGRIND
 valgrind: all
 
 .PHONY: clean
@@ -182,8 +182,7 @@ CLANG_TIDY = clang-tidy
 CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
 
 clang-tidy: $(PASST_SRCS)
-	$(CLANG_TIDY) $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \
-		$(CLANG_TIDY_FLAGS)
+	$(CLANG_TIDY) $^ -- $(BASE_CPPFLAGS) $(CPPFLAGS) $(CLANG_TIDY_FLAGS)
 
 CPPCHECK = cppcheck
 CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
-- 
2.53.0

[PATCH 04/13] Makefile: Remove non-standard $(FLAGS) variable

[David Gibson]

FLAGS was introduced over the more standard CFLAGS, because there are some
options we can't compile without, so overriding CFLAGS from the command
line wasn't practical.  We've now better dealt with that using
BASE_CPPFLAGS, so there's no real need for FLAGS any more.  Replace it
with the more conventional CFLAGS, which now *can* be reasonable overridden
from the command line.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index e89e5556..1e5f0282 100644
--- a/Makefile
+++ b/Makefile
@@ -36,8 +36,8 @@ BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE \
 	-DVERSION=\"$(VERSION)\"
 CPPFLAGS := $(FORTIFY_FLAG) -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
 
-FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security
-FLAGS += -pedantic -std=c11 -O2 -pie -fPIE
+WARNINGS = -Wall -Wextra -Wno-format-zero-length -Wformat-security
+CFLAGS = -pedantic -std=c11 -O2 -pie -fPIE $(WARNINGS)
 
 PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
 	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
@@ -66,7 +66,7 @@ ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
 endif
 
 ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
-	FLAGS += -fstack-protector-strong
+	CFLAGS += -fstack-protector-strong
 endif
 
 prefix		?= /usr/local
@@ -85,7 +85,7 @@ endif
 
 all: $(BIN) $(MANPAGES) docs
 
-static: FLAGS += -static
+static: CFLAGS += -static
 static: CPPFLAGS += -DGLIBC_NO_STATIC_NSS
 static: clean all
 
@@ -96,12 +96,11 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
 	@ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS)
 
 passt: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
+	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
 
-passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
+passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
 passt.avx2: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) \
-		$(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
+	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
 
 passt.avx2: passt
 
@@ -109,16 +108,16 @@ pasta.avx2 pasta.1 pasta: pasta%: passt%
 	ln -sf $< $@
 
 qrap: $(QRAP_SRCS) passt.h
-	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
+	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
 
 passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h
-	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
+	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
 
 valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
 			    rt_sigreturn getpid gettid kill clock_gettime \
 			    mmap|mmap2 munmap open unlink gettimeofday futex \
 			    statx readlink
-valgrind: FLAGS += -g
+valgrind: CFLAGS += -g
 valgrind: CPPFLAGS += -DVALGRIND
 valgrind: all
 
-- 
2.53.0

[PATCH 05/13] Makefile: Make conditional definition of $(BIN) clearer

[David Gibson]

The list of binaries is dependent on the target architecture, because
x86_64 addes the passt.avx2 and pasta.avx2 binaries.  Make this more clear
by defining BIN in common, then augmenting it in the x86_64 case.

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

diff --git a/Makefile b/Makefile
index 1e5f0282..a2576771 100644
--- a/Makefile
+++ b/Makefile
@@ -77,10 +77,9 @@ docdir		?= $(datarootdir)/doc/passt
 mandir		?= $(datarootdir)/man
 man1dir		?= $(mandir)/man1
 
+BIN = passt pasta qrap passt-repair
 ifeq ($(TARGET_ARCH),x86_64)
-BIN := passt passt.avx2 pasta pasta.avx2 qrap passt-repair
-else
-BIN := passt pasta qrap passt-repair
+BIN += passt.avx2 pasta.avx2
 endif
 
 all: $(BIN) $(MANPAGES) docs
-- 
2.53.0

[PATCH 06/13] Makefile: Use common binary compilation rule

[David Gibson]

Each of our binaries (passt, passt.avx2, qrap and passt-repair) has a
separate Make rule instructing how to compile it, but they're all basically
identical.  Combine these all into a single pattern rule, just using
different dependencies and variable overrides where necessary.

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

diff --git a/Makefile b/Makefile
index a2576771..18343491 100644
--- a/Makefile
+++ b/Makefile
@@ -77,9 +77,14 @@ docdir		?= $(datarootdir)/doc/passt
 mandir		?= $(datarootdir)/man
 man1dir		?= $(mandir)/man1
 
-BIN = passt pasta qrap passt-repair
+BASEBIN = passt qrap passt-repair
 ifeq ($(TARGET_ARCH),x86_64)
-BIN += passt.avx2 pasta.avx2
+BASEBIN += passt.avx2
+endif
+
+BIN = $(BASEBIN) pasta
+ifeq ($(TARGET_ARCH),x86_64)
+BIN += pasta.avx2
 endif
 
 all: $(BIN) $(MANPAGES) docs
@@ -94,23 +99,21 @@ seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
 seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
 	@ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS)
 
+$(BASEBIN): %:
+	$(CC) $(BASE_CPPFLAGS) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^) -o $@
+
 passt: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
 
 passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
 passt.avx2: $(PASST_SRCS) $(HEADERS)
-	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
-
-passt.avx2: passt
 
 pasta.avx2 pasta.1 pasta: pasta%: passt%
 	ln -sf $< $@
 
+qrap: BASE_CPPFLAGS += -DARCH=\"$(TARGET_ARCH)\"
 qrap: $(QRAP_SRCS) passt.h
-	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
 
 passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h
-	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
 
 valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
 			    rt_sigreturn getpid gettid kill clock_gettime \
-- 
2.53.0

[PATCH 07/13] Makefile: Remove unhelpful $(HEADERS) variable

[David Gibson]

Confusingly HEADERS is not headers that are shared between our various
binaries.  Rather it's just the (non generated) headers for passt, plus
seccomp.h.  This isn't particularly useful, just open code it in the
handful of places we need it.

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

diff --git a/Makefile b/Makefile
index 18343491..03113803 100644
--- a/Makefile
+++ b/Makefile
@@ -58,7 +58,6 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
 	siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \
 	tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \
 	virtio.h vu_common.h
-HEADERS = $(PASST_HEADERS) seccomp.h
 
 C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
 ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
@@ -102,10 +101,10 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
 $(BASEBIN): %:
 	$(CC) $(BASE_CPPFLAGS) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^) -o $@
 
-passt: $(PASST_SRCS) $(HEADERS)
+passt: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 
 passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
-passt.avx2: $(PASST_SRCS) $(HEADERS)
+passt.avx2: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 
 pasta.avx2 pasta.1 pasta: pasta%: passt%
 	ln -sf $< $@
@@ -198,5 +197,5 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	--suppress=unusedStructMember					\
 	 -D CPPCHECK_6936
 
-cppcheck: $(PASST_SRCS) $(HEADERS)
+cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
-- 
2.53.0

[PATCH 08/13] Makefile: Add header dependencies for secondary binaries

[David Gibson]

PASST_HEADERS contains all the headers used by passt, which we use in
various dependencies.  However, qrap and passt-repair each use several
headers which we don't have dependencies for.  Add handling for this to the
Makefile.

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

diff --git a/Makefile b/Makefile
index 03113803..9cb5e1ba 100644
--- a/Makefile
+++ b/Makefile
@@ -58,6 +58,8 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \
 	siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \
 	tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \
 	virtio.h vu_common.h
+QRAP_HEADERS = arp.h ip.h passt.h util.h
+PASST_REPAIR_HEADERS = linux_dep.h
 
 C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
 ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
@@ -95,7 +97,7 @@ static: clean all
 seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
 	@ EXTRA_SYSCALLS="$(EXTRA_SYSCALLS)" ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp.h $(PASST_SRCS) $(PASST_HEADERS)
 
-seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
+seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS)
 	@ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS)
 
 $(BASEBIN): %:
@@ -110,9 +112,9 @@ pasta.avx2 pasta.1 pasta: pasta%: passt%
 	ln -sf $< $@
 
 qrap: BASE_CPPFLAGS += -DARCH=\"$(TARGET_ARCH)\"
-qrap: $(QRAP_SRCS) passt.h
+qrap: $(QRAP_SRCS) $(QRAP_HEADERS)
 
-passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h
+passt-repair: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
 
 valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
 			    rt_sigreturn getpid gettid kill clock_gettime \
-- 
2.53.0

[PATCH 09/13] Makefile: Split static checker targets

[David Gibson]

Currently we have a single 'cppcheck' and 'clang-tidy' target which checks
passt.  However, it doesn't check the additional binaries, qrap and
passt-repair.  In preparation for running the static checkers on those as
well, split the targets into a top-level rule and a pattern rule which we
will be able to reuse.

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

diff --git a/Makefile b/Makefile
index 9cb5e1ba..b8a3cf9e 100644
--- a/Makefile
+++ b/Makefile
@@ -183,8 +183,13 @@ docs: README.md
 CLANG_TIDY = clang-tidy
 CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
 
-clang-tidy: $(PASST_SRCS)
-	$(CLANG_TIDY) $^ -- $(BASE_CPPFLAGS) $(CPPFLAGS) $(CLANG_TIDY_FLAGS)
+clang-tidy: passt.clang-tidy
+
+.PHONY: %.clang-tidy
+%.clang-tidy:
+	$(CLANG_TIDY) $(filter %.c,$^) -- $(BASE_CPPFLAGS) $(CPPFLAGS) $(CLANG_TIDY_FLAGS)
+
+passt.clang-tidy: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 
 CPPCHECK = cppcheck
 CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
@@ -199,5 +204,10 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	--suppress=unusedStructMember					\
 	 -D CPPCHECK_6936
 
-cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
+cppcheck: passt.cppcheck
+
+.PHONY: %.cppcheck
+%.cppcheck:
 	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
+
+passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
-- 
2.53.0

[PATCH 10/13] passt-repair: Split out inotify handling to its own function

[David Gibson]

passt-repair can operate two ways: either it can be given an explicit
socket path to connect to, or it can be given a directory.  In the second
mode, it will wait for a socket to appear in that directory before
connecting to it.

That waiting involves some inotify logic that is essentially unrelated to
the rest of the code.  However, it's currently inline in main() making that
very long.  Moreover, the block handling inotify shadows several variables
used in the rest of main() which will make static checkers complain once
we get them running on passt-repair.

Address this by moving the inotify handling into its own function.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt-repair.c | 137 +++++++++++++++++++++++++++----------------------
 1 file changed, 77 insertions(+), 60 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index c3c140f5..980b0b09 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -46,6 +46,82 @@
 #define REPAIR_EXT		".repair"
 #define REPAIR_EXT_LEN		strlen(REPAIR_EXT)
 
+/**
+ * wait_for_socket() - Wait for a Unix socket to appear in a directory
+ * @a:		Unix domain address to update with socket's path
+ * @dir:	Path to directory to wait for socket in
+ * @sb:		Stat block, populated for the discovered socket on exit
+ *
+ * Return: Length of socket address
+ *
+ * #syscalls:repair close
+ * #syscalls:repair stat|statx stat64|statx statx
+ * #syscalls:repair inotify_init1 inotify_add_watch
+ */
+static int wait_for_socket(struct sockaddr_un *a, const char *dir,
+			   struct stat *sb)
+{
+	char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		__attribute__ ((aligned(__alignof__(struct inotify_event))));
+	const struct inotify_event *ev = NULL;
+	char path[PATH_MAX + 1];
+	bool found = false;
+	ssize_t n;
+	int fd;
+
+	if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
+		fprintf(stderr, "inotify_init1: %i\n", errno);
+		_exit(1);
+	}
+
+	if (inotify_add_watch(fd, dir, IN_CREATE) < 0) {
+		fprintf(stderr, "inotify_add_watch: %i\n", errno);
+		_exit(1);
+	}
+
+	do {
+		char *p;
+
+		n = read(fd, buf, sizeof(buf));
+		if (n < 0) {
+			fprintf(stderr, "inotify read: %i\n", errno);
+			_exit(1);
+		}
+		buf[n - 1] = '\0';
+
+		if (n < (ssize_t)sizeof(*ev)) {
+			fprintf(stderr, "Short inotify read: %zi\n", n);
+			continue;
+		}
+
+		for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
+			ev = (const struct inotify_event *)p;
+
+			if (ev->len >= REPAIR_EXT_LEN &&
+			    !memcmp(ev->name +
+				    strnlen(ev->name, ev->len) -
+				    REPAIR_EXT_LEN,
+				    REPAIR_EXT, REPAIR_EXT_LEN)) {
+				found = true;
+				break;
+			}
+		}
+	} while (!found);
+
+	if (ev->len > NAME_MAX + 1 || ev->name[ev->len - 1] != '\0') {
+		fprintf(stderr, "Invalid filename from inotify\n");
+		_exit(1);
+	}
+
+	snprintf(path, sizeof(path), "%s/%s", dir, ev->name);
+	if ((stat(path, sb))) {
+		fprintf(stderr, "Can't stat() %s: %i\n", path, errno);
+		_exit(1);
+	}
+
+	return snprintf(a->sun_path, sizeof(a->sun_path), "%s", path);
+}
+
 /**
  * main() - Entry point and whole program with loop
  * @argc:	Argument count, must be 2
@@ -59,7 +135,6 @@
  * #syscalls:repair sendto sendmsg arm:send ppc64le:send
  * #syscalls:repair stat|statx stat64|statx statx
  * #syscalls:repair fstat|fstat64 newfstatat|fstatat64
- * #syscalls:repair inotify_init1 inotify_add_watch
  */
 int main(int argc, char **argv)
 {
@@ -112,65 +187,7 @@ int main(int argc, char **argv)
 	}
 
 	if ((sb.st_mode & S_IFMT) == S_IFDIR) {
-		char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
-		   __attribute__ ((aligned(__alignof__(struct inotify_event))));
-		const struct inotify_event *ev = NULL;
-		char path[PATH_MAX + 1];
-		bool found = false;
-		ssize_t n;
-		int fd;
-
-		if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
-			fprintf(stderr, "inotify_init1: %i\n", errno);
-			_exit(1);
-		}
-
-		if (inotify_add_watch(fd, argv[1], IN_CREATE) < 0) {
-			fprintf(stderr, "inotify_add_watch: %i\n", errno);
-			_exit(1);
-		}
-
-		do {
-			char *p;
-
-			n = read(fd, buf, sizeof(buf));
-			if (n < 0) {
-				fprintf(stderr, "inotify read: %i\n", errno);
-				_exit(1);
-			}
-			buf[n - 1] = '\0';
-
-			if (n < (ssize_t)sizeof(*ev)) {
-				fprintf(stderr, "Short inotify read: %zi\n", n);
-				continue;
-			}
-
-			for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
-				ev = (const struct inotify_event *)p;
-
-				if (ev->len >= REPAIR_EXT_LEN &&
-				    !memcmp(ev->name +
-					    strnlen(ev->name, ev->len) -
-					    REPAIR_EXT_LEN,
-					    REPAIR_EXT, REPAIR_EXT_LEN)) {
-					found = true;
-					break;
-				}
-			}
-		} while (!found);
-
-		if (ev->len > NAME_MAX + 1 || ev->name[ev->len - 1] != '\0') {
-			fprintf(stderr, "Invalid filename from inotify\n");
-			_exit(1);
-		}
-
-		snprintf(path, sizeof(path), "%s/%s", argv[1], ev->name);
-		if ((stat(path, &sb))) {
-			fprintf(stderr, "Can't stat() %s: %i\n", path, errno);
-			_exit(1);
-		}
-
-		ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", path);
+		ret = wait_for_socket(&a, argv[1], &sb);
 		inotify_dir = true;
 	} else {
 		ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]);
-- 
2.53.0

[PATCH 11/13] passt-repair: Simplify construction of Unix path from inotify

[David Gibson]

When passt-repair is invoked with a directory name, it waits for a Unix
socket to appear in that directory.  We need to build the Unix path name
from the given directory, plus the stem file name from the inotify event.

Currently, we build that path into a temporary buffer of size PATH_MAX,
then move it into the smaller buffer inside the Unix sockaddr.  There's no
particular reason for this two step process, we can build the address
directly within the sockaddr_un.  This will give a slightly different error
if the constructed path exceeds the maximum length of a Unix address, but
it will fail either way so it doesn't really matter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt-repair.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index 980b0b09..d4c8ce9a 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -64,10 +64,9 @@ static int wait_for_socket(struct sockaddr_un *a, const char *dir,
 	char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
 		__attribute__ ((aligned(__alignof__(struct inotify_event))));
 	const struct inotify_event *ev = NULL;
-	char path[PATH_MAX + 1];
 	bool found = false;
+	int fd, ret;
 	ssize_t n;
-	int fd;
 
 	if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
 		fprintf(stderr, "inotify_init1: %i\n", errno);
@@ -113,13 +112,15 @@ static int wait_for_socket(struct sockaddr_un *a, const char *dir,
 		_exit(1);
 	}
 
-	snprintf(path, sizeof(path), "%s/%s", dir, ev->name);
-	if ((stat(path, sb))) {
-		fprintf(stderr, "Can't stat() %s: %i\n", path, errno);
+	ret = snprintf(a->sun_path, sizeof(a->sun_path), "%s/%s",
+		       dir, ev->name);
+
+	if ((stat(a->sun_path, sb))) {
+		fprintf(stderr, "Can't stat() %s: %i\n", a->sun_path, errno);
 		_exit(1);
 	}
 
-	return snprintf(a->sun_path, sizeof(a->sun_path), "%s", path);
+	return ret;
 }
 
 /**
-- 
2.53.0

[PATCH 12/13] passt-repair: Run static checkers

[David Gibson]

Run the static checkers, cppcheck and clang-tidy on passt-repair as well
as on passt proper.  This shows up handful of remaining minor warnings,
which we correct.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile       |  6 ++++--
 linux_dep.h    |  2 +-
 passt-repair.c | 49 +++++++++++++++++++++++++------------------------
 3 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/Makefile b/Makefile
index b8a3cf9e..d29f46d2 100644
--- a/Makefile
+++ b/Makefile
@@ -183,13 +183,14 @@ docs: README.md
 CLANG_TIDY = clang-tidy
 CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
 
-clang-tidy: passt.clang-tidy
+clang-tidy: passt.clang-tidy passt-repair.clang-tidy
 
 .PHONY: %.clang-tidy
 %.clang-tidy:
 	$(CLANG_TIDY) $(filter %.c,$^) -- $(BASE_CPPFLAGS) $(CPPFLAGS) $(CLANG_TIDY_FLAGS)
 
 passt.clang-tidy: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
+passt-repair.clang-tidy: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
 
 CPPCHECK = cppcheck
 CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
@@ -204,10 +205,11 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	--suppress=unusedStructMember					\
 	 -D CPPCHECK_6936
 
-cppcheck: passt.cppcheck
+cppcheck: passt.cppcheck passt-repair.cppcheck
 
 .PHONY: %.cppcheck
 %.cppcheck:
 	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
 
 passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
+passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
diff --git a/linux_dep.h b/linux_dep.h
index 3f8184bd..fa539ce4 100644
--- a/linux_dep.h
+++ b/linux_dep.h
@@ -145,7 +145,7 @@ struct tcp_info_linux {
 #endif
 
 __attribute__ ((weak))
-/* cppcheck-suppress [funcArgNamesDifferent,unmatchedSuppression] */
+/* cppcheck-suppress [funcArgNamesDifferent,unusedFunction,unmatchedSuppression] */
 int close_range(unsigned int first, unsigned int last, int flags) {
 	return syscall(SYS_close_range, first, last, flags);
 }
diff --git a/passt-repair.c b/passt-repair.c
index d4c8ce9a..80a39086 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -46,6 +46,9 @@
 #define REPAIR_EXT		".repair"
 #define REPAIR_EXT_LEN		strlen(REPAIR_EXT)
 
+/* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */
+#define FPRINTF(f, ...)	(void)fprintf(f, __VA_ARGS__)
+
 /**
  * wait_for_socket() - Wait for a Unix socket to appear in a directory
  * @a:		Unix domain address to update with socket's path
@@ -66,33 +69,33 @@ static int wait_for_socket(struct sockaddr_un *a, const char *dir,
 	const struct inotify_event *ev = NULL;
 	bool found = false;
 	int fd, ret;
-	ssize_t n;
 
 	if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
-		fprintf(stderr, "inotify_init1: %i\n", errno);
+		FPRINTF(stderr, "inotify_init1: %i\n", errno);
 		_exit(1);
 	}
 
 	if (inotify_add_watch(fd, dir, IN_CREATE) < 0) {
-		fprintf(stderr, "inotify_add_watch: %i\n", errno);
+		FPRINTF(stderr, "inotify_add_watch: %i\n", errno);
 		_exit(1);
 	}
 
 	do {
+		ssize_t n;
 		char *p;
 
 		n = read(fd, buf, sizeof(buf));
 		if (n < 0) {
-			fprintf(stderr, "inotify read: %i\n", errno);
+			FPRINTF(stderr, "inotify read: %i\n", errno);
 			_exit(1);
 		}
-		buf[n - 1] = '\0';
 
 		if (n < (ssize_t)sizeof(*ev)) {
-			fprintf(stderr, "Short inotify read: %zi\n", n);
+			FPRINTF(stderr, "Short inotify read: %zi\n", n);
 			continue;
 		}
 
+		buf[n - 1] = '\0';
 		for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
 			ev = (const struct inotify_event *)p;
 
@@ -108,7 +111,7 @@ static int wait_for_socket(struct sockaddr_un *a, const char *dir,
 	} while (!found);
 
 	if (ev->len > NAME_MAX + 1 || ev->name[ev->len - 1] != '\0') {
-		fprintf(stderr, "Invalid filename from inotify\n");
+		FPRINTF(stderr, "Invalid filename from inotify\n");
 		_exit(1);
 	}
 
@@ -116,7 +119,7 @@ static int wait_for_socket(struct sockaddr_un *a, const char *dir,
 		       dir, ev->name);
 
 	if ((stat(a->sun_path, sb))) {
-		fprintf(stderr, "Can't stat() %s: %i\n", a->sun_path, errno);
+		FPRINTF(stderr, "Can't stat() %s: %i\n", a->sun_path, errno);
 		_exit(1);
 	}
 
@@ -160,7 +163,7 @@ int main(int argc, char **argv)
 	prog.filter = filter_repair;
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
 	    prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-		fprintf(stderr, "Failed to apply seccomp filter\n");
+		FPRINTF(stderr, "Failed to apply seccomp filter\n");
 		_exit(1);
 	}
 
@@ -173,17 +176,17 @@ int main(int argc, char **argv)
 	cmsg = CMSG_FIRSTHDR(&msg);
 
 	if (argc != 2) {
-		fprintf(stderr, "Usage: %s PATH\n", argv[0]);
+		FPRINTF(stderr, "Usage: %s PATH\n", argv[0]);
 		_exit(2);
 	}
 
 	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
-		fprintf(stderr, "Failed to create AF_UNIX socket: %i\n", errno);
+		FPRINTF(stderr, "Failed to create AF_UNIX socket: %i\n", errno);
 		_exit(1);
 	}
 
 	if ((stat(argv[1], &sb))) {
-		fprintf(stderr, "Can't stat() %s: %i\n", argv[1], errno);
+		FPRINTF(stderr, "Can't stat() %s: %i\n", argv[1], errno);
 		_exit(1);
 	}
 
@@ -195,12 +198,12 @@ int main(int argc, char **argv)
 	}
 
 	if (ret <= 0 || ret >= (int)sizeof(a.sun_path)) {
-		fprintf(stderr, "Invalid socket path\n");
+		FPRINTF(stderr, "Invalid socket path\n");
 		_exit(2);
 	}
 
 	if ((sb.st_mode & S_IFMT) != S_IFSOCK) {
-		fprintf(stderr, "%s is not a socket\n", a.sun_path);
+		FPRINTF(stderr, "%s is not a socket\n", a.sun_path);
 		_exit(2);
 	}
 
@@ -208,7 +211,7 @@ int main(int argc, char **argv)
 		if (inotify_dir && errno == ECONNREFUSED)
 			continue;
 
-		fprintf(stderr, "Failed to connect to %s: %s\n", a.sun_path,
+		FPRINTF(stderr, "Failed to connect to %s: %s\n", a.sun_path,
 			strerror(errno));
 		_exit(1);
 	}
@@ -219,7 +222,7 @@ loop:
 		if (errno == ECONNRESET) {
 			ret = 0;
 		} else {
-			fprintf(stderr, "Failed to read message: %i\n", errno);
+			FPRINTF(stderr, "Failed to read message: %i\n", errno);
 			_exit(1);
 		}
 	}
@@ -231,7 +234,7 @@ loop:
 	    cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
 	    cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) ||
 	    cmsg->cmsg_type != SCM_RIGHTS) {
-		fprintf(stderr, "No/bad ancillary data from peer\n");
+		FPRINTF(stderr, "No/bad ancillary data from peer\n");
 		_exit(1);
 	}
 
@@ -246,7 +249,7 @@ loop:
 	}
 	if (!n) {
 		cmsg_len = cmsg->cmsg_len; /* socklen_t is 'unsigned' on musl */
-		fprintf(stderr, "Invalid ancillary data length %zu from peer\n",
+		FPRINTF(stderr, "Invalid ancillary data length %zu from peer\n",
 			cmsg_len);
 		_exit(1);
 	}
@@ -255,15 +258,15 @@ loop:
 
 	if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF &&
 	    cmd != TCP_REPAIR_OFF_NO_WP) {
-		fprintf(stderr, "Unsupported command 0x%04x\n", cmd);
+		FPRINTF(stderr, "Unsupported command 0x%04x\n", cmd);
 		_exit(1);
 	}
 
-	op = cmd;
+	op = (int)cmd;
 
 	for (i = 0; i < n; i++) {
 		if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) {
-			fprintf(stderr,
+			FPRINTF(stderr,
 				"Setting TCP_REPAIR to %i on socket %i: %s\n",
 				op, fds[i], strerror(errno));
 			_exit(1);
@@ -275,11 +278,9 @@ loop:
 
 	/* Confirm setting by echoing the command back */
 	if (send(s, &cmd, sizeof(cmd), 0) < 0) {
-		fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno));
+		FPRINTF(stderr, "Reply to %i: %s\n", op, strerror(errno));
 		_exit(1);
 	}
 
 	goto loop;
-
-	return 0;
 }
-- 
2.53.0

[PATCH 13/13] qrap: Run static checkers

[David Gibson]

Run the static checkers on qrap as well as on passt-repair and passt.  This
shows a number of minor warnings, which we fix.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile |  9 +++++++--
 qrap.c   | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index d29f46d2..7875d23b 100644
--- a/Makefile
+++ b/Makefile
@@ -183,7 +183,7 @@ docs: README.md
 CLANG_TIDY = clang-tidy
 CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992
 
-clang-tidy: passt.clang-tidy passt-repair.clang-tidy
+clang-tidy: passt.clang-tidy passt-repair.clang-tidy qrap.clang-tidy
 
 .PHONY: %.clang-tidy
 %.clang-tidy:
@@ -191,6 +191,7 @@ clang-tidy: passt.clang-tidy passt-repair.clang-tidy
 
 passt.clang-tidy: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 passt-repair.clang-tidy: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
+qrap.clang-tidy: $(QRAP_SRCS) $(QRAP_HEADERS)
 
 CPPCHECK = cppcheck
 CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
@@ -205,7 +206,7 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force	\
 	--suppress=unusedStructMember					\
 	 -D CPPCHECK_6936
 
-cppcheck: passt.cppcheck passt-repair.cppcheck
+cppcheck: passt.cppcheck passt-repair.cppcheck qrap.cppcheck
 
 .PHONY: %.cppcheck
 %.cppcheck:
@@ -213,3 +214,7 @@ cppcheck: passt.cppcheck passt-repair.cppcheck
 
 passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
 passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
+
+qrap.cppcheck: BASE_CPPFLAGS += -DARCH=\"$(TARGET_ARCH)\"
+qrap.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction
+qrap.cppcheck: $(QRAP_SRCS) $(QRAP_HEADERS)
diff --git a/qrap.c b/qrap.c
index d5967062..4a01ed83 100644
--- a/qrap.c
+++ b/qrap.c
@@ -46,6 +46,15 @@ static char *qemu_names[] = {
 	NULL,
 };
 
+#define BUILDARG(s_, ...)						\
+	do {								\
+		static_assert(sizeof(s_) == ARG_MAX, "Bad buffer size"); \
+		if (snprintf((s_), ARG_MAX, __VA_ARGS__) >= ARG_MAX) {	\
+			FPRINTF(stderr, "Arguments too long");		\
+			exit(EXIT_FAILURE);				\
+		}							\
+	} while (0)
+
 /**
  * struct drop_arg - Drop matching arguments on command line
  * @name:	Option name
@@ -120,11 +129,11 @@ static const struct pci_dev {
  * usage() - Print usage and exit
  * @name:	Executable name
  */
-void usage(const char *name)
+static void usage(const char *name)
 {
-	fprintf(stderr, "Usage: %s [FDNUM QEMU_CMD] [QEMU_ARG]...\n", name);
-	fprintf(stderr, "\n");
-	fprintf(stderr, "If first and second arguments aren't a socket number\n"
+	FPRINTF(stderr, "Usage: %s [FDNUM QEMU_CMD] [QEMU_ARG]...\n", name);
+	FPRINTF(stderr, "\n");
+	FPRINTF(stderr, "If first and second arguments aren't a socket number\n"
 			"and a path, %s will try to locate a qemu binary\n"
 			"and directly patch the command line\n", name);
 
@@ -202,13 +211,14 @@ int main(int argc, char **argv)
 		errno = 0;
 		fd = strtol(argv[1], NULL, 0);
 		if (fd >= 3 && fd < INT_MAX && !errno && path) {
-			char env_path[ARG_MAX + 1], *p, command[ARG_MAX];
+			char env_path[ARG_MAX + 1], command[ARG_MAX];
+			const char *p;
 
 			strncpy(env_path, path, ARG_MAX);
 			/* cppcheck-suppress strtokCalled */
 			p = strtok(env_path, ":");
 			while (p) {
-				snprintf(command, ARG_MAX, "%s/%s", p, argv[2]);
+				BUILDARG(command, "%s/%s", p, argv[2]);
 				if (!access(command, X_OK))
 					goto valid_args;
 
@@ -281,7 +291,7 @@ int main(int argc, char **argv)
 			break;
 	}
 	if (i == dev->last) {
-		fprintf(stderr, "Couldn't find free address for device\n");
+		FPRINTF(stderr, "Couldn't find free address for device\n");
 		usage(argv[0]);
 	}
 
@@ -289,21 +299,21 @@ int main(int argc, char **argv)
 		qemu_argv[qemu_argc++] = "-device";
 		if (!has_json) {
 			if (dev->base == 16) {
-				snprintf(dev_str, ARG_MAX,
-				         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
+				BUILDARG(dev_str,
+					 "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
 				         dev->name, dev->template, i, dev->template_post);
 			} else if (dev->base == 10) {
-				snprintf(dev_str, ARG_MAX,
+				BUILDARG(dev_str,
 				         "%s,%s%d%s,netdev=hostnet0,x-txburst=4096",
 				         dev->name, dev->template, i, dev->template_post);
 			}
 		} else {
 			if (dev->base == 16) {
-				snprintf(dev_str, ARG_MAX,
+				BUILDARG(dev_str,
 				         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
 				         dev->name, dev->template_json, i, dev->template_json_post);
 			} else if (dev->base == 10) {
-				snprintf(dev_str, ARG_MAX,
+				BUILDARG(dev_str,
 				         "{\"driver\":\"%s\",%s%d\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
 				         dev->name, dev->template_json, i, dev->template_json_post);
 			}
@@ -335,7 +345,7 @@ retry:
 		if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
 			perror("setsockopt SO_SNDTIMEO");
 
-		snprintf(addr.sun_path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
+		(void)snprintf(addr.sun_path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
 
 		errno = 0;
 
@@ -381,7 +391,7 @@ retry:
 			goto retry;
 		}
 
-		fprintf(stderr, "Probe of %s failed\n", addr.sun_path);
+		FPRINTF(stderr, "Probe of %s failed\n", addr.sun_path);
 
 		close(s);
 	}
@@ -397,7 +407,7 @@ retry:
 	if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
 		perror("setsockopt, SO_SNDTIMEO reset");
 
-	fprintf(stderr, "Connected to %s\n", addr.sun_path);
+	FPRINTF(stderr, "Connected to %s\n", addr.sun_path);
 
 	if (dup2(s, (int)fd) < 0) {
 		perror("dup");
@@ -418,7 +428,7 @@ retry:
 			}
 		}
 		if (errno == ENOENT)
-			fprintf(stderr, "Couldn't find qemu command\n");
+			FPRINTF(stderr, "Couldn't find qemu command\n");
 	} else {
 		execvp(argv[2], argv + 2);
 	}
-- 
2.53.0

Re: [PATCH 00/13] Improvements to static checker invocation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Tue, Apr 21, 2026 at 12:43:31PM +1000, David Gibson wrote:
> While working on pesto, I ran into a number of awkward errors with the
> static checkers.  This series reworks the invocation of the checkers
> in a way that will let us deal with that.  As a bonus, it also gives
> us static checking for passt-repair and qrap.  It also makes a few
> other cleanups to the Makefile that seemed natural along the way.

Sorry, realised 4/13 introduced a test failure due to a bad
interaction with test/build/build.py.  New spin coming shortly.

> 
> David Gibson (13):
>   Makefile: Use make variables for static checker configuration
>   cppcheck: Split out essential defines into a BASE_CPPFLAGS variable
>   Makefile: Remove preprocessor flags from $(FLAGS)
>   Makefile: Remove non-standard $(FLAGS) variable
>   Makefile: Make conditional definition of $(BIN) clearer
>   Makefile: Use common binary compilation rule
>   Makefile: Remove unhelpful $(HEADERS) variable
>   Makefile: Add header dependencies for secondary binaries
>   Makefile: Split static checker targets
>   passt-repair: Split out inotify handling to its own function
>   passt-repair: Simplify construction of Unix path from inotify
>   passt-repair: Run static checkers
>   qrap: Run static checkers
> 
>  Makefile       | 106 ++++++++++++++++++------------
>  linux_dep.h    |   2 +-
>  passt-repair.c | 171 +++++++++++++++++++++++++++----------------------
>  qrap.c         |  42 +++++++-----
>  4 files changed, 188 insertions(+), 133 deletions(-)
> 
> -- 
> 2.53.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]