Re: [PATCH 03/11] log: Drop newlines in the middle of the perror()-like messages

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

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

On Wed, Jul 24, 2024 at 11:50:09PM +0200, Stefano Brivio wrote:
> Calling vlogmsg() twice from logmsg_perror() results in this beauty:
> 
>   $ ./pasta -i foo
>   Invalid interface name foo
>   : No such device
> 
> because the first part of the message, corresponding to the first
> call, doesn't end with a newline, and vlogmsg() adds it.
> 
> Given that we can't easily append an argument (error description) to
> a variadic list, add a 'newline' parameter to all the functions that
> currently add a newline if missing, and disable that on the first call
> to vlogmsg() from logmsg_perror(). Not very pretty but I can't think
> of any solution that's less messy than this.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

I think my personal inclination would be to rename all the
lowest-level functions slightly and remove the newline adding logic
unconditionally.  The create wrappers under the old name which add the
"\n".  I think that can be done in an easy macro, since the "\n" can
be constant string appended to the format string.  Just the special
paths that need to suppress the newline would call the low level "no
newline" variants.

But, I don't actually care that much so

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  flow.c |  2 +-
>  log.c  | 34 ++++++++++++++++++++++------------
>  log.h  | 18 +++++++++---------
>  3 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index d7d548d..bd5fa2b 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -279,7 +279,7 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...)
>  	else
>  		type_or_state = FLOW_TYPE(f);
>  
> -	logmsg(pri, "Flow %u (%s): %s", flow_idx(f), type_or_state, msg);
> +	logmsg(true, pri, "Flow %u (%s): %s", flow_idx(f), type_or_state, msg);
>  }
>  
>  /**
> diff --git a/log.c b/log.c
> index 9ed6bc1..54483e7 100644
> --- a/log.c
> +++ b/log.c
> @@ -46,7 +46,14 @@ int		log_trace;		/* --trace mode enabled */
>  bool		log_conf_parsed;	/* Logging options already parsed */
>  bool		log_runtime;		/* Daemonised, or ready in foreground */
>  
> -void vlogmsg(int pri, const char *format, va_list ap)
> +/**
> + * vlogmsg() - Print or send messages to log or output files as configured
> + * @newline:	Append newline at the end of the message, if missing
> + * @pri:	Facility and level map, same as priority for vsyslog()
> + * @format:	Message
> + * @ap:		Variable argument list
> + */
> +void vlogmsg(bool newline, int pri, const char *format, va_list ap)
>  {
>  	bool debug_print = (log_mask & LOG_MASK(LOG_DEBUG)) && log_file == -1;
>  	struct timespec tp;
> @@ -63,9 +70,9 @@ void vlogmsg(int pri, const char *format, va_list ap)
>  
>  		va_copy(ap2, ap); /* Don't clobber ap, we need it again */
>  		if (log_file != -1)
> -			logfile_write(pri, format, ap2);
> +			logfile_write(newline, pri, format, ap2);
>  		else if (!(log_mask & LOG_MASK(LOG_DEBUG)))
> -			passt_vsyslog(pri, format, ap2);
> +			passt_vsyslog(newline, pri, format, ap2);
>  
>  		va_end(ap2);
>  	}
> @@ -73,22 +80,23 @@ void vlogmsg(int pri, const char *format, va_list ap)
>  	if (debug_print || !log_conf_parsed ||
>  	    (!log_runtime && (log_mask & LOG_MASK(LOG_PRI(pri))))) {
>  		(void)vfprintf(stderr, format, ap);
> -		if (format[strlen(format)] != '\n')
> +		if (newline && format[strlen(format)] != '\n')
>  			fprintf(stderr, "\n");
>  	}
>  }
>  
>  /**
>   * logmsg() - vlogmsg() wrapper for variable argument lists
> + * @newline:	Append newline at the end of the message, if missing
>   * @pri:	Facility and level map, same as priority for vsyslog()
>   * @format:	Message
>   */
> -void logmsg(int pri, const char *format, ...)
> +void logmsg(bool newline, int pri, const char *format, ...)
>  {
>  	va_list ap;
>  
>  	va_start(ap, format);
> -	vlogmsg(pri, format, ap);
> +	vlogmsg(newline, pri, format, ap);
>  	va_end(ap);
>  }
>  
> @@ -103,10 +111,10 @@ void logmsg_perror(int pri, const char *format, ...)
>  	va_list ap;
>  
>  	va_start(ap, format);
> -	vlogmsg(pri, format, ap);
> +	vlogmsg(false, pri, format, ap);
>  	va_end(ap);
>  
> -	logmsg(pri, ": %s", strerror(errno_copy));
> +	logmsg(true, pri, ": %s", strerror(errno_copy));
>  }
>  
>  /* Prefixes for log file messages, indexed by priority */
> @@ -174,11 +182,12 @@ void __setlogmask(int mask)
>  
>  /**
>   * passt_vsyslog() - vsyslog() implementation not using heap memory
> + * @newline:	Append newline at the end of the message, if missing
>   * @pri:	Facility and level map, same as priority for vsyslog()
>   * @format:	Same as vsyslog() format
>   * @ap:		Same as vsyslog() ap
>   */
> -void passt_vsyslog(int pri, const char *format, va_list ap)
> +void passt_vsyslog(bool newline, int pri, const char *format, va_list ap)
>  {
>  	char buf[BUFSIZ];
>  	int n;
> @@ -188,7 +197,7 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
>  
>  	n += vsnprintf(buf + n, BUFSIZ - n, format, ap);
>  
> -	if (format[strlen(format)] != '\n')
> +	if (newline && format[strlen(format)] != '\n')
>  		n += snprintf(buf + n, BUFSIZ - n, "\n");
>  
>  	if (log_sock >= 0 && send(log_sock, buf, n, 0) != n && !log_runtime)
> @@ -360,11 +369,12 @@ static int logfile_rotate(int fd, const struct timespec *now)
>  
>  /**
>   * logfile_write() - Write entry to log file, trigger rotation if full
> + * @newline:	Append newline at the end of the message, if missing
>   * @pri:	Facility and level map, same as priority for vsyslog()
>   * @format:	Same as vsyslog() format
>   * @ap:		Same as vsyslog() ap
>   */
> -void logfile_write(int pri, const char *format, va_list ap)
> +void logfile_write(bool newline, int pri, const char *format, va_list ap)
>  {
>  	struct timespec now;
>  	char buf[BUFSIZ];
> @@ -380,7 +390,7 @@ void logfile_write(int pri, const char *format, va_list ap)
>  
>  	n += vsnprintf(buf + n, BUFSIZ - n, format, ap);
>  
> -	if (format[strlen(format)] != '\n')
> +	if (newline && format[strlen(format)] != '\n')
>  		n += snprintf(buf + n, BUFSIZ - n, "\n");
>  
>  	if ((log_written + n >= log_size) && logfile_rotate(log_file, &now))
> diff --git a/log.h b/log.h
> index 6f34e8d..51ddafa 100644
> --- a/log.h
> +++ b/log.h
> @@ -13,16 +13,16 @@
>  #define LOGFILE_CUT_RATIO		30	/* When full, cut ~30% size */
>  #define LOGFILE_SIZE_MIN		(5UL * MAX(BUFSIZ, PAGE_SIZE))
>  
> -void vlogmsg(int pri, const char *format, va_list ap);
> -void logmsg(int pri, const char *format, ...)
> -	__attribute__((format(printf, 2, 3)));
> +void vlogmsg(bool newline, int pri, const char *format, va_list ap);
> +void logmsg(bool newline, int pri, const char *format, ...)
> +	__attribute__((format(printf, 3, 4)));
>  void logmsg_perror(int pri, const char *format, ...)
>  	__attribute__((format(printf, 2, 3)));
>  
> -#define err(...)		logmsg(		LOG_ERR,	__VA_ARGS__)
> -#define warn(...)		logmsg(		LOG_WARNING,	__VA_ARGS__)
> -#define info(...)		logmsg(		LOG_INFO,	__VA_ARGS__)
> -#define debug(...)		logmsg(		LOG_DEBUG,	__VA_ARGS__)
> +#define err(...)		logmsg(true,	LOG_ERR,	__VA_ARGS__)
> +#define warn(...)		logmsg(true,	LOG_WARNING,	__VA_ARGS__)
> +#define info(...)		logmsg(true,	LOG_INFO,	__VA_ARGS__)
> +#define debug(...)		logmsg(true,	LOG_DEBUG,	__VA_ARGS__)
>  
>  #define err_perror(...)		logmsg_perror(	LOG_ERR,	__VA_ARGS__)
>  #define warn_perror(...)	logmsg_perror(	LOG_WARNING,	__VA_ARGS__)
> @@ -54,8 +54,8 @@ void trace_init(int enable);
>  
>  void __openlog(const char *ident, int option, int facility);
>  void logfile_init(const char *name, const char *path, size_t size);
> -void passt_vsyslog(int pri, const char *format, va_list ap);
> -void logfile_write(int pri, const char *format, va_list ap);
> +void passt_vsyslog(bool newline, int pri, const char *format, va_list ap);
> +void logfile_write(bool newline, int pri, const char *format, va_list ap);
>  void __setlogmask(int mask);
>  
>  #endif /* LOG_H */

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