[PATCH v3 00/12] Improvements to static checker invocation

David Gibson

12 May 2026 12 May '26

7:52 a.m.

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. It also a number of other cleanups to the Makefile that seemed natural along the way. v3: - Rework changes to $(FLAGS) so they're much less likely to have side effects we're not ready for. v2: - Fixed nasty test failure in test/build/build.py David Gibson (12): Makefile: Use make variables for static checker configuration 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 $(FLAGS) into cpp and cc components cppcheck, clang-tidy: Static checkers don't need non-preprocessor flags 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 pesto: Run static checkers on pesto sources Makefile | 135 ++++++++++++++++++++++++-------------- linux_dep.h | 2 +- passt-repair.c | 171 +++++++++++++++++++++++++++---------------------- pesto.c | 1 - 4 files changed, 182 insertions(+), 127 deletions(-) -- 2.54.0

Show replies by date

David Gibson

12 May 12 May

7:52 a.m.

New subject: [PATCH v3 01/12] Makefile: Use make variables for static checker configuration

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 --- Makefile | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 5e91da11..48c5fb62 100644 --- a/Makefile +++ b/Makefile @@ -181,21 +181,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.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 07/12] cppcheck, clang-tidy: Static checkers don't need non-preprocessor flags

Currently we pass all our compiler flags to clang-tidy, except -pie, which it won't accept. In fact in order to run the checker, we only need the preprocessor flags. Simplify the command line by passing only those. For cppcheck we already filter out just -D options from the compiler flags. Simplify this by only passing preprocessor flags, now that we've split those out into their own variables. Furthermore, one of cppcheck's features which we're currently not exploiting is to check multiple / all preprocessor option combinations in a single pass. Therefore, pass only $(BASE_CPPFLAGS), which contains the mandatory options with which we can't compile at all. While we're there remove a redundant $^ that slipped in at some point. Signed-off-by: David Gibson --- Makefile | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 697f229f..c6dbbc67 100644 --- a/Makefile +++ b/Makefile @@ -194,9 +194,7 @@ CLANG_TIDY = clang-tidy CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992 clang-tidy: $(PASST_SRCS) - $(CLANG_TIDY) $^ -- \ - $(filter-out -pie,$(BASE_CPPFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) $(CFLAGS)) \ - $(CLANG_TIDY_FLAGS) + $(CLANG_TIDY) $^ -- $(BASE_CPPFLAGS) $(CPPFLAGS) $(CLANG_TIDY_FLAGS) CPPCHECK = cppcheck CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force \ @@ -212,6 +210,4 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force \ -D CPPCHECK_6936 cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h - $(CPPCHECK) $(CPPCHECK_FLAGS) \ - $(filter -D%,$(BASE_CPPFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) $(CFLAGS)) $^ \ - $^ + $(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^ -- 2.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 02/12] Makefile: Make conditional definition of $(BIN) clearer

The list of binaries is dependent on the target architecture, because x86_64 adds the passt.avx2 and pasta.avx2 binaries. Make this more obvious by defining BIN in common, then augmenting it in the x86_64 case. Signed-off-by: David Gibson --- Makefile | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 48c5fb62..d489a862 100644 --- a/Makefile +++ b/Makefile @@ -76,10 +76,9 @@ docdir ?= $(datarootdir)/doc/passt mandir ?= $(datarootdir)/man man1dir ?= $(mandir)/man1 -ifeq ($(TARGET_ARCH),x86_64) -BIN := passt passt.avx2 pasta pasta.avx2 qrap passt-repair pesto -else BIN := passt pasta qrap passt-repair pesto +ifeq ($(TARGET_ARCH),x86_64) +BIN += passt.avx2 pasta.avx2 endif all: $(BIN) $(MANPAGES) docs -- 2.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 06/12] Makefile: Split $(FLAGS) into cpp and cc components

