Re: [PATCH v11 09/23] pesto, log: Share log.h (but not log.c) with pesto tool

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13110 bytes --]

On Wed, May 06, 2026 at 11:31:41PM +0200, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> In pesto we're going to want several levels of error/warning messages, much
> 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.  However
> we don't want to use the same implementation - logging to a file or syslog
> doesn't make sense for the command line tool.
> 
> 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.
> 
> 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.
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> [dwg: Based on changes part of a larger patch by Stefano]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [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 <david@gibson.dropbear.id.au>

> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  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(-)
> 
> diff --git a/Makefile b/Makefile
> index 76b9b5c..2639472 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -50,10 +50,10 @@ SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
>  
>  MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
>  
> -PASST_HEADERS = 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 = 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-repair $(LDFLAGS)
>  
>  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 $(LDFLAGS)
>  
>  valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
>  			    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__)
>  
> +/*
> + * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror,
> + * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs
> + * getrandom(2) and brk(2) as it allocates memory for the locale-translated
> + * error description, but our seccomp profiles forbid both.
> + *
> + * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get
> + * a static untranslated string. It's a GNU implementation, but also defined by
> + * bionic.
> + *
> + * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries
> + * 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 available
> + * @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_() instead"
> +
>  #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 *arg)
>   * @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 != 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
>  
> -#include <stdarg.h>
>  #include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#ifdef PESTO
> +
> +#include <errno.h>
> +
> +#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_ = 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 <stdarg.h>
>  #include <stddef.h>
>  #include <syslog.h>
>  
> @@ -109,4 +158,6 @@ void __openlog(const char *ident, int option, int facility);
>  void logfile_init(const char *name, const char *path, size_t size);
>  void __setlogmask(int mask);
>  
> +#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_fd)
>  {
>  	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"
>  
> -static bool debug_flag = false;
> +bool debug_flag = false;
>  
>  static char stdout_buf[BUFSIZ];
>  
> -#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");
>  
>  	do {
>  		optname = getopt_long(argc, argv, optstring, options, NULL);
> @@ -126,7 +120,7 @@ int main(int argc, char **argv)
>  	if (argc - optind != 1)
>  		usage(argv[0], stderr, EXIT_FAILURE);
>  
> -	printf("debug_flag=%d, path=\"%s\"\n", debug_flag, argv[optind]);
> +	debug("debug_flag=%d, path=\"%s\"", debug_flag, argv[optind]);
>  
>  	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:
>  
>  		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];
>  
>  			debug("Loopback address on tap interface: %s -> %s",
> @@ -929,6 +934,11 @@ resume:
>  			continue;
>  
>  		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];
>  
>  			debug("Loopback address on tap interface: %s -> %s",
> @@ -1304,7 +1314,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
>   * 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)
>  
>  void raw_random(void *buf, size_t buflen);
>  
> -/*
> - * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror,
> - * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs
> - * getrandom(2) and brk(2) as it allocates memory for the locale-translated
> - * error description, but our seccomp profiles forbid both.
> - *
> - * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get
> - * a static untranslated string. It's a GNU implementation, but also defined by
> - * bionic.
> - *
> - * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries
> - * 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 available
> - * @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_() instead"
> -
>  /*
>   * Workarounds for https://github.com/llvm/llvm-project/issues/58992
>   *
> -- 
> 2.43.0
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]