From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=aQkwMoa7; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id F23CD5A0627 for ; Tue, 05 May 2026 01:10:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1777936235; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wBCEpyFpDJHMTwtRRg3SZCpV1QHG0l0VAs8/t638xpk=; b=aQkwMoa7JWz1fpgxQCr1IBV2tIioG3MaWr00InaIVyftDWoBahuokTMCZYDJpMedk2HLa1 ihytKtJUI1VVnBm5RX1AG/xFDvmpFZlOt/SH43U83qgfK4pxtAFzhORY6/sLe0efqd8i5u RSKvqYjpfzzkFFoieTAtBnKlHN+gcaw= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-668-eBVBmPnAOkKkhsldpyc83w-1; Mon, 04 May 2026 19:10:34 -0400 X-MC-Unique: eBVBmPnAOkKkhsldpyc83w-1 X-Mimecast-MFC-AGG-ID: eBVBmPnAOkKkhsldpyc83w_1777936233 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-44ffa15dc73so172716f8f.1 for ; Mon, 04 May 2026 16:10:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777936233; x=1778541033; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wBCEpyFpDJHMTwtRRg3SZCpV1QHG0l0VAs8/t638xpk=; b=p9cWjP/27UD15y4WJs6xwEGCEh45ZUaoPBKu5sYb9xGl4U0j4dXXFAzONpe3MMeekV TiwlMLg5H/2hHhn9JFBzen8Cg+RuXbZHe4FuBRC8CFiEizmXjxMRblsSlI6VtCjPL4PO f37lmkaSew42Vo62NP0KblQTIaGIA2BmLM5ugjpVcuPM8hSgWSifJNdRk8CaIBxSCKS1 lxSVhrjSfrtTCeKFiJftJCgsEKqnXsWk4MB1U6caoXeYSQbhodC4hxAGo+8TsDDlYfEc 2BlS02EDuJpcuTkwzw9JPdhqE9A2kZdLJZ7I0aagCFkAn+jGjaeLpgbeoZfmCHwmrrGA pmSw== X-Gm-Message-State: AOJu0YyklGkBrgZHmNsFJH64UxvtH49iErObuVehKtS0X0CJrwBYWT9K 5rPIpu8cpDjlxrzaJvpQc9pv4yILeFbBRw3xOBHEQtxli1b/DO1JKlc7ktstc8YgwFAKjtxCi8b 8nOCALMiTvJ1265qVzIyKoRHJJ3x+c+FbNthtw6P0XXDwmS9eJOOUhnKuU+2Vjg== X-Gm-Gg: AeBDiesKs9MBMIZ2BGydgzTZ/2iXQYsUoouo+oml/nQZQMNXwuWrRJ9nEKW3KbWibfG erP3+DZT3RgRfhGmHpUght4OZqaMehVgb9tzGbGu9jAYVQV/iAodNcahsZibja4ZBdg523+vKQT Unm1lQZwZb/6WR26hzm1pw/LtcGcSl1U5fWYs58G3Qt1SKQ+tdXTsBwuJrU8DaVh+d8JZy6Wyoi +6qGEM3O9cu7Dk5RsinvindFzfkwMIGTczTc1kFpDd3O6ihQKxoAu44oNCIkfl82GwEvElU01zP FfvinMuOdlDQhv4HfNZXpcfnK14zO+CC6Oka1ASQx7CbC3hqEiecZfCw6fq9tbWLwtaYvc0JiLR m5BKCuYOApvrH9fOYS258119zaosQpJoOjhKvv9AH9ZU= X-Received: by 2002:a05:6000:1acb:b0:43d:1bf6:30f7 with SMTP id ffacd0b85a97d-44bb4442d00mr20366478f8f.18.1777936232566; Mon, 04 May 2026 16:10:32 -0700 (PDT) X-Received: by 2002:a05:6000:1acb:b0:43d:1bf6:30f7 with SMTP id ffacd0b85a97d-44bb4442d00mr20366411f8f.18.1777936231980; Mon, 04 May 2026 16:10:31 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4502b38b568sm570616f8f.23.2026.05.04.16.10.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 May 2026 16:10:31 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 04/13] Makefile: Remove non-standard $(FLAGS) variable Message-ID: <20260505011029.09533ca1@elisabeth> In-Reply-To: References: <20260421032338.1909084-1-david@gibson.dropbear.id.au> <20260421032338.1909084-5-david@gibson.dropbear.id.au> <20260428091728.3b00b5be@elisabeth> <20260503235647.4aa1672c@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Tue, 05 May 2026 01:10:30 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 544NbL74G9kqypHQbyQ6ZA7A1lQtUrmWlBdc9IXkhk8_1777936233 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: EGGLTXILNSU2ME5ZTJVCAGPQZCTZMBPS X-Message-ID-Hash: EGGLTXILNSU2ME5ZTJVCAGPQZCTZMBPS X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. > > 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