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

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 4 May 2026 11:49:06 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> 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()"?

I'm not sure, but it's not used anyway, so I dropped it altogether in
v7.

> > +	do {								\
> > +		if (debug_flag)						\
> > +			msg_perror(__VA_ARGS__);			\
> > +	} while (0)
> > +
> > +#else /* !PESTO */

-- 
Stefano