The $(FLAGS) variable contains mandatory compiler flags that should not be overridden. However, it contains a mixture of flags for the preprocessor and for the compiler proper. That's causing some inconvenience for other Makefile cleanups, so split it into $(BASE_CPPFLAGS) and $(BASE_CFLAGS) variables. Signed-off-by: David Gibson --- Makefile | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index a8f8d06e..697f229f 100644 --- a/Makefile +++ b/Makefile @@ -30,12 +30,17 @@ ifeq ($(shell $(CC) -O2 -dM -E - < /dev/null 2>&1 | grep ' _FORTIFY_SOURCE ' > / FORTIFY_FLAG := -D_FORTIFY_SOURCE=2 endif -FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security -FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE -FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DVERSION=\"$(VERSION)\" -FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) +# Mandatory preprocessor flags that won't be overridden with $(CPPFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE $(FORTIFY_FLAG) +BASE_CPPFLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) +BASE_CPPFLAGS += -DVERSION=\"$(VERSION)\" +BASE_CPPFLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) + +# Mandatory compiler flags that won't be overridden with $(CFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CFLAGS := -std=c11 -pie -fPIE -O2 +BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security 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 \ @@ -62,11 +67,11 @@ PASST_REPAIR_HEADERS = linux_dep.h C := \#include \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 + BASE_CPPFLAGS += -DHAS_GETRANDOM endif ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0) - FLAGS += -fstack-protector-strong + BASE_CFLAGS += -fstack-protector-strong endif prefix ?= /usr/local @@ -89,7 +94,8 @@ endif all: $(BIN) $(MANPAGES) docs -static: FLAGS += -static -DGLIBC_NO_STATIC_NSS +static: BASE_CPPFLAGS += -DGLIBC_NO_STATIC_NSS +static: BASE_CFLAGS += -static static: clean all seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS) @@ -102,29 +108,30 @@ seccomp_pesto.h: seccomp.sh $(PESTO_SRCS) @ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_pesto.h $(PESTO_SRCS) $(BASEBIN): %: - $(CC) $(FLAGS) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^) -o $@ + $(CC) $(BASE_CPPFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^) -o $@ passt: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h -passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops +passt.avx2: BASE_CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops passt.avx2: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h pasta.avx2 pasta.1 pasta: pasta%: passt% ln -sf $< $@ -qrap: FLAGS += -DARCH=\"$(TARGET_ARCH)\" +qrap: BASE_CPPFLAGS += -DARCH=\"$(TARGET_ARCH)\" qrap: $(QRAP_SRCS) $(QRAP_HEADERS) passt-repair: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h -pesto: FLAGS += -DPESTO +pesto: BASE_CPPFLAGS += -DPESTO pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h 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: BASE_CPPFLAGS += -DVALGRIND +valgrind: BASE_CFLAGS += -g valgrind: all .PHONY: clean @@ -187,7 +194,8 @@ CLANG_TIDY = clang-tidy CLANG_TIDY_FLAGS = -DCLANG_TIDY_58992 clang-tidy: $(PASST_SRCS) - $(CLANG_TIDY) $^ -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \ + $(CLANG_TIDY) $^ -- \ + $(filter-out -pie,$(BASE_CPPFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) $(CFLAGS)) \ $(CLANG_TIDY_FLAGS) CPPCHECK = cppcheck @@ -205,5 +213,5 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force \ cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h $(CPPCHECK) $(CPPCHECK_FLAGS) \ - $(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) $^ \ + $(filter -D%,$(BASE_CPPFLAGS) $(CPPFLAGS) $(BASE_CFLAGS) $(CFLAGS)) $^ \ $^ -- 2.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 08/12] Makefile: Split static checker targets

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 --- Makefile | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index c6dbbc67..36a2654b 100644 --- a/Makefile +++ b/Makefile @@ -193,8 +193,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 \ @@ -209,5 +214,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.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 05/12] Makefile: Add header dependencies for secondary binaries

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 --- Makefile | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index ff442fa2..a8f8d06e 100644 --- a/Makefile +++ b/Makefile @@ -57,6 +57,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 \nint main(){int a=getrandom(0, 0, 0);} ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) @@ -93,7 +95,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) seccomp_pesto.h: seccomp.sh $(PESTO_SRCS) @@ -111,9 +113,9 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% ln -sf $< $@ qrap: FLAGS += -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 pesto: FLAGS += -DPESTO pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h -- 2.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 12/12] pesto: Run static checkers on pesto sources

Update the Makefile to run both clang-tidy and cppcheck on pesto as well as on passt and passt-repair. This requires a couple of secondary corrections: * pesto.c had an inline suppression that is no longer correct now that the protocol version has been bumped to 1. Remove it. * We were globally suppressing the unusedStructMember because it hit many false positives on both passt and passt-repair. It doesn't in pesto, meaning it instead creates an unusedSuppression warning. Apply the suppression as a flag override for passt and passt-repair, instead of globally. Signed-off-by: David Gibson --- Makefile | 19 ++++++++++++++++--- pesto.c | 1 - 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 5027d587..7939bd79 100644 --- a/Makefile +++ b/Makefile @@ -193,7 +193,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 pesto.clang-tidy .PHONY: %.clang-tidy %.clang-tidy: @@ -201,6 +201,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 +pesto.clang-tidy: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h CPPCHECK = cppcheck CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force \ @@ -212,14 +213,26 @@ CPPCHECK_FLAGS = --std=c11 --error-exitcode=1 --enable=all --force \ echo ""; \ fi) \ --suppress=missingIncludeSystem \ - --suppress=unusedStructMember \ -D CPPCHECK_6936 -cppcheck: passt.cppcheck passt-repair.cppcheck +cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck .PHONY: %.cppcheck %.cppcheck: $(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^ +passt.cppcheck: BASE_CPPFLAGS += -UPESTO +passt.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h + +passt-repair.cppcheck: CPPCHECK_FLAGS += --suppress=unusedStructMember passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h + +pesto.cppcheck: BASE_CPPFLAGS += -DPESTO +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:bitmap.c +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.h +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c +pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h diff --git a/pesto.c b/pesto.c index f4d752bb..eeaf274d 100644 --- a/pesto.c +++ b/pesto.c @@ -410,7 +410,6 @@ int main(int argc, char **argv) s_version, PESTO_PROTOCOL_VERSION); } - /* cppcheck-suppress knownConditionTrueFalse */ if (!s_version) { if (PESTO_PROTOCOL_VERSION) die("Unsupported experimental server protocol"); -- 2.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 09/12] passt-repair: Split out inotify handling to its own function

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 --- 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.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 10/12] passt-repair: Simplify construction of Unix path from inotify

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 --- 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.54.0

