[PATCH v2 00/13] Improvements to static checker invocation
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. v2: - Fixed nasty test failure in test/build/build.py 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 ++++++----- test/build/build.py | 4 +- 5 files changed, 190 insertions(+), 135 deletions(-) -- 2.53.0
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
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
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
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
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
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
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 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_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
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
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
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
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
On Tue, 21 Apr 2026 13:23:29 +1000
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
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now. -- Stefano
On Tue, 21 Apr 2026 13:23:38 +1000
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.
This is actually for qrap only, so I would drop it. See also 8346216c9adf ("Makefile: Simplify exclusion of qrap from static checks") and 988a4d75f894 ("Makefile: Exclude qrap.c from clang-tidy checks") It's not really something we maintain and the next commit touching qrap should really remove the whole thing instead. The "soon" in 988a4d75f894 was almost two years ago. My worry is that if we enable static checkers here we risk having to waste time on warnings in the near future. Other than this and the comment to 4/13 the series looks good to me, and I'm basing v6 of "RFC: Dynamic configuration update implementation" on it. -- Stefano
On Tue, Apr 28, 2026 at 09:17:38AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:38 +1000 David Gibson
wrote: Run the static checkers on qrap as well as on passt-repair and passt. This shows a number of minor warnings, which we fix.
This is actually for qrap only, so I would drop it. See also 8346216c9adf ("Makefile: Simplify exclusion of qrap from static checks") and 988a4d75f894 ("Makefile: Exclude qrap.c from clang-tidy checks")
Ah, yeah, leftover comment from an earlier draft that handled qrap and passt-repair in the same patch.
It's not really something we maintain and the next commit touching qrap should really remove the whole thing instead. The "soon" in 988a4d75f894 was almost two years ago.
My worry is that if we enable static checkers here we risk having to waste time on warnings in the near future.
Yeah, that's fair. Let's drop this one.
Other than this and the comment to 4/13 the series looks good to me, and I'm basing v6 of "RFC: Dynamic configuration update implementation" on it.
-- Stefano
-- 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
On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here. -- 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
On Wed, Apr 29, 2026 at 01:47:13PM +1000, David Gibson wrote:
On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here.
Sorry, to cclarify. Just leaving build.py as is doesn't work because with the other changes in thie patch, setting CFLAGS here now replaces the default CFLAGS. That removes -O, but the default CPPFLAGS still has -D_FORTIFY_SOURCE, which causes warnings if optimization is off. So, we could also fix this by adding -O<something> to the build.py options or in several other ways. I think using -Werror by default is useful in other ways, and I think it makes some sense for build.py to check that things build using the default options. -- 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
On Wed, 29 Apr 2026 13:47:13 +1000
David Gibson
On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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.
I was about to add CFLAGS back to test/build/build.py (see below), but then I realised that this patch actually defeats the purpose of FLAGS, see commit 512f5b1aab2a ("Makefile: Allow define overrides by prepending, not appending, CFLAGS") for the actual reason behind it. That is, with this patch:
Replace it with the more conventional CFLAGS, which now *can* be reasonable overridden from the command line.
...passing CFLAGS completely overrides the default CFLAGS (instead of prepending them, that is, overriding single existing options). So you can't practically pass (add) "-Werror" anymore, as that will drop stuff like -std=c11, which means one can't use -Werror in build tests. It also drops bits like -pie -fPIE, so distributions (e.g. openSUSE) can't use that to override FORTIFY_SOURCE. When I committed 512f5b1aab2a, I realised that, with the "prepending" semantics, there's no way to completely override / clear CFLAGS, but: - it doesn't seem like a common use case anyway - what passing CFLAGS from the command line does isn't really standardised, and it seems to be common to use it as "extra" CFLAGS Now, it would be possible to introduce something like EXTRA_CFLAGS and ask users and distributions to switch to it, but it needs to happen in two steps: - add EXTRA_CFLAGS and ask distribution maintainers to switch to that - after a reasonable amount of time (I would say six months or so) make command-line CFLAGS replace the default CFLAGS I don't really see a strong motivation to do this though, at the moment.
Signed-off-by: David Gibson
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here.
That's not really doable as it would occasionally break distributions, where specific toolchain or architecture combinations lead quite often to harmless warnings. I can remember dozens of those, and not a single one that was actually a critical problem that made it preferable to have missing packages. It's also very annoying for developers, especially for myself as I often have to run quick tests with different compilers. I would rather add -Werror back to test/build/build.py for the moment being by dropping this patch, and then drop patches that non-trivially depend on this one (due to lack of time, not because I have anything against the other patches, which look good to me except for 13/13). That is, I would reduce this series to the bare minimum that's needed for the "RFC: Dynamic configuration update implementation" series, to avoid blocking progress there. I haven't quite figured out how to do that yet, but that's next on my list unless you get to that first. -- Stefano
On Sun, May 03, 2026 at 11:56:47PM +0200, Stefano Brivio wrote:
On Wed, 29 Apr 2026 13:47:13 +1000 David Gibson
wrote: On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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.
I was about to add CFLAGS back to test/build/build.py (see below), but then I realised that this patch actually defeats the purpose of FLAGS, see commit 512f5b1aab2a ("Makefile: Allow define overrides by prepending, not appending, CFLAGS") for the actual reason behind it.
That is, with this patch:
Replace it with the more conventional CFLAGS, which now *can* be reasonable overridden from the command line.
...passing CFLAGS completely overrides the default CFLAGS (instead of prepending them, that is, overriding single existing options).
Yes, that was intentional. Fully overriding CFLAGS isn't always the most convenient, but it is how CFLAGS conventionally works in Makefiles, so least surprise and all that.
So you can't practically pass (add) "-Werror" anymore, as that will drop stuff like -std=c11, which means one can't use -Werror in build tests.
Right, that's the most awkward consequence that I've spotted. For this case we can neutralize that by including -Werror by default, which I think is worthwhile anyway.
It also drops bits like -pie -fPIE, so distributions (e.g. openSUSE) can't use that to override FORTIFY_SOURCE.
Hm, ok. I guess I'm not really sure what the purpose of the -pie -fPIE is, so I'm not clear whether it's essential for the build, or just a sensible default that users/distros could have reason to override. I didn't realise it was connected to FORTIFY_SOURCE.
When I committed 512f5b1aab2a, I realised that, with the "prepending" semantics, there's no way to completely override / clear CFLAGS, but:
Right. I think gcc is "last option wins".
- it doesn't seem like a common use case anyway
Maybe?
- what passing CFLAGS from the command line does isn't really standardised, and it seems to be common to use it as "extra" CFLAGS
Huh.. ok. That's the opposite of the impression I've generally had. Although that said I guess "essential" compiler flags aren't usually in CFLAGS, which could be interpreted as extra flags.
Now, it would be possible to introduce something like EXTRA_CFLAGS and ask users and distributions to switch to it, but it needs to happen in two steps:
Actually, I'd suggest inverting the pattern. Put vital flags in another variable (say BASE_CFLAGS) and have the default CFLAGS contain only flags that are a suggested default build, but can be safely overridden. This is the same pattern I've used for BASE_CPPFLAGS/CPPFLAGS in the earlier patches. I didn't implement that in this patch originally, because I hadn't recognized that anything in FLAGS (other than the -D options I already moved to BASE_CPPFLAGS) was virtal in that sense. But it sounds like I was incorrect about that, so we can change accordingly.
- add EXTRA_CFLAGS and ask distribution maintainers to switch to that
- after a reasonable amount of time (I would say six months or so) make command-line CFLAGS replace the default CFLAGS
Splitting BASE_CPPFLAGS/CFLAGS instead of CFLAGS/EXTRA_CFLAGS should make that transitions significantly less awkward.
I don't really see a strong motivation to do this though, at the moment.
Signed-off-by: David Gibson
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here.
That's not really doable as it would occasionally break distributions, where specific toolchain or architecture combinations lead quite often to harmless warnings. I can remember dozens of those, and not a single one that was actually a critical problem that made it preferable to have missing packages.
Ah, right. And those distributions don't already override CFLAGS with something that doesn't include -Werror?
It's also very annoying for developers, especially for myself as I often have to run quick tests with different compilers.
Ok, fair point.
I would rather add -Werror back to test/build/build.py for the moment being by dropping this patch, and then drop patches that non-trivially depend on this one (due to lack of time, not because I have anything against the other patches, which look good to me except for 13/13).
That is, I would reduce this series to the bare minimum that's needed for the "RFC: Dynamic configuration update implementation" series, to avoid blocking progress there. I haven't quite figured out how to do that yet, but that's next on my list unless you get to that first.
I'm still unwell, so unlikely. -- 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
On Mon, 4 May 2026 14:47:05 +1000
David Gibson
On Sun, May 03, 2026 at 11:56:47PM +0200, Stefano Brivio wrote:
On Wed, 29 Apr 2026 13:47:13 +1000 David Gibson
wrote: On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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.
I was about to add CFLAGS back to test/build/build.py (see below), but then I realised that this patch actually defeats the purpose of FLAGS, see commit 512f5b1aab2a ("Makefile: Allow define overrides by prepending, not appending, CFLAGS") for the actual reason behind it.
That is, with this patch:
Replace it with the more conventional CFLAGS, which now *can* be reasonable overridden from the command line.
...passing CFLAGS completely overrides the default CFLAGS (instead of prepending them, that is, overriding single existing options).
Yes, that was intentional. Fully overriding CFLAGS isn't always the most convenient, but it is how CFLAGS conventionally works in Makefiles, so least surprise and all that.
So you can't practically pass (add) "-Werror" anymore, as that will drop stuff like -std=c11, which means one can't use -Werror in build tests.
Right, that's the most awkward consequence that I've spotted. For this case we can neutralize that by including -Werror by default, which I think is worthwhile anyway.
...except we can't do that because of the other reasons I mentioned.
It also drops bits like -pie -fPIE, so distributions (e.g. openSUSE) can't use that to override FORTIFY_SOURCE.
Hm, ok. I guess I'm not really sure what the purpose of the -pie -fPIE is, so I'm not clear whether it's essential for the build, or just a sensible default that users/distros could have reason to override.
It's essential for security as it enables usage of ASLR (address space layout randomization) and most distributions turn it on by default (or require it in guidelines these days). We want to avoid that some distributions might miss that by mistake.
I didn't realise it was connected to FORTIFY_SOURCE.
It's not directly connected, but if openSUSE just needs to redefine FORTIFY_SOURCE, then they can't just add it by passing CFLAGS, because that would also disable -pie -fPIE, plus whatever flag we want to have on for security reasons that are independent from a specific distribution.
When I committed 512f5b1aab2a, I realised that, with the "prepending" semantics, there's no way to completely override / clear CFLAGS, but:
Right. I think gcc is "last option wins".
Yes, as described in that commit (same for Clang).
- it doesn't seem like a common use case anyway
Maybe?
- what passing CFLAGS from the command line does isn't really standardised, and it seems to be common to use it as "extra" CFLAGS
Huh.. ok. That's the opposite of the impression I've generally had. Although that said I guess "essential" compiler flags aren't usually in CFLAGS, which could be interpreted as extra flags.
Maybe EXTRA_CFLAGS is more common, but we don't have it...
Now, it would be possible to introduce something like EXTRA_CFLAGS and ask users and distributions to switch to it, but it needs to happen in two steps:
Actually, I'd suggest inverting the pattern. Put vital flags in another variable (say BASE_CFLAGS) and have the default CFLAGS contain only flags that are a suggested default build, but can be safely overridden. This is the same pattern I've used for BASE_CPPFLAGS/CPPFLAGS in the earlier patches.
Ah, good idea, that would avoid bothering users and distributions. By the way, I think all current FLAGS are vital flags that should be in BASE_CFLAGS, and one can still override -O or similar by passing CFLAGS if CFLAGS is appended.
I didn't implement that in this patch originally, because I hadn't recognized that anything in FLAGS (other than the -D options I already moved to BASE_CPPFLAGS) was virtal in that sense. But it sounds like I was incorrect about that, so we can change accordingly.
- add EXTRA_CFLAGS and ask distribution maintainers to switch to that
- after a reasonable amount of time (I would say six months or so) make command-line CFLAGS replace the default CFLAGS
Splitting BASE_CPPFLAGS/CFLAGS instead of CFLAGS/EXTRA_CFLAGS should make that transitions significantly less awkward.
Right.
I don't really see a strong motivation to do this though, at the moment.
Signed-off-by: David Gibson
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here.
That's not really doable as it would occasionally break distributions, where specific toolchain or architecture combinations lead quite often to harmless warnings. I can remember dozens of those, and not a single one that was actually a critical problem that made it preferable to have missing packages.
Ah, right. And those distributions don't already override CFLAGS with something that doesn't include -Werror?
We don't have -Werror on. That's just for tests.
It's also very annoying for developers, especially for myself as I often have to run quick tests with different compilers.
Ok, fair point.
I would rather add -Werror back to test/build/build.py for the moment being by dropping this patch, and then drop patches that non-trivially depend on this one (due to lack of time, not because I have anything against the other patches, which look good to me except for 13/13).
That is, I would reduce this series to the bare minimum that's needed for the "RFC: Dynamic configuration update implementation" series, to avoid blocking progress there. I haven't quite figured out how to do that yet, but that's next on my list unless you get to that first.
I'm still unwell, so unlikely.
Maybe it can all be made simpler by keeping this series substantially as it is with the change you suggested above (essentially, BASE_CFLAGS instead of FLAGS, and CFLAGS adding on top of those). -- Stefano
On 4/21/26 05:23, David Gibson wrote:
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 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)
Perhaps we can add a reusable function to check flags: check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2)) Then $(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive) \
+ --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)) $^ \ + $^
You have duplicate '$^' here.
On Tue, 5 May 2026 12:14:02 +0200
Laurent Vivier
On 4/21/26 05:23, David Gibson wrote:
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 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)
Perhaps we can add a reusable function to check flags:
check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2))
Then $(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive) \
+ --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)) $^ \ + $^
You have duplicate '$^' here.
Just note that it's not necessarily important to review this series right now (I'm not sure). I guess if we can "fix" it (see my concerns about 4/13) quickly, then it's useful, and I'll try to pick it up like that (feel free to post a new version yourself, of course!). But if it needs major reworks I'd rather try to come up with something minimal instead, for the moment. -- Stefano
On 4/21/26 05:23, David Gibson wrote:
Our cppcheck target need certain flags from the compiler so that they it can analyse the code correctly. Currently we extract these rather
"so that it can analyze"
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
Reviewed-by: Laurent Vivier
--- 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) $^
On 5/5/26 12:20, Stefano Brivio wrote:
On Tue, 5 May 2026 12:14:02 +0200 Laurent Vivier
wrote: On 4/21/26 05:23, David Gibson wrote:
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 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)
Perhaps we can add a reusable function to check flags:
check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2))
Then $(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive) \
+ --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)) $^ \ + $^
You have duplicate '$^' here.
Just note that it's not necessarily important to review this series right now (I'm not sure).
okay, I was reviewing it because "Dynamic configuration" series doesn't apply cleanly without it.
I guess if we can "fix" it (see my concerns about 4/13) quickly, then it's useful, and I'll try to pick it up like that (feel free to post a new version yourself, of course!).
But if it needs major reworks I'd rather try to come up with something minimal instead, for the moment.
Thanks, Laurent
On Tue, 5 May 2026 12:49:11 +0200
Laurent Vivier
On 5/5/26 12:20, Stefano Brivio wrote:
On Tue, 5 May 2026 12:14:02 +0200 Laurent Vivier
wrote: On 4/21/26 05:23, David Gibson wrote:
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 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)
Perhaps we can add a reusable function to check flags:
check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2))
Then $(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive) \
+ --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)) $^ \ + $^
You have duplicate '$^' here.
Just note that it's not necessarily important to review this series right now (I'm not sure).
okay, I was reviewing it because "Dynamic configuration" series doesn't apply cleanly without it.
Yes, absolutely, and we can't run static checkers on pesto without it, so we need something like this series. If we (especially: you ;)) can fix whatever comment you have plus my concern on 4/13 quickly enough, it makes sense to continue with it. If it becomes a big effort maybe we should go for a less involved approach though. I'm not sure. -- Stefano
On Tue, May 05, 2026 at 01:10:30AM +0200, Stefano Brivio wrote:
On Mon, 4 May 2026 14:47:05 +1000 David Gibson
wrote: On Sun, May 03, 2026 at 11:56:47PM +0200, Stefano Brivio wrote:
On Wed, 29 Apr 2026 13:47:13 +1000 David Gibson
wrote: On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:29 +1000 David Gibson
wrote: 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.
I was about to add CFLAGS back to test/build/build.py (see below), but then I realised that this patch actually defeats the purpose of FLAGS, see commit 512f5b1aab2a ("Makefile: Allow define overrides by prepending, not appending, CFLAGS") for the actual reason behind it.
That is, with this patch:
Replace it with the more conventional CFLAGS, which now *can* be reasonable overridden from the command line.
...passing CFLAGS completely overrides the default CFLAGS (instead of prepending them, that is, overriding single existing options).
Yes, that was intentional. Fully overriding CFLAGS isn't always the most convenient, but it is how CFLAGS conventionally works in Makefiles, so least surprise and all that.
So you can't practically pass (add) "-Werror" anymore, as that will drop stuff like -std=c11, which means one can't use -Werror in build tests.
Right, that's the most awkward consequence that I've spotted. For this case we can neutralize that by including -Werror by default, which I think is worthwhile anyway.
...except we can't do that because of the other reasons I mentioned.
Yeah, sorry, I hadn't read that far when I wrote this.
It also drops bits like -pie -fPIE, so distributions (e.g. openSUSE) can't use that to override FORTIFY_SOURCE.
Hm, ok. I guess I'm not really sure what the purpose of the -pie -fPIE is, so I'm not clear whether it's essential for the build, or just a sensible default that users/distros could have reason to override.
It's essential for security as it enables usage of ASLR (address space layout randomization) and most distributions turn it on by default (or require it in guidelines these days).
We want to avoid that some distributions might miss that by mistake.
Ah, ok. Yeah, that sounds essential to me. I think you could also make the argument that -pie is about the type of output we want, more than a "flag" per se, so it doesn't belong in CFLAGS any more than -c or -o would. -fPIE follows as a requirement for -pie.
I didn't realise it was connected to FORTIFY_SOURCE.
It's not directly connected, but if openSUSE just needs to redefine FORTIFY_SOURCE, then they can't just add it by passing CFLAGS, because that would also disable -pie -fPIE, plus whatever flag we want to have on for security reasons that are independent from a specific distribution.
Ok, I see.
When I committed 512f5b1aab2a, I realised that, with the "prepending" semantics, there's no way to completely override / clear CFLAGS, but:
Right. I think gcc is "last option wins".
Yes, as described in that commit (same for Clang).
Ah, yes. I think som of the $(filter) and $(filter-out) confused be in terms of how FLAGS and CFLAGS interact(ed).
- it doesn't seem like a common use case anyway
Maybe?
- what passing CFLAGS from the command line does isn't really standardised, and it seems to be common to use it as "extra" CFLAGS
Huh.. ok. That's the opposite of the impression I've generally had. Although that said I guess "essential" compiler flags aren't usually in CFLAGS, which could be interpreted as extra flags.
Maybe EXTRA_CFLAGS is more common, but we don't have it...
I don't recall EXTRA_CFLAGS being particularly standard, but I'm not very certain.
Now, it would be possible to introduce something like EXTRA_CFLAGS and ask users and distributions to switch to it, but it needs to happen in two steps:
Actually, I'd suggest inverting the pattern. Put vital flags in another variable (say BASE_CFLAGS) and have the default CFLAGS contain only flags that are a suggested default build, but can be safely overridden. This is the same pattern I've used for BASE_CPPFLAGS/CPPFLAGS in the earlier patches.
Ah, good idea, that would avoid bothering users and distributions. By the way, I think all current FLAGS are vital flags that should be in BASE_CFLAGS,
I'm not really convinced for -Owhatever, -pedantic, -fstack-protectr-strong or the warning options.
and one can still override -O or similar by passing CFLAGS if CFLAGS is appended.
It kind of depends depends what the caller is expecting. Do they expect omitting -Oxyz from CFLAGS to imply -O0, or "project default"?
I didn't implement that in this patch originally, because I hadn't recognized that anything in FLAGS (other than the -D options I already moved to BASE_CPPFLAGS) was virtal in that sense. But it sounds like I was incorrect about that, so we can change accordingly.
- add EXTRA_CFLAGS and ask distribution maintainers to switch to that
- after a reasonable amount of time (I would say six months or so) make command-line CFLAGS replace the default CFLAGS
Splitting BASE_CPPFLAGS/CFLAGS instead of CFLAGS/EXTRA_CFLAGS should make that transitions significantly less awkward.
Right.
Details about what belongs in BASE_CFLAGS versus CFLAGS can be thrashed out later. The vital thing for this series is to split $(FLAGS) into preprocessor versus compiler flags (with clearer names for both). That's what we need in order to properly invoke the static checkers for both passt and pesto (more particularly, cppcheck needs -DPESTO with pesto and -UPESTO with passt, or log.h will give it conniptions).
I don't really see a strong motivation to do this though, at the moment.
Signed-off-by: David Gibson
--- Makefile | 21 ++++++++++----------- test/build/build.py | 4 ++-- 2 files changed, 12 insertions(+), 13 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
diff --git a/test/build/build.py b/test/build/build.py index e3de8305..7c9cbb44 100755 --- a/test/build/build.py +++ b/test/build/build.py @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None: with clone_sources(): for p in ex_paths: assert not p.exists(), f"{p} existed before make" - sh(f'make {target} CFLAGS="-Werror"') + sh(f'make {target}') for p in ex_paths: assert p.exists(), f"{p} wasn't made" sh('make clean') @@ -90,7 +90,7 @@ def test_install_uninstall() -> None: progs = ['passt', 'pasta', 'qrap']
# Install - sh(f'make install CFLAGS="-Werror" prefix={prefix}') + sh(f'make install prefix={prefix}')
Here, and above: I don't understand what (if anything) implies -Werror now.
Ah, oops. I misread the -Werror in test/Makefile, thinking it was in Makefile and applied to everything. I think the right fix is to put -Werror in the default CFLAGS and remove it from here.
That's not really doable as it would occasionally break distributions, where specific toolchain or architecture combinations lead quite often to harmless warnings. I can remember dozens of those, and not a single one that was actually a critical problem that made it preferable to have missing packages.
Ah, right. And those distributions don't already override CFLAGS with something that doesn't include -Werror?
We don't have -Werror on. That's just for tests.
It's also very annoying for developers, especially for myself as I often have to run quick tests with different compilers.
Ok, fair point.
I would rather add -Werror back to test/build/build.py for the moment being by dropping this patch, and then drop patches that non-trivially depend on this one (due to lack of time, not because I have anything against the other patches, which look good to me except for 13/13).
That is, I would reduce this series to the bare minimum that's needed for the "RFC: Dynamic configuration update implementation" series, to avoid blocking progress there. I haven't quite figured out how to do that yet, but that's next on my list unless you get to that first.
I'm still unwell, so unlikely.
Maybe it can all be made simpler by keeping this series substantially as it is with the change you suggested above (essentially, BASE_CFLAGS instead of FLAGS, and CFLAGS adding on top of those).
Right, sounds basically the same as my suggestion above. -- 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
On Tue, May 05, 2026 at 12:14:02PM +0200, Laurent Vivier wrote:
On 4/21/26 05:23, David Gibson wrote:
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 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)
Perhaps we can add a reusable function to check flags:
check_flag = $(shell $(1) $(2) >/dev/null 2>&1 && echo $(2))
Then $(call check_flag, $(CPPCHECK) /dev/null, --check-level=exhaustive)
We could, but that seems orthogonal to what I'm trying to accomplish here.
+ --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)) $^ \ + $^
You have duplicate '$^' here.
Oops, that's a bug. -- 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
On Tue, 21 Apr 2026 13:23:27 +1000
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
--- 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
I tried a bit harder but this distinction looks bogus to me (we must *not* build without -std=c11, FORTIFY_SOURCE, -pie, -fPIE, or DUAL_STACK_SOCKETS anyway) and adapting the whole series to a BASE_CPPFLAGS / CPPFLAGS / CFLAGS split is rather time consuming, even if I drop unrelated patches such as 5/13 to 8/13 and 10/13 to 13/13, so I would drop this series for now. I'm running static checkers on pesto manually for the moment. Note that the rationale given for 3/13 and 4/13 ignores documented reasons behind the current sets of flags. It can be changed indeed but functionality needs to be maintained, as I already mentioned in the discussion about 4/13.
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) $^
-- Stefano
On Wed, May 06, 2026 at 09:46:51AM +0200, Stefano Brivio wrote:
On Tue, 21 Apr 2026 13:23:27 +1000 David Gibson
wrote: 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
--- 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
I tried a bit harder but this distinction looks bogus to me (we must
I'm not sure which distinction you're referring to. There are two options AFAICT: the distinction between "required" and "default only" flags. That one I'll grant is a bit tenuous. Mind you, that's kind of already the split between FLAGS and CFLAGS. The other is the distinction between flags for the preprocessor versus for the compiler proper. That's the one I actually need to do what this series is aiming to do.
*not* build without -std=c11, FORTIFY_SOURCE, -pie, -fPIE, or DUAL_STACK_SOCKETS anyway)
Agreed for -pie and -fPIE. Maybe for -std=c11. FORTIFY_SOURCE seems like something we want to encourage, but by no means essential to the build. DUAL_STACK_SOCKETS seems like it should be configurable. Omitting when possible will waste memory, but should not result in a failed or incorrect build.
and adapting the whole series to a BASE_CPPFLAGS / CPPFLAGS / CFLAGS split is rather time consuming, even if I drop unrelated patches such as 5/13 to 8/13 and 10/13 to 13/13, so I would drop this series for now.
I'm running static checkers on pesto manually for the moment.
Eh, ok. I'll rework on top of whatever once I'm back.
Note that the rationale given for 3/13 and 4/13 ignores documented reasons behind the current sets of flags. It can be changed indeed but functionality needs to be maintained, as I already mentioned in the discussion about 4/13.
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) $^
-- Stefano
-- 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
participants (3)
-
David Gibson
-
Laurent Vivier
-
Stefano Brivio