These are not directly apropose anything, but I encountered a number
of minor uglies in the Makefiles while I was working on other stuff,
so heres a bunch of cleanups.
David Gibson (6):
Makefile: Avoid using wildcard sources
Makefile: Use $(BIN) and $(MANPAGES) variable to simplify several
targets
Makefile: Simplify pasta* targets with a pattern rule
Makefile: Tweak $(RM) usage
Makefile: Don't create extraneous -.s file
Makefile: Spell prefix as PREFIX
Makefile | 77 +++++++++++++++++++++++++++---------------------------
seccomp.sh | 5 ++--
2 files changed, 41 insertions(+), 41 deletions(-)
--
2.36.1
The passt/pasta Makefile makes fairly heavy use of GNU make's $(wildcard) function to locate the sources and headers to build. Using wildcards for the things to compile is usually a bad idea though: if somehow you end up with a .c or .h file in your tree you didn't expect it can misbuild in an exceedingly confusing way. In particular this can sometimes happen if switching between releases / branches where files have been added or removed without 100% cleaning the tree. It also makes life a bit complicated if building multiple different binaries in the same tree: we already have some rather awkward $(filter-out) constructions to avoid including qrap.c in the passt build. Replace use of $(wildcard) with the more idiomatic approach of defining variables listing all the relevant source files then using that throughout. In the rule for seccomp.h there was also a bare "*.c" which caused make to always rebuild that target. Fix that as well. Similarly, seccomp.sh uses a wildcard to locate the sources, which is unwise for the same reasons. Make it take the sources to examine on the command line instead, and have the Makefile pass them in from the same variables. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- Makefile | 37 ++++++++++++++++++++++--------------- seccomp.sh | 5 +++-- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 9f2ec3a..f7ca3ef 100644 --- a/Makefile +++ b/Makefile @@ -31,6 +31,17 @@ CFLAGS += -DPASST_AUDIT_ARCH=AUDIT_ARCH_$(AUDIT_ARCH) CFLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL) CFLAGS += -DARCH=\"$(TARGET_ARCH)\" +PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \ + mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \ + tap.c tcp.c tcp_splice.c udp.c util.c +QRAP_SRCS = qrap.c +SRCS = $(PASST_SRCS) $(QRAP_SRCS) + +PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \ + ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \ + tap.h tcp.h tcp_splice.h udp.h util.h +HEADERS = $(PASST_HEADERS) + # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined, # seem to be hitting something similar to: # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78993 @@ -82,18 +93,15 @@ endif static: CFLAGS += -static -DGLIBC_NO_STATIC_NSS static: clean all -seccomp.h: *.c $(filter-out seccomp.h,$(wildcard *.h)) - @ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh +seccomp.h: $(PASST_SRCS) $(PASST_HEADERS) + @ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh $^ -passt: $(filter-out qrap.c,$(wildcard *.c)) \ - $(filter-out qrap.h,$(wildcard *.h)) seccomp.h - $(CC) $(CFLAGS) $(filter-out qrap.c,$(wildcard *.c)) -o passt +passt: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h + $(CC) $(CFLAGS) $(PASST_SRCS) -o passt passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops -passt.avx2: $(filter-out qrap.c,$(wildcard *.c)) \ - $(filter-out qrap.h,$(wildcard *.h)) seccomp.h - $(CC) $(filter-out -O2,$(CFLAGS)) $(filter-out qrap.c,$(wildcard *.c)) \ - -o passt.avx2 +passt.avx2: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h + $(CC) $(filter-out -O2,$(CFLAGS)) $(PASST_SRCS) -o passt.avx2 passt.avx2: passt @@ -104,9 +112,8 @@ pasta: passt ln -s passt pasta ln -s passt.1 pasta.1 -qrap: qrap.c passt.h - $(CC) $(CFLAGS) \ - qrap.c -o qrap +qrap: $(QRAP_SRCS) passt.h + $(CC) $(CFLAGS) $(QRAP_SRCS) -o qrap valgrind: EXTRA_SYSCALLS="rt_sigprocmask rt_sigtimedwait rt_sigaction \ getpid gettid kill clock_gettime mmap munmap open \ @@ -203,7 +210,7 @@ pkgs: static # - concurrency-mt-unsafe # TODO: check again if multithreading is implemented -clang-tidy: $(wildcard *.c) $(wildcard *.h) +clang-tidy: $(SRCS) $(HEADERS) clang-tidy -checks=*,-modernize-*,\ -clang-analyzer-valist.Uninitialized,\ -cppcoreguidelines-init-variables,\ @@ -227,7 +234,7 @@ clang-tidy: $(wildcard *.c) $(wildcard *.h) -altera-struct-pack-align,\ -concurrency-mt-unsafe \ -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \ - --warnings-as-errors=* $(wildcard *.c) -- $(filter-out -pie,$(CFLAGS)) + --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(CFLAGS)) ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1) TARGET := $(shell ${CC} -v 2>&1 | sed -n 's/Target: \(.*\)/\1/p') @@ -237,7 +244,7 @@ EXTRA_INCLUDES_OPT := -I$(EXTRA_INCLUDES) else EXTRA_INCLUDES_OPT := endif -cppcheck: $(wildcard *.c) $(wildcard *.h) +cppcheck: $(SRCS) $(HEADERS) cppcheck --std=c99 --error-exitcode=1 --enable=all --force \ --inconclusive --library=posix \ -I/usr/include $(EXTRA_INCLUDES_OPT) \ diff --git a/seccomp.sh b/seccomp.sh index 74eeb4b..17def4d 100755 --- a/seccomp.sh +++ b/seccomp.sh @@ -14,6 +14,7 @@ # Author: Stefano Brivio <sbrivio(a)redhat.com> TMP="$(mktemp)" +IN="$@" OUT="seccomp.h" HEADER="/* This file was automatically generated by $(basename ${0}) */ @@ -231,9 +232,9 @@ gen_profile() { } printf '%s\n' "${HEADER}" > "${OUT}" -__profiles="$(sed -n 's/[\t ]*\*[\t ]*#syscalls:\([^ ]*\).*/\1/p' *.[ch] | sort -u)" +__profiles="$(sed -n 's/[\t ]*\*[\t ]*#syscalls:\([^ ]*\).*/\1/p' ${IN} | sort -u)" for __p in ${__profiles}; do - __calls="$(sed -n 's/[\t ]*\*[\t ]*#syscalls\(:'"${__p}"'\|\)[\t ]\{1,\}\(.*\)/\2/p' *.[ch])" + __calls="$(sed -n 's/[\t ]*\*[\t ]*#syscalls\(:'"${__p}"'\|\)[\t ]\{1,\}\(.*\)/\2/p' ${IN})" __calls="${__calls} ${EXTRA_SYSCALLS:-}" __calls="$(filter ${__calls})" echo "seccomp profile ${__p} allows: ${__calls}" | tr '\n' ' ' | fmt -t -- 2.36.1
On Tue, 14 Jun 2022 15:12:21 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:The passt/pasta Makefile makes fairly heavy use of GNU make's $(wildcard) function to locate the sources and headers to build. Using wildcards for the things to compile is usually a bad idea though: if somehow you end up with a .c or .h file in your tree you didn't expect it can misbuild in an exceedingly confusing way. In particular this can sometimes happen if switching between releases / branches where files have been added or removed without 100% cleaning the tree.Thanks for fixing this, I have to admit I hit this very issue all the time ;) -- Stefano
There are several places which explicitly list the various generated
binaries, even though a $(BIN) variable already lists them. There are
several more places that list all the manpage files, introduce a
$(MANPAGES) variable to remove that repetition as well.
Tweak the generation of pasta.1 as a link to passt.1 so it's not just made
as a side effect of the pasta target.
---
Makefile | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/Makefile b/Makefile
index f7ca3ef..88a3f47 100644
--- a/Makefile
+++ b/Makefile
@@ -37,6 +37,8 @@ PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c
igmp.c \
QRAP_SRCS = qrap.c
SRCS = $(PASST_SRCS) $(QRAP_SRCS)
+MANPAGES = passt.1 pasta.1 qrap.1
+
PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \
tap.h tcp.h tcp_splice.h udp.h util.h
@@ -83,13 +85,13 @@ endif
prefix ?= /usr/local
ifeq ($(TARGET_ARCH),X86_64)
-all: passt passt.avx2 pasta pasta.avx2 qrap
BIN := passt passt.avx2 pasta pasta.avx2 qrap
else
-all: passt pasta qrap
BIN := passt pasta qrap
endif
+all: $(BIN) $(MANPAGES)
+
static: CFLAGS += -static -DGLIBC_NO_STATIC_NSS
static: clean all
@@ -110,6 +112,8 @@ pasta.avx2: passt.avx2
pasta: passt
ln -s passt pasta
+
+pasta.1: passt.1
ln -s passt.1 pasta.1
qrap: $(QRAP_SRCS) passt.h
@@ -123,28 +127,22 @@ valgrind: all
.PHONY: clean
clean:
- -${RM} passt passt.avx2 *.o seccomp.h qrap pasta pasta.avx2 pasta.1 \
+ -${RM} $(BIN) *.o seccomp.h pasta.1 \
passt.tar passt.tar.gz *.deb *.rpm
-install: $(BIN)
+install: $(BIN) $(MANPAGES)
mkdir -p $(DESTDIR)$(prefix)/bin $(DESTDIR)$(prefix)/share/man/man1
cp -d $(BIN) $(DESTDIR)$(prefix)/bin
- cp -d passt.1 pasta.1 qrap.1 $(DESTDIR)$(prefix)/share/man/man1
+ cp -d $(MANPAGES) $(DESTDIR)$(prefix)/share/man/man1
uninstall:
- -${RM} $(DESTDIR)$(prefix)/bin/passt
- -${RM} $(DESTDIR)$(prefix)/bin/passt.avx2
- -${RM} $(DESTDIR)$(prefix)/bin/pasta
- -${RM} $(DESTDIR)$(prefix)/bin/pasta.avx2
- -${RM} $(DESTDIR)$(prefix)/bin/qrap
- -${RM} $(DESTDIR)$(prefix)/share/man/man1/passt.1
- -${RM} $(DESTDIR)$(prefix)/share/man/man1/pasta.1
- -${RM} $(DESTDIR)$(prefix)/share/man/man1/qrap.1
+ -${RM} $(BIN:%=$(DESTDIR)$(prefix)/bin/%)
+ -${RM} $(MANPAGES:%=$(DESTDIR)$(prefix)/share/man/man1/%)
pkgs: static
tar cf passt.tar -P --xform 's//\/usr\/bin\//' $(BIN)
tar rf passt.tar -P --xform 's//\/usr\/share\/man\/man1\//' \
- passt.1 pasta.1 qrap.1
+ $(MANPAGES)
gzip passt.tar
EMAIL="sbrivio(a)redhat.com" fakeroot alien --to-deb \
--description="User-mode networking for VMs and namespaces" \
--
2.36.1
On Tue, 14 Jun 2022 15:12:22 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:There are several places which explicitly list the various generated binaries, even though a $(BIN) variable already lists them. There are several more places that list all the manpage files, introduce a $(MANPAGES) variable to remove that repetition as well.This also needs (sorry ;)) diff --git a/test/distro/debian b/test/distro/debian index f748dea..239e225 100644 --- a/test/distro/debian +++ b/test/distro/debian @@ -39,7 +39,7 @@ endef hostb ./passt -P __PIDFILE__ & sleep 1 host echo -hout GUEST_FILES ls -1 *.c *.h *.sh Makefile | tr '\n' ' '; echo +hout GUEST_FILES ls -1 *.c *.h *.sh passt.1 qrap.1 Makefile | tr '\n' ' '; echo test Debian GNU/Linux 8 (jessie), amd64 diff --git a/test/distro/fedora b/test/distro/fedora index 7a5eaef..013cb45 100644 --- a/test/distro/fedora +++ b/test/distro/fedora @@ -60,7 +60,7 @@ hostb ./passt -P __PIDFILE__ & sleep 1 host echo hout DNS6 sed -n 's/^nameserver \([^:]*:\)\([^%]*\).*/\1\2/p' /etc/resolv.conf | head -1 -hout GUEST_FILES ls -1 *.c *.h *.sh Makefile | tr '\n' ' '; echo +hout GUEST_FILES ls -1 *.c *.h *.sh passt.1 qrap.1 Makefile | tr '\n' ' '; echo test Fedora 26, x86_64 diff --git a/test/distro/opensuse b/test/distro/opensuse index 39f059a..3d71c42 100644 --- a/test/distro/opensuse +++ b/test/distro/opensuse @@ -39,7 +39,7 @@ hostb ./passt -P __PIDFILE__ & sleep 1 host echo hout DNS6 sed -n 's/^nameserver \([^:]*:\)\([^%]*\).*/\1\2/p' /etc/resolv.conf | head -1 -hout GUEST_FILES ls -1 *.c *.h *.sh Makefile | tr '\n' ' '; echo +hout GUEST_FILES ls -1 *.c *.h *.sh passt.1 qrap.1 Makefile | tr '\n' ' '; echo test OpenSUSE Leap 15.1 diff --git a/test/distro/ubuntu b/test/distro/ubuntu index c9a2b4d..c3d1630 100644 --- a/test/distro/ubuntu +++ b/test/distro/ubuntu @@ -38,7 +38,7 @@ endef hostb ./passt -P __PIDFILE__ & sleep 1 host echo -hout GUEST_FILES ls -1 *.c *.h *.sh Makefile | tr '\n' ' '; echo +hout GUEST_FILES ls -1 *.c *.h *.sh passt.1 qrap.1 Makefile | tr '\n' ' '; echo test Ubuntu 14.04.5 LTS (Trusty Tahr), amd64 ...added. -- Stefano
pasta, pasta.avx2 and pasta.1 are all generated as a link to the corresponding passt file. We can consolidate the 3 rules for these targets into a single pattern rule. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- Makefile | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 88a3f47..760d458 100644 --- a/Makefile +++ b/Makefile @@ -107,14 +107,8 @@ passt.avx2: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h passt.avx2: passt -pasta.avx2: passt.avx2 - ln -s passt.avx2 pasta.avx2 - -pasta: passt - ln -s passt pasta - -pasta.1: passt.1 - ln -s passt.1 pasta.1 +pasta.avx2 pasta.1 pasta: pasta%: passt% + ln -s $< $@ qrap: $(QRAP_SRCS) passt.h $(CC) $(CFLAGS) $(QRAP_SRCS) -o qrap -- 2.36.1
The use of rm commands in the clean and uninstall targets adds an explicit
leading - to ignore errors. However the built-in RM variable in make is
actually "rm -f" which already ignores errors, so the - isn't neccessary.
Also replace ${RM} with $(RM) which is the more conventional form in
Makefiles.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 760d458..f918bb8 100644
--- a/Makefile
+++ b/Makefile
@@ -121,7 +121,7 @@ valgrind: all
.PHONY: clean
clean:
- -${RM} $(BIN) *.o seccomp.h pasta.1 \
+ $(RM) $(BIN) *.o seccomp.h pasta.1 \
passt.tar passt.tar.gz *.deb *.rpm
install: $(BIN) $(MANPAGES)
@@ -130,8 +130,8 @@ install: $(BIN) $(MANPAGES)
cp -d $(MANPAGES) $(DESTDIR)$(prefix)/share/man/man1
uninstall:
- -${RM} $(BIN:%=$(DESTDIR)$(prefix)/bin/%)
- -${RM} $(MANPAGES:%=$(DESTDIR)$(prefix)/share/man/man1/%)
+ $(RM) $(BIN:%=$(DESTDIR)$(prefix)/bin/%)
+ $(RM) $(MANPAGES:%=$(DESTDIR)$(prefix)/share/man/man1/%)
pkgs: static
tar cf passt.tar -P --xform 's//\/usr\/bin\//' $(BIN)
--
2.36.1
In order to probe availability of certain features the Makefile test compiles a handful of tiny snippets, feeding those in from stdin. However in one case - the one for -fstack-protector - it forgets to redirect the output to stdout, meaning it creates a stray '-.s' file when make is invoked (even make clean). Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f918bb8..b0dde68 100644 --- a/Makefile +++ b/Makefile @@ -78,7 +78,7 @@ ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) CFLAGS += -DHAS_GETRANDOM endif -ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - >/dev/null 2>&1; echo $$?),0) +ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0) CFLAGS += -fstack-protector-strong endif -- 2.36.1
Makefile conventions (at least in the GNUish world) tend to control the base install directory with the variable $(PREFIX) (all caps). The passt Makefile has the same thing but in lower case $(prefix). Capitalize it to match conventions. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- Makefile | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index b0dde68..c334605 100644 --- a/Makefile +++ b/Makefile @@ -82,7 +82,7 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec CFLAGS += -fstack-protector-strong endif -prefix ?= /usr/local +PREFIX ?= /usr/local ifeq ($(TARGET_ARCH),X86_64) BIN := passt passt.avx2 pasta pasta.avx2 qrap @@ -125,13 +125,13 @@ clean: passt.tar passt.tar.gz *.deb *.rpm install: $(BIN) $(MANPAGES) - mkdir -p $(DESTDIR)$(prefix)/bin $(DESTDIR)$(prefix)/share/man/man1 - cp -d $(BIN) $(DESTDIR)$(prefix)/bin - cp -d $(MANPAGES) $(DESTDIR)$(prefix)/share/man/man1 + mkdir -p $(DESTDIR)$(PREFIX)/bin $(DESTDIR)$(PREFIX)/share/man/man1 + cp -d $(BIN) $(DESTDIR)$(PREFIX)/bin + cp -d $(MANPAGES) $(DESTDIR)$(PREFIX)/share/man/man1 uninstall: - $(RM) $(BIN:%=$(DESTDIR)$(prefix)/bin/%) - $(RM) $(MANPAGES:%=$(DESTDIR)$(prefix)/share/man/man1/%) + $(RM) $(BIN:%=$(DESTDIR)$(PREFIX)/bin/%) + $(RM) $(MANPAGES:%=$(DESTDIR)$(PREFIX)/share/man/man1/%) pkgs: static tar cf passt.tar -P --xform 's//\/usr\/bin\//' $(BIN) -- 2.36.1
On Tue, 14 Jun 2022 15:12:26 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Makefile conventions (at least in the GNUish world) tend to control the base install directory with the variable $(PREFIX) (all caps). The passt Makefile has the same thing but in lower case $(prefix). Capitalize it to match conventions.I've been wondering about this when I first added it to the Makefile, and I couldn't really find a rationale for it. From the documentation of GNU make: http://www.gnu.org/software/make/manual/make.html#Directory-Variables http://www.gnu.org/software/make/manual/make.html#DESTDIR prefix is (conventionally) lowercase, DESTDIR is uppercase, so I'd stick to that. I guess there's just some historic reason behind it. Some Makefiles of long-standing projects use prefix, some PREFIX. By the way, the rest of the series all look good to me (and is very appreciated!). -- Stefano
On Tue, Jun 14, 2022 at 04:45:05PM +0200, Stefano Brivio wrote:On Tue, 14 Jun 2022 15:12:26 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Huh.. interesting. I guess the things I've looked at personally must have tended towards PREFIX. I'd assumed the GNU conventions specified that, but apparently not. Ok, let's drop this one.Makefile conventions (at least in the GNUish world) tend to control the base install directory with the variable $(PREFIX) (all caps). The passt Makefile has the same thing but in lower case $(prefix). Capitalize it to match conventions.I've been wondering about this when I first added it to the Makefile, and I couldn't really find a rationale for it. From the documentation of GNU make: http://www.gnu.org/software/make/manual/make.html#Directory-Variables http://www.gnu.org/software/make/manual/make.html#DESTDIR prefix is (conventionally) lowercase, DESTDIR is uppercase, so I'd stick to that. I guess there's just some historic reason behind it. Some Makefiles of long-standing projects use prefix, some PREFIX.By the way, the rest of the series all look good to me (and is very appreciated!).-- David Gibson | 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, 14 Jun 2022 15:12:20 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:These are not directly apropose anything, but I encountered a number of minor uglies in the Makefiles while I was working on other stuff, so heres a bunch of cleanups. David Gibson (6): Makefile: Avoid using wildcard sources Makefile: Use $(BIN) and $(MANPAGES) variable to simplify several targets Makefile: Simplify pasta* targets with a pattern rule Makefile: Tweak $(RM) usage Makefile: Don't create extraneous -.s file Makefile: Spell prefix as PREFIX Makefile | 77 +++++++++++++++++++++++++++--------------------------- seccomp.sh | 5 ++-- 2 files changed, 41 insertions(+), 41 deletions(-)Series applied, along with the previous ones, thanks! -- Stefano