From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=fdIi4uHM; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id A53885A0269 for ; Thu, 07 May 2026 01:55:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778111697; bh=3Uhsy47NcDm6AX5mIe378C3v4L2eokgb3De6D/43y3Q=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fdIi4uHMYIep5f4v6QP0cg2jk4b+GX8icHR7yCWYtuO9d7KG34dgqDa2HSi3PCuBA J4+ueJXLh/QEdWfUWTS3v2Oi3eAQN7YEfnsYoeVOWK8N352kj86WRgtOhmXdUrOwJz H7ZqilF4X2LWhZeiNGolHm1Z2xXbHAIeO561oIAgRO4/3ExkYZllZb78rUoADyijGN 80bdoJ2g3PoVgYTq7wMf55yPScsu4KwaA1pBH2BLUr2mn7KHDakpi2GT5WtCpC2QPw ejlGouBtssBIO3t+eKeM5LwVGdXRwrB6Oey8+z8rgFFz5lVZN7QkfKsqDIooju4kIK jsZ/KG/wSql+g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g9sgj144cz4wJg; Thu, 07 May 2026 09:54:57 +1000 (AEST) Date: Thu, 7 May 2026 09:41:37 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v11 09/23] pesto, log: Share log.h (but not log.c) with pesto tool Message-ID: References: <20260506213155.1886983-1-sbrivio@redhat.com> <20260506213155.1886983-10-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="1AHGLxK+mt1hb5p6" Content-Disposition: inline In-Reply-To: <20260506213155.1886983-10-sbrivio@redhat.com> Message-ID-Hash: LGRWUHJCZJCBSI7HIWLW5HCBFZEMRCIK X-Message-ID-Hash: LGRWUHJCZJCBSI7HIWLW5HCBFZEMRCIK X-MailFrom: dgibson@gandalf.ozlabs.org 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, Jon Maloy , Laurent Vivier , Paul Holzinger 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: --1AHGLxK+mt1hb5p6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, May 06, 2026 at 11:31:41PM +0200, Stefano Brivio wrote: > From: David Gibson >=20 > In pesto we're going to want several levels of error/warning messages, mu= ch > like passt itself. Particularly as we start to share mode code between > passt and pesto, we want to use a similar interface to emit those. Howev= er > we don't want to use the same implementation - logging to a file or syslog > doesn't make sense for the command line tool. >=20 > To accomplish this loosely share log.h, but not log.c between pesto and > passt. In fact, an #ifdef means even most of log.h isn't actually shared, > but we do provide similar warn(), die() etc. macros. >=20 > This includes the *_perror() variants, which need strerror(). However, > we want to avoid allocations for pesto as we do for passt, and strerror() > allocates in some libc versions. Therefore, also move our workaround for > this to be shared with pesto. >=20 > Reviewed-by: Laurent Vivier > [dwg: Based on changes part of a larger patch by Stefano] > Signed-off-by: David Gibson > [sbrivio: Dropped debug_perror_() as it's not used anyway, Laurent was > asking about its name] > [sbrivio: Fix conflicts in the Makefile caused by the fact that I'm > not merging a previous series reworking it] > [sbrivio: For some reason, this triggers some unrelated, but valid, > cppcheck warnings in tap.c and conf.c: fix / suppress them] Yeah, I've hit some of the same warnings appearing and disappearing for no clear reason. The scope reduction warnings around debug() are particularly nasty, because they could need to be suppressed in a bunch of places. Oh well, this will do for now. Reviewed-by: David Gibson > Signed-off-by: Stefano Brivio > --- > Makefile | 10 +++++----- > common.h | 32 ++++++++++++++++++++++++++++++++ > conf.c | 4 +++- > log.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > pasta.c | 4 ++-- > pesto.c | 14 ++++---------- > tap.c | 12 +++++++++++- > util.h | 32 -------------------------------- > 8 files changed, 109 insertions(+), 52 deletions(-) >=20 > diff --git a/Makefile b/Makefile > index 76b9b5c..2639472 100644 > --- a/Makefile > +++ b/Makefile > @@ -50,10 +50,10 @@ SRCS =3D $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SR= CS) $(PESTO_SRCS) > =20 > MANPAGES =3D passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1 > =20 > -PASST_HEADERS =3D arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.= h \ > - epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h icmp_flow.h \ > - inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h ndp.h \ > - netlink.h packet.h passt.h pasta.h pesto.h pcap.h pif.h repair.h \ > +PASST_HEADERS =3D arch.h arp.h bitmap.h checksum.h common.h conf.h dhcp.= h \ > + dhcpv6.h epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h \ > + icmp_flow.h inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h \ > + ndp.h netlink.h packet.h passt.h pasta.h pcap.h pesto.h pif.h repair.h \ > serialise.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 > @@ -116,7 +116,7 @@ passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h > $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repa= ir $(LDFLAGS) > =20 > pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h > - $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PESTO_SRCS) -o pesto $(LDFLAGS) > + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DPESTO $(PESTO_SRCS) -o pesto $(L= DFLAGS) > =20 > valgrind: EXTRA_SYSCALLS +=3D rt_sigprocmask rt_sigtimedwait rt_sigactio= n \ > rt_sigreturn getpid gettid kill clock_gettime \ > diff --git a/common.h b/common.h > index f3506b4..4251781 100644 > --- a/common.h > +++ b/common.h > @@ -21,4 +21,36 @@ > /* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */ > #define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__) > =20 > +/* > + * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strer= ror, > + * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() n= eeds > + * getrandom(2) and brk(2) as it allocates memory for the locale-transla= ted > + * error description, but our seccomp profiles forbid both. > + * > + * Use the strerror_() wrapper instead, calling into strerrordesc_np() t= o get > + * a static untranslated string. It's a GNU implementation, but also def= ined by > + * bionic. > + * > + * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C l= ibraries > + * not defining strerrordesc_np() are expected to provide strerror() > + * implementations that are simple enough for us to call. > + */ > +__attribute__ ((weak)) const char *strerrordesc_np(int errnum); > + > +/** > + * strerror_() - strerror() wrapper calling strerrordesc_np() if availab= le > + * @errnum: Error code > + * > + * Return: error description string > + */ > +static inline const char *strerror_(int errnum) > +{ > + if (strerrordesc_np) > + return strerrordesc_np(errnum); > + > + return strerror(errnum); > +} > + > +#define strerror(x) @ "Don't call strerror() directly, use strerror_() i= nstead" > + > #endif /* COMMON_H */ > diff --git a/conf.c b/conf.c > index 0586107..05e93db 100644 > --- a/conf.c > +++ b/conf.c > @@ -308,7 +308,9 @@ static void conf_netns_opt(char *netns, const char *a= rg) > * @argv: Command line arguments > */ > static void conf_pasta_ns(int *netns_only, char *userns, char *netns, > - int optind, int argc, char *argv[]) > + int optind, int argc, > +/* cppcheck-suppress [constParameter, unmatchedSuppression] */ > + char *argv[]) > { > if (*netns && optind !=3D argc) > die("Both --netns and PID or command given"); > diff --git a/log.h b/log.h > index dbab006..c6befe3 100644 > --- a/log.h > +++ b/log.h > @@ -6,8 +6,57 @@ > #ifndef LOG_H > #define LOG_H > =20 > -#include > #include > +#include > +#include > + > +#ifdef PESTO > + > +#include > + > +#include "common.h" > + > +extern bool debug_flag; > + > +#define msg(...) \ > + do { \ > + FPRINTF(stderr, __VA_ARGS__); \ > + FPRINTF(stderr, "\n"); \ > + } while (0) > + > +#define msg_perror(...) \ > + do { \ > + int errno_ =3D errno; \ > + FPRINTF(stderr, __VA_ARGS__); \ > + FPRINTF(stderr, ": %s\n", strerror_(errno_)); \ > + } while (0) > + > +#define die(...) \ > + do { \ > + msg(__VA_ARGS__); \ > + exit(EXIT_FAILURE); \ > + } while (0) > + > +#define die_perror(...) \ > + do { \ > + msg_perror(__VA_ARGS__); \ > + exit(EXIT_FAILURE); \ > + } while (0) > + > +#define warn(...) msg(__VA_ARGS__) > +#define warn_perror(...) msg_perror(__VA_ARGS__) > +#define info(...) msg(__VA_ARGS__) > +#define info_perror(...) msg_perror(__VA_ARGS__) > + > +#define debug(...) \ > + do { \ > + if (debug_flag) \ > + msg(__VA_ARGS__); \ > + } while (0) > + > +#else /* !PESTO */ > + > +#include > #include > #include > =20 > @@ -109,4 +158,6 @@ void __openlog(const char *ident, int option, int fac= ility); > void logfile_init(const char *name, const char *path, size_t size); > void __setlogmask(int mask); > =20 > +#endif /* !PESTO */ > + > #endif /* LOG_H */ > diff --git a/pasta.c b/pasta.c > index bab945f..4e7ee54 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -521,7 +521,7 @@ void pasta_netns_quit_init(const struct ctx *c) > * @c: Execution context > * @inotify_fd: inotify file descriptor with watch on namespace directory > */ > -void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd) > +void pasta_netns_quit_inotify_handler(const struct ctx *c, int inotify_f= d) > { > char buf[sizeof(struct inotify_event) + NAME_MAX + 1] > __attribute__ ((aligned(__alignof__(struct inotify_event)))); > @@ -547,7 +547,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, = int inotify_fd) > * @c: Execution context > * @ref: epoll reference for timer descriptor > */ > -void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref) > +void pasta_netns_quit_timer_handler(const struct ctx *c, union epoll_ref= ref) > { > uint64_t expirations; > ssize_t n; > diff --git a/pesto.c b/pesto.c > index 9f2fa5d..f0916e8 100644 > --- a/pesto.c > +++ b/pesto.c > @@ -34,18 +34,12 @@ > #include "common.h" > #include "seccomp_pesto.h" > #include "pesto.h" > +#include "log.h" > =20 > -static bool debug_flag =3D false; > +bool debug_flag =3D false; > =20 > static char stdout_buf[BUFSIZ]; > =20 > -#define die(...) \ > - do { \ > - FPRINTF(stderr, __VA_ARGS__); \ > - FPRINTF(stderr, "\n"); \ > - exit(EXIT_FAILURE); \ > - } while (0) > - > /** > * usage() - Print usage, exit with given status code > * @name: Executable name > @@ -99,7 +93,7 @@ int main(int argc, char **argv) > * breaking our seccomp profile. > */ > if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf))) > - die("Failed to set stdout buffer"); > + die_perror("Failed to set stdout buffer"); > =20 > do { > optname =3D getopt_long(argc, argv, optstring, options, NULL); > @@ -126,7 +120,7 @@ int main(int argc, char **argv) > if (argc - optind !=3D 1) > usage(argv[0], stderr, EXIT_FAILURE); > =20 > - printf("debug_flag=3D%d, path=3D\"%s\"\n", debug_flag, argv[optind]); > + debug("debug_flag=3D%d, path=3D\"%s\"", debug_flag, argv[optind]); > =20 > die("pesto is not implemented yet"); > } > diff --git a/tap.c b/tap.c > index 7d06189..412f437 100644 > --- a/tap.c > +++ b/tap.c > @@ -756,6 +756,11 @@ resume: > =20 > if (IN4_IS_ADDR_LOOPBACK(&iph->saddr) || > IN4_IS_ADDR_LOOPBACK(&iph->daddr)) { > + /* The scope of sstr and dstr could be in theory reduced > + * into the conditional block debug() expands to, but > + * it's awkward and unreadable, so ignore this warning. > + */ > + /* cppcheck-suppress [variableScope,unmatchedSuppression] */ > char sstr[INET_ADDRSTRLEN], dstr[INET_ADDRSTRLEN]; > =20 > debug("Loopback address on tap interface: %s -> %s", > @@ -929,6 +934,11 @@ resume: > continue; > =20 > if (IN6_IS_ADDR_LOOPBACK(saddr) || IN6_IS_ADDR_LOOPBACK(daddr)) { > + /* The scope of sstr and dstr could be in theory reduced > + * into the conditional block debug() expands to, but > + * it's awkward and unreadable, so ignore this warning. > + */ > + /* cppcheck-suppress [variableScope,unmatchedSuppression] */ > char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN]; > =20 > debug("Loopback address on tap interface: %s -> %s", > @@ -1304,7 +1314,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t even= ts, > * tap_backend_show_hints() - Give help information to start QEMU > * @c: Execution context > */ > -static void tap_backend_show_hints(struct ctx *c) > +static void tap_backend_show_hints(const struct ctx *c) > { > switch (c->mode) { > case MODE_PASTA: > diff --git a/util.h b/util.h > index 770ff93..e90be47 100644 > --- a/util.h > +++ b/util.h > @@ -302,38 +302,6 @@ static inline bool mod_between(unsigned x, unsigned = i, unsigned j, unsigned m) > =20 > void raw_random(void *buf, size_t buflen); > =20 > -/* > - * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strer= ror, > - * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() n= eeds > - * getrandom(2) and brk(2) as it allocates memory for the locale-transla= ted > - * error description, but our seccomp profiles forbid both. > - * > - * Use the strerror_() wrapper instead, calling into strerrordesc_np() t= o get > - * a static untranslated string. It's a GNU implementation, but also def= ined by > - * bionic. > - * > - * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C l= ibraries > - * not defining strerrordesc_np() are expected to provide strerror() > - * implementations that are simple enough for us to call. > - */ > -__attribute__ ((weak)) const char *strerrordesc_np(int errnum); > - > -/** > - * strerror_() - strerror() wrapper calling strerrordesc_np() if availab= le > - * @errnum: Error code > - * > - * Return: error description string > - */ > -static inline const char *strerror_(int errnum) > -{ > - if (strerrordesc_np) > - return strerrordesc_np(errnum); > - > - return strerror(errnum); > -} > - > -#define strerror(x) @ "Don't call strerror() directly, use strerror_() i= nstead" > - > /* > * Workarounds for https://github.com/llvm/llvm-project/issues/58992 > * > --=20 > 2.43.0 >=20 --=20 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 --1AHGLxK+mt1hb5p6 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIyBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn70aoACgkQzQJF27ox 2GdlGQ/3ZjkeoJ5gcvmuBrSToNh00433YG3GXCOjkgT8ucPZisCtdCWpryZ1gV/7 q7VPT6HaqZI4g7WBnNnngt/9bU11Kppssfj8ZFIa9zMyaA4hO3No8jyCxSO4pR3g UQ6WaGt+RbVuK/uuYD1jf9bRMQwxVd/KGGjJ1c/Col78aksaVPx4iRSY/LFg5SiE BrP1Ut0K7lW9DsD33iLnoOFtuFVPEQPmU0/ZXVIZ5el1uhajXOtkuBG8qKmkalvQ 5JVzUkIGlyfjf46NJ186Bq5v9rwqNAzp4ySvpK6/0yu2jaOQVriseTNR5FepcbMG rlH4H/AhzAjHZfhZDvU1CZmpd/zUEAdUQqwN1ppIFR+Gzu5Z15sCazE+o/XIFdxk N41Tf+AX39wwczAkxHZCXCY8F1Ez5Q+5EyoMM5RAmoeX+8P2z4rQaRus4JI3MFH2 Pnlt5M8JdtK0ZXb2MB16KVT1kxCeynkwjRENljpYCq1cJ62xx1aM7A/KKRQ487Vz BQhsnhC85JNmqQ9OZa6UClYPtRHB1pV22lKWiUfacrnIIliWKu9w+VgMzxcwLqRY zA8p1HzRr1zIeyS18MP2kP4bN0ON/ZKmN+hfa/KtmXI0csf7q1KsriNPyLLEwpAU QAjcUavR67ducRWO6TbWlw0FfIqPDN5k15RM2WC9SZHMsJSuew== =+jjY -----END PGP SIGNATURE----- --1AHGLxK+mt1hb5p6--