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, Yalan Zhang <yalzhang@redhat.com>
Subject: Re: [PATCH v4 4/8] log, passt: Always print to stderr before initialisation is complete
Date: Thu, 20 Jun 2024 14:34:04 +1000	[thread overview]
Message-ID: <ZnOxPBY2QbErEseq@zatzit> (raw)
In-Reply-To: <20240620040800.3065684-5-sbrivio@redhat.com>

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

On Thu, Jun 20, 2024 at 06:07:56AM +0200, Stefano Brivio wrote:
> After commit 15001b39ef1d ("conf: set the log level much earlier"), we
> had a phase during initialisation when messages wouldn't be printed to
> standard error anymore.
> 
> Commit f67238aa864d ("passt, log: Call __openlog() earlier, log to
> stderr until we detach") fixed that, but only for the case where no
> log files are given.
> 
> If a log file is configured, vlogmsg() will not call passt_vsyslog(),
> but during initialisation, LOG_PERROR is set, so to avoid duplicated
> prints (which would result from passt_vsyslog() printing to stderr),
> we don't call fprintf() from vlogmsg() either.
> 
> This is getting a bit too complicated. Instead of abusing LOG_PERROR,
> define an internal logging flag that clearly represents that we're not
> done with the initialisation phase yet.
> 
> If this flag is not set, make sure we always print to stderr, if the
> log mask matches.
> 
> Reported-by: Yalan Zhang <yalzhang@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  log.c   | 17 ++++++++---------
>  log.h   |  1 +
>  passt.c | 15 +++++++--------
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/log.c b/log.c
> index 05b7f80..025129a 100644
> --- a/log.c
> +++ b/log.c
> @@ -33,7 +33,6 @@
>  static int	log_sock = -1;		/* Optional socket to system logger */
>  static char	log_ident[BUFSIZ];	/* Identifier string for openlog() */
>  static int	log_mask;		/* Current log priority mask */
> -static int	log_opt;		/* Options for openlog() */
>  
>  static int	log_file = -1;		/* Optional log file descriptor */
>  static size_t	log_size;		/* Maximum log file size in bytes */
> @@ -45,6 +44,7 @@ static time_t	log_start;		/* Start timestamp */
>  
>  int		log_trace;		/* --trace mode enabled */
>  bool		log_conf_parsed;	/* Logging options already parsed */
> +bool		log_daemonised;		/* Daemonised, standard error is gone */
>  
>  void vlogmsg(int pri, const char *format, va_list ap)
>  {
> @@ -70,7 +70,8 @@ void vlogmsg(int pri, const char *format, va_list ap)
>  		va_end(ap2);
>  	}
>  
> -	if (debug_print || (!log_conf_parsed && !(log_opt & LOG_PERROR))) {
> +	if (debug_print || !log_conf_parsed ||
> +	    (!log_daemonised && (log_mask & LOG_MASK(LOG_PRI(pri))))) {
>  		(void)vfprintf(stderr, format, ap);
>  		if (format[strlen(format)] != '\n')
>  			fprintf(stderr, "\n");
> @@ -108,13 +109,15 @@ void trace_init(int enable)
>  /**
>   * __openlog() - Non-optional openlog() implementation, for custom vsyslog()
>   * @ident:	openlog() identity (program name)
> - * @option:	openlog() options
> + * @option:	openlog() options, unused
>   * @facility:	openlog() facility (LOG_DAEMON)
>   */
>  void __openlog(const char *ident, int option, int facility)
>  {
>  	struct timespec tp;
>  
> +	(void)option;
> +
>  	clock_gettime(CLOCK_REALTIME, &tp);
>  	log_start = tp.tv_sec;
>  
> @@ -135,7 +138,6 @@ void __openlog(const char *ident, int option, int facility)
>  
>  	log_mask |= facility;
>  	strncpy(log_ident, ident, sizeof(log_ident) - 1);
> -	log_opt = option;
>  }
>  
>  /**
> @@ -156,20 +158,17 @@ void __setlogmask(int mask)
>   */
>  void passt_vsyslog(int pri, const char *format, va_list ap)
>  {
> -	int prefix_len, n;
>  	char buf[BUFSIZ];
> +	int n;
>  
>  	/* Send without timestamp, the system logger should add it */
> -	n = prefix_len = snprintf(buf, BUFSIZ, "<%i> %s: ", pri, log_ident);
> +	n = snprintf(buf, BUFSIZ, "<%i> %s: ", pri, log_ident);
>  
>  	n += vsnprintf(buf + n, BUFSIZ - n, format, ap);
>  
>  	if (format[strlen(format)] != '\n')
>  		n += snprintf(buf + n, BUFSIZ - n, "\n");
>  
> -	if (log_opt & LOG_PERROR)
> -		fprintf(stderr, "%s", buf + prefix_len);
> -
>  	if (log_sock >= 0 && send(log_sock, buf, n, 0) != n)
>  		fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
>  }
> diff --git a/log.h b/log.h
> index 3dab284..7bfd92d 100644
> --- a/log.h
> +++ b/log.h
> @@ -30,6 +30,7 @@ void logmsg(int pri, const char *format, ...)
>  
>  extern int log_trace;
>  extern bool log_conf_parsed;
> +extern bool log_daemonised;
>  
>  void trace_init(int enable);
>  #define trace(...)							\
> diff --git a/passt.c b/passt.c
> index 19ecd68..e3c1d50 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -200,8 +200,8 @@ void exit_handler(int signal)
>  int main(int argc, char **argv)
>  {
>  	struct epoll_event events[EPOLL_EVENTS];
> -	char *log_name, argv0[PATH_MAX], *name;
>  	int nfds, i, devnull_fd = -1;
> +	char argv0[PATH_MAX], *name;
>  	struct ctx c = { 0 };
>  	struct rlimit limit;
>  	struct timespec now;
> @@ -225,7 +225,7 @@ int main(int argc, char **argv)
>  	strncpy(argv0, argv[0], PATH_MAX - 1);
>  	name = basename(argv0);
>  	if (strstr(name, "pasta")) {
> -		__openlog(log_name = "pasta", LOG_PERROR, LOG_DAEMON);
> +		__openlog("pasta", 0, LOG_DAEMON);
>  
>  		sa.sa_handler = pasta_child_handler;
>  		if (sigaction(SIGCHLD, &sa, NULL)) {
> @@ -240,7 +240,7 @@ int main(int argc, char **argv)
>  
>  		c.mode = MODE_PASTA;
>  	} else if (strstr(name, "passt")) {
> -		__openlog(log_name = "passt", LOG_PERROR, LOG_DAEMON);
> +		__openlog("passt", 0, LOG_DAEMON);
>  
>  		c.mode = MODE_PASST;
>  	} else {
> @@ -302,13 +302,12 @@ int main(int argc, char **argv)
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
>  
> -	if (!c.foreground)
> -		__openlog(log_name, 0, LOG_DAEMON);
> -
> -	if (!c.foreground)
> +	if (!c.foreground) {
>  		__daemon(c.pidfile_fd, devnull_fd);
> -	else
> +		log_daemonised = true;
> +	} else {
>  		pidfile_write(c.pidfile_fd, getpid());
> +	}
>  
>  	if (pasta_child_pid)
>  		kill(pasta_child_pid, SIGUSR1);

-- 
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-06-20  4:34 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-20  4:07 [PATCH v4 0/8] Fixes for early logging/prints and related cleanups Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 1/8] conf, passt: Don't try to log to stderr after we close it Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 2/8] conf, passt: Make --stderr do nothing, and deprecate it Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 3/8] conf, log: Instead of abusing log levels, add log_conf_parsed flag Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 4/8] log, passt: Always print to stderr before initialisation is complete Stefano Brivio
2024-06-20  4:34   ` David Gibson [this message]
2024-06-20  4:07 ` [PATCH v4 5/8] log: Add _perror() logging function variants Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 6/8] treewide: Replace perror() calls with calls to logging functions Stefano Brivio
2024-06-20  4:07 ` [PATCH v4 7/8] treewide: Replace strerror() calls Stefano Brivio
2024-06-20  4:08 ` [PATCH v4 8/8] conf, passt: Don't call __openlog() if a log file is used 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=ZnOxPBY2QbErEseq@zatzit \
    --to=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    --cc=yalzhang@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).