public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH 03/11] log: Drop newlines in the middle of the perror()-like messages
Date: Thu, 25 Jul 2024 13:26:47 +1000	[thread overview]
Message-ID: <ZqHF96EVtSZhol5f@zatzit> (raw)
In-Reply-To: <20240724215021.3366863-4-sbrivio@redhat.com>

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

  reply	other threads:[~2024-07-25  3:47 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-24 21:50 [PATCH 00/11] Minor assorted fixes, mostly logging and tests Stefano Brivio
2024-07-24 21:50 ` [PATCH 01/11] tap: Don't quit if pasta gets EIO on writev() to tap, interface might be down Stefano Brivio
2024-07-25  3:21   ` David Gibson
2024-07-24 21:50 ` [PATCH 02/11] tcp: Change SO_PEEK_OFF support message to debug() Stefano Brivio
2024-07-25  3:22   ` David Gibson
2024-07-24 21:50 ` [PATCH 03/11] log: Drop newlines in the middle of the perror()-like messages Stefano Brivio
2024-07-25  3:26   ` David Gibson [this message]
2024-07-25 11:27     ` Stefano Brivio
2024-07-26  0:33       ` David Gibson
2024-07-24 21:50 ` [PATCH 04/11] log: Fix sub-second part in relative log time calculation Stefano Brivio
2024-07-25  3:32   ` David Gibson
2024-07-25  7:51     ` Stefano Brivio
2024-07-24 21:50 ` [PATCH 05/11] log: Initialise timestamp for relative log time also if we use a log file Stefano Brivio
2024-07-25  3:35   ` David Gibson
2024-07-25  7:51     ` Stefano Brivio
2024-07-24 21:50 ` [PATCH 06/11] test: Fix memory/passt tests, --netns-only is not a valid option for passt Stefano Brivio
2024-07-25  3:50   ` David Gibson
2024-07-24 21:50 ` [PATCH 07/11] test: Update names of symbols and slabinfo entries Stefano Brivio
2024-07-25  3:54   ` David Gibson
2024-07-24 21:50 ` [PATCH 08/11] test: iperf3 3.16 introduces multiple threads, drop our own implementation of that Stefano Brivio
2024-07-25  4:23   ` David Gibson
2024-07-24 21:50 ` [PATCH 09/11] tap: Exit if we fail to bind a UNIX domain socket with explicit path Stefano Brivio
2024-07-25  4:00   ` David Gibson
2024-07-24 21:50 ` [PATCH 10/11] tap: Discard guest data on length descriptor mismatch Stefano Brivio
2024-07-25  4:37   ` David Gibson
2024-07-25  9:15     ` Stefano Brivio
2024-07-25 10:23       ` David Gibson
2024-07-25 11:09         ` Stefano Brivio
2024-07-26  1:22           ` David Gibson
2024-07-24 21:50 ` [PATCH 11/11] conf: Accept addresses enclosed by square brackets in port forwarding specifiers Stefano Brivio
2024-07-25  4:39   ` David Gibson
2024-07-25 11:26     ` Stefano Brivio
2024-07-25 11:28 ` [PATCH 00/11] Minor assorted fixes, mostly logging and tests Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZqHF96EVtSZhol5f@zatzit \
    --to=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).