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

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

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

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

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

On Wed, May 13, 2026 at 09:11:00AM +0200, Stefano Brivio wrote:

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.

Makes sense, though I'm pretty sure it will be safe. I considered having some later patches that started removing non-essential things from BASE_*FLAGS. Those would certainly have required careful testing across distros. But working out what should move became more complicated than I initially thought (that's basically the discussion later in this mail), so I postponed it.

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.

True, I was thinking "overridden" in the make variable sense, but at the actual compiler that's misleading.

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.

Right, it's kind of the subtle distinction between two types of default: default unless that specific option is overridden (BASE_CFLAGS), or default unless all the "default" options are overridden as a block (CFLAGS).

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)

Yes.

* -D_FORTIFY_SOURCE, -pie, -fPIE are not required to build but they are critical for security

Right. I'd kind of consider -pie and -fPIE in a different category than "flags", since they're choosing the kind of output object we're generating.

* -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

Right. I guess I'd also consider it mandatory. The fact that you can sort of get away without it seems more an accident of how we include it, rather than policy.

- -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?

Huh, good point. I'm not sure it was exactly intentional. I was certainly grappling with the fact that in a sense it sometimes wanted to return two sockets in the !DUAL_STACK_SOCKETS case was really awkward. My plan, as best I recall was to move the handling of that case up layers, until it reached what would eventually become the forward table - so separate forwarding entries for v4 and v6 if we don't have dual stack sockets. But, since we introduced DUAL_STACK_SOCKETS, I've discovered that the dual stack socket interface is described by RFC and supported by BSD and Windows as well as Linux. I'm pretty happy killing this define.

- -std=c11 is strictly required to ensure we build things correctly

Huh, is it? I would have thought it just catches us from using any C23 or GNU extensions.

- -O2 is optional, but dropping it (by default) might require annoying adjustments in distributions

Right. But on the other hand, overriding "all" packages with the distro preferred optimization flags is one of the main uses for distro CFLAGS overrides. Not sure what to do about this one.

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

They're security relevant in the sense that it's good for us to get the warnings to catch possible security bugs. But a tree that builds clean with the flags shouldn't have different security properties if it's built without them. So I think it only matters that *we* use the flags, we don't need to impose it on anything else. But, there's an awkward gotcha here. The build.py testcase needs -Werror to catch warnings, but obviously we don't want it to have to reiterate all the other warning flags. If we put these all into CFLAGS, it's tricky for build.py to just add -Werror without touching the rest. From our point of view, I like the idea of adding -Werror to the default CFLAGS, which fixes that problem. But as you pointed out when I did that in the earlier version that might break distro builds causing use more trouble to fix it. -- 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 Thu, May 14, 2026 at 11:41:02AM +0200, Stefano Brivio wrote:

On Thu, 14 May 2026 12:01:24 +1000 David Gibson wrote:

...

On Wed, May 13, 2026 at 09:11:00AM +0200, Stefano Brivio wrote:

...

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.

Makes sense, though I'm pretty sure it will be safe. I considered having some later patches that started removing non-essential things from BASE_*FLAGS. Those would certainly have required careful testing across distros. But working out what should move became more complicated than I initially thought (that's basically the discussion later in this mail), so I postponed it.

If you plan to submit another series soon, note that I don't plan to re-do the testing I'm currently doing, though, so you should plan to do those yourself and work with maintainers if things break, in case.

Understood, but I don't plan to resume this particularly soon.

...

...

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.

True, I was thinking "overridden" in the make variable sense, but at the actual compiler that's misleading.

...

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.

Right, it's kind of the subtle distinction between two types of default: default unless that specific option is overridden (BASE_CFLAGS), or default unless all the "default" options are overridden as a block (CFLAGS).

...

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)

Yes.

...

* -D_FORTIFY_SOURCE, -pie, -fPIE are not required to build but they are critical for security

Right. I'd kind of consider -pie and -fPIE in a different category than "flags", since they're choosing the kind of output object we're generating.

...

* -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

Right. I guess I'd also consider it mandatory. The fact that you can sort of get away without it seems more an accident of how we include it, rather than policy.

...

- -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?

Huh, good point. I'm not sure it was exactly intentional. I was certainly grappling with the fact that in a sense it sometimes wanted to return two sockets in the !DUAL_STACK_SOCKETS case was really awkward. My plan, as best I recall was to move the handling of that case up layers, until it reached what would eventually become the forward table - so separate forwarding entries for v4 and v6 if we don't have dual stack sockets.

But, since we introduced DUAL_STACK_SOCKETS, I've discovered that the dual stack socket interface is described by RFC and supported by BSD and Windows as well as Linux. I'm pretty happy killing this define.

...

- -std=c11 is strictly required to ensure we build things correctly

Huh, is it? I would have thought it just catches us from using any C23 or GNU extensions.

It is because, at least in theory, while some constructs we use might be valid C99 or C17, we don't have guarantees that they have the same semantics. I don't see it happening in practice but I wouldn't risk it.

I mean... the C committee has done some weird stuff, but changing the behaviour of existing valid constructs seems well beyond the pale.

Also mind that building with -std=c99 doesn't work (anymore).

Certainly - at this point it's a constraint that CC refer to a C11 compiler, not just a C compiler.

...

...

- -O2 is optional, but dropping it (by default) might require annoying adjustments in distributions

Right. But on the other hand, overriding "all" packages with the distro preferred optimization flags is one of the main uses for distro CFLAGS overrides. Not sure what to do about this one.

Leave it like it is because it works as intended. It's our default which is also the default for most distributions, and Gentoo users (it's pretty much about them) can still override it by passing -O3 later.

Yeah, ok.

...

...

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

They're security relevant in the sense that it's good for us to get the warnings to catch possible security bugs. But a tree that builds clean with the flags shouldn't have different security properties if it's built without them. So I think it only matters that *we* use the flags, we don't need to impose it on anything else.

See above: "(including distribution development)". If those aren't there, we won't get some warnings from building distribution packages, and a few of those were useful in the past.

...

But, there's an awkward gotcha here. The build.py testcase needs -Werror to catch warnings, but obviously we don't want it to have to reiterate all the other warning flags. If we put these all into CFLAGS, it's tricky for build.py to just add -Werror without touching the rest.

My whole point was / is that CFLAGS needs to *add* to the existing flags, including warnings, instead of replacing them. You can override warnings flags too by appending options.

...

From our point of view, I like the idea of adding -Werror to the default CFLAGS, which fixes that problem.

At the moment there's no problem though.

...

But as you pointed out when I did that in the earlier version that might break distro builds causing use more trouble to fix it.

That *would* break distribution builds at the moment, so it's not acceptable. I constantly have fixes for warnings I spot in the packages I build on my to-do list.

Yeah, fair points. -- 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, 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

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

Trailing whitespace I fixed up on merge. -- Stefano