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

[Laurent Vivier <lvivier@redhat.com>]

On 5/3/26 23:55, 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.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> [dwg: Based on changes part of a larger patch by Stefano]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

One little nit below

> ---
>   Makefile |  6 +++++-
>   common.h | 32 ++++++++++++++++++++++++++++++
>   log.h    | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   pesto.c  | 14 ++++----------
>   util.h   | 32 ------------------------------
>   5 files changed, 99 insertions(+), 44 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 030681b..f6cec8a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -61,7 +61,7 @@ PASST_HEADERS = arch.h arp.h bitmap.h checksum.h common.h conf.h dhcp.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
> -PESTO_HEADERS = common.h pesto.h
> +PESTO_HEADERS = common.h pesto.h log.h
>   
>   C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
>   ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> @@ -121,6 +121,7 @@ qrap: $(QRAP_SRCS) $(QRAP_HEADERS)
>   
>   passt-repair: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
>   
> +pesto: BASE_CPPFLAGS += -DPESTO
>   pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
>   
>   valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
> @@ -221,9 +222,12 @@ cppcheck: passt.cppcheck passt-repair.cppcheck pesto.cppcheck qrap.cppcheck
>   %.cppcheck:
>   	$(CPPCHECK) $(CPPCHECK_FLAGS) $(BASE_CPPFLAGS) $^
>   
> +passt.cppcheck: BASE_CPPFLAGS += -UPESTO
>   passt.cppcheck: $(PASST_SRCS) $(PASST_HEADERS) seccomp.h
> +
>   passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repair.h
>   
> +pesto.cppcheck: BASE_CPPFLAGS += -DPESTO
>   pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unmatchedSuppression
>   pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
>   
> diff --git a/common.h b/common.h
> index a9c115a..2f2e6f1 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/log.h b/log.h
> index dbab006..1058ca5 100644
> --- a/log.h
> +++ b/log.h
> @@ -6,8 +6,63 @@
>   #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)
> +
> +#define debug_perror_(...)						\

Why is this "debug_perror_()" and not "debug_perror()"?

> +	do {								\
> +		if (debug_flag)						\
> +			msg_perror(__VA_ARGS__);			\
> +	} while (0)
> +
> +#else /* !PESTO */
> +
> +#include <stdarg.h>
>   #include <stddef.h>
>   #include <syslog.h>
>   
> @@ -109,4 +164,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/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/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
>    *