David Gibson

7:52 a.m.

New subject: [PATCH v3 11/12] passt-repair: Run static checkers

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 --- Makefile | 6 ++++-- linux_dep.h | 2 +- passt-repair.c | 49 +++++++++++++++++++++++++------------------------ 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/Makefile b/Makefile index 36a2654b..5027d587 100644 --- a/Makefile +++ b/Makefile @@ -193,13 +193,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 \ @@ -214,10 +215,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.54.0

Stefano Brivio

13 May 13 May

9:11 a.m.

New subject: [PATCH v3 06/12] Makefile: Split $(FLAGS) into cpp and cc components

On Tue, 12 May 2026 15:52:50 +1000 David Gibson wrote:

...

The $(FLAGS) variable contains mandatory compiler flags that should not be overridden. However, it contains a mixture of flags for the preprocessor and for the compiler proper. That's causing some inconvenience for other Makefile cleanups, so split it into $(BASE_CPPFLAGS) and $(BASE_CFLAGS) variables.

Signed-off-by: David Gibson --- Makefile | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile index a8f8d06e..697f229f 100644 --- a/Makefile +++ b/Makefile @@ -30,12 +30,17 @@ ifeq ($(shell $(CC) -O2 -dM -E - < /dev/null 2>&1 | grep ' _FORTIFY_SOURCE ' > / FORTIFY_FLAG := -D_FORTIFY_SOURCE=2 endif

-FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security -FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE -FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE -FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DVERSION=\"$(VERSION)\" -FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) +# Mandatory preprocessor flags that won't be overridden with $(CPPFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE $(FORTIFY_FLAG) +BASE_CPPFLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) +BASE_CPPFLAGS += -DVERSION=\"$(VERSION)\" +BASE_CPPFLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) + +# Mandatory compiler flags that won't be overridden with $(CFLAGS) +# FIXME: Could some of these be default, rather than required? +BASE_CFLAGS := -std=c11 -pie -fPIE -O2 +BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security

This new version of the series looks good to me in general (minus potential concern reported below), and everything seems to work on Debian and Fedora, but I would still like to try things out on Alpine or Void Linux because musl might cause surprises. I haven't got to it yet. Meanwhile, regarding these FIXME comments: I think it *is* currently possible to override those flags (with different values for the same options), and overriding -D_FORTIFY_SOURCE on openSUSE (I haven't tried right now) was the initial motivation behind FLAGS. That is, the overriding role of CFLAGS seems to be preserved for these BASE_* flags as well, because $CFLAGS is given to the compiler after $BASE_CPPFLAGS, $CPPFLAGS, and $BASE_CFLAGS. So, in this sense, I would already call them "default" flags. If that's the case, I think it's fine. Otherwise we need to find another solution at least for the short term. By the way, if it helps addressing those comments at some point (I would apply anyway this series meanwhile if I don't find breakages, because not being able to run static checkers automatically on pesto is pretty nasty), out of those flags: * -D_XOPEN_SOURCE, -D_GNU_SOURCE, and -DPAGE_SIZE are strictly required to build (at least in some environments) * -D_FORTIFY_SOURCE, -pie, -fPIE are not required to build but they are critical for security * -DVERSION is not required to build but makes things confusing and issues hard to debug because the version (usually supplied by the distribution) isn't reported in logs and logs of other tools - -DDUAL_STACK_SOCKETS doesn't seem to be used anymore starting from commit b8d4fac6a2e7 ("util, pif: Replace sock_l4() with pif_sock_l4()")... was it intended, actually? - -std=c11 is strictly required to ensure we build things correctly - -O2 is optional, but dropping it (by default) might require annoying adjustments in distributions - -pedantic, -Wall, -Wextra, -Wno-format-zero-length, -Wformat-security are all optional and useful for development (including distribution development), and might be security relevant in some cases -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

11 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH v3 00/12] Improvements to static checker invocation

tags

participants (2)