From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 4C6065A0050 for ; Thu, 20 Jun 2024 02:49:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718844566; bh=huZAQ1oDKhu7omaWIblBQsmwX/GzRUPZu0D3LjNVLZY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UiqpBxcMZp/DUHJRwe4ZiXCLgVADArUrjM7mRELbjEn7MXjqhZuVbWVH/It7Hagxz SXEq9gOvs0V14SY4VZ/LP8XlrBrBg49vmqNhcOuUBI35G4MOrCjJElUCQkkcyZ39gR SDb7uCYfD++yo8GUaKaAMGPxUbs4H83PlS3mL6bKmh3KWw+24x/2zEIevF2IpHCN2V 0vPpq2+45uqXlFE7uN4iuMm25XTu1U6K55a67ch+4FnLRZKfsK2mJOywbJRHXp0LGv anXXqNpsON/OMl2n17Z8QJ1opTqGUma8pFSCNADRo7qIpdD6WFm1784KwjY7V2k9g6 VHiufkkxuTYxg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W4MMB50BKz4w2Q; Thu, 20 Jun 2024 10:49:26 +1000 (AEST) Date: Thu, 20 Jun 2024 10:44:17 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 4/8] log, passt: Always print to stderr before initialisation is complete Message-ID: References: <20240619194028.2913930-1-sbrivio@redhat.com> <20240619194028.2913930-5-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2JwLpg8+A7aZUZpp" Content-Disposition: inline In-Reply-To: <20240619194028.2913930-5-sbrivio@redhat.com> Message-ID-Hash: T7XAS5CX3G4OMIWJSDRJUIMD4WJDWGAT X-Message-ID-Hash: T7XAS5CX3G4OMIWJSDRJUIMD4WJDWGAT X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Yalan Zhang X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --2JwLpg8+A7aZUZpp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 19, 2024 at 09:40:24PM +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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > If this flag is not set, make sure we always print to stderr, if the > log mask matches. >=20 > Reported-by: Yalan Zhang > Signed-off-by: Stefano Brivio Overall a good simplification, a couple of minor nits > --- > log.c | 17 ++++++++--------- > log.h | 1 + > passt.c | 15 +++++++-------- > 3 files changed, 16 insertions(+), 17 deletions(-) >=20 > diff --git a/log.c b/log.c > index 05b7f80..7018bda 100644 > --- a/log.c > +++ b/log.c > @@ -33,7 +33,6 @@ > static int log_sock =3D -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() */ > =20 > static int log_file =3D -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 */ > =20 > int log_trace; /* --trace mode enabled */ > bool log_conf_parsed; /* Logging options already parsed */ > +bool log_daemon_ready; /* Daemonised, standard error is gone */ I find the name 'log_daemon_ready' kind of confusing (why would the log daemon not be ready?). Maybe "log_daemonised", or "log_background"? > 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); > } > =20 > - if (debug_print || (!log_conf_parsed && !(log_opt & LOG_PERROR))) { > + if (debug_print || !log_conf_parsed || > + (!log_daemon_ready && (log_mask & LOG_MASK(LOG_PRI(pri))))) { > (void)vfprintf(stderr, format, ap); > if (format[strlen(format)] !=3D '\n') > fprintf(stderr, "\n"); > @@ -108,13 +109,15 @@ void trace_init(int enable) > /** > * __openlog() - Non-optional openlog() implementation, for custom vsysl= og() > * @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; > =20 > + (void)option; > + > clock_gettime(CLOCK_REALTIME, &tp); > log_start =3D tp.tv_sec; > =20 > @@ -135,7 +138,6 @@ void __openlog(const char *ident, int option, int fac= ility) > =20 > log_mask |=3D facility; > strncpy(log_ident, ident, sizeof(log_ident) - 1); > - log_opt =3D option; > } > =20 > /** > @@ -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; > =20 > /* Send without timestamp, the system logger should add it */ > - n =3D prefix_len =3D snprintf(buf, BUFSIZ, "<%i> %s: ", pri, log_ident); > + n =3D snprintf(buf, BUFSIZ, "<%i> %s: ", pri, log_ident); > =20 > n +=3D vsnprintf(buf + n, BUFSIZ - n, format, ap); > =20 > if (format[strlen(format)] !=3D '\n') > n +=3D snprintf(buf + n, BUFSIZ - n, "\n"); > =20 > - if (log_opt & LOG_PERROR) > - fprintf(stderr, "%s", buf + prefix_len); > - > if (log_sock >=3D 0 && send(log_sock, buf, n, 0) !=3D n) > fprintf(stderr, "Failed to send %i bytes to syslog\n", n); > } > diff --git a/log.h b/log.h > index 3dab284..1d6dd1d 100644 > --- a/log.h > +++ b/log.h > @@ -30,6 +30,7 @@ void logmsg(int pri, const char *format, ...) > =20 > extern int log_trace; > extern bool log_conf_parsed; > +extern bool log_daemon_ready; > =20 > void trace_init(int enable); > #define trace(...) \ > diff --git a/passt.c b/passt.c > index 19ecd68..c5538bd 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 =3D -1; > + char argv0[PATH_MAX], *name; > struct ctx c =3D { 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 =3D basename(argv0); > if (strstr(name, "pasta")) { > - __openlog(log_name =3D "pasta", LOG_PERROR, LOG_DAEMON); > + __openlog("pasta", 0, LOG_DAEMON); Nice to be rid of those assignments-in-parameters. > =20 > sa.sa_handler =3D pasta_child_handler; > if (sigaction(SIGCHLD, &sa, NULL)) { > @@ -240,7 +240,7 @@ int main(int argc, char **argv) > =20 > c.mode =3D MODE_PASTA; > } else if (strstr(name, "passt")) { > - __openlog(log_name =3D "passt", LOG_PERROR, LOG_DAEMON); > + __openlog("passt", 0, LOG_DAEMON); > =20 > c.mode =3D MODE_PASST; > } else { > @@ -302,13 +302,12 @@ int main(int argc, char **argv) > if (isolate_prefork(&c)) > die("Failed to sandbox process, exiting"); > =20 > - if (!c.foreground) > - __openlog(log_name, 0, LOG_DAEMON); > - > - if (!c.foreground) > + if (!c.foreground) { > __daemon(c.pidfile_fd, devnull_fd); > - else > + log_daemon_ready =3D true; > + } else { > pidfile_write(c.pidfile_fd, getpid()); > + } > =20 > if (pasta_child_pid) > kill(pasta_child_pid, SIGUSR1); --=20 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 --2JwLpg8+A7aZUZpp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZze2AACgkQzQJF27ox 2GdQdg//d176mLsRdk6zeP/7fKwpX+enCyuUSGKpvIiD2LEQxqiDbPlta7yCji4X q8gGlT1KqtRAgu1/HGlpoiVLUP7WdQ9tAjhUPFsjxkMBwfQXrXxSPBQmfU19lrb6 mtJm04VL0OvMExgMjPsLT6SmiBjUqnFt6JQ0+tRSwdk7xz0GUGbPnsKznXoPVJzK IDGIOLdqay/i1iTOc0VHYQQM7Z/2V5uzT6DTsOu3kX8jQruOtb/cujulprycJ8U2 RgyAkwCNlzzZl/9p6hd3qYfuN3Tz6P0NTL1U6UaoC50O17KRUzXesS0+Usc3GNF0 Cb4cvaAggoeslCMjgJXdF0VaGInX/Ohjz3epTvurjWMwhOow7FxM0ZLYQqnGtLSz ZJvDknhDIvD1SsvVAntIS7IfZChwXCBQOpVsavMzKDW1boZZxm+5PDYGMbZJ0WM+ i+9NG7fiArwH+o/viSygoeygItL5mcQB8+9g6jZ5DTcF7s2HRqepLJg3bvcQoGLN phWdUa+0GDsjoOJwhrhqRHJdx0/nV4qnc+kgYteGkMytRNLGwWrZFlcQjFLnfbxH a8WOKspbuBu3ntlyrk6wINAL0hEvldR8LvDH+3FKHIn0foTgkylVc+WJoG2ih61y T5a6Gi0vtUDJueS4KKjlHzxh8Ef+wnIsK1+ZMZv+RJzb2tzBkrs= =QqqG -----END PGP SIGNATURE----- --2JwLpg8+A7aZUZpp--