Re: [PATCH v3 2/8] conf, passt: Make --stderr do nothing, and deprecate it

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

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

On Wed, Jun 19, 2024 at 09:40:22PM +0200, Stefano Brivio wrote:
> The original behaviour of printing messages to standard error by
> default when running from a non-interactive terminal was introduced
> because the first KubeVirt integration draft used to start passt in
> foreground and get messages via standard error.
> 
> For development purposes, the system logger was more convenient at
> that point, and passt was running from interactive terminals only if
> not started by the KubeVirt integration.
> 
> This behaviour was introduced by 84a62b79a2bc ("passt: Also log to
> stderr, don't fork to background if not interactive").
> 
> Later, I added command-line options in 1e49d194d017 ("passt, pasta:
> Introduce command-line options and port re-mapping") and accidentally
> reversed this condition, which wasn't a problem as --stderr could
> force printing to standard error anyway (and it was used by KubeVirt).
> 
> Nowadays, the KubeVirt integration uses a log file (requested via
> libvirt configuration), and the same applies for Podman if one
> actually needs to look at runtime logs. There are no use cases left,
> as far as I know, where passt runs in foreground in non-interactive
> terminals.
> 
> Seize the chance to reintroduce some sanity here. If we fork to
> background, standard error is closed, so --stderr is useless in that
> case.
> 
> If we run in foreground, there's no harm in printing messages to
> standard error, and that accidentally became the default behaviour
> anyway, so --stderr is not needed in that case.
> 
> It would be needed for non-interactive terminals, but there are no
> use cases, and if there were, let's log to standard error anyway:
> the user can always redirect standard error to /dev/null if needed.
> 
> Before we're up and running, we need to print to standard error anyway
> if something happens, otherwise we can't report failure to start in
> any kind of usage, stand-alone or in integrations.
> 
> So, make --stderr do nothing, and deprecate it.
> 
> While at it, drop a left-over comment about --foreground being the
> default only for interactive terminals, because it's not the case
> anymore.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  conf.c  | 18 ++----------------
>  passt.1 |  9 +++++----
>  passt.c |  2 +-
>  passt.h |  2 --
>  4 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index dbdbb62..9f869f5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -730,9 +730,7 @@ static void usage(const char *name, FILE *f, int status)
>  		"      --trace		Be extra verbose, implies --debug\n"
>  		"  -q, --quiet		Don't print informational messages\n"
>  		"  -f, --foreground	Don't run in background\n"
> -		"    default: run in background if started from a TTY\n"
> -		"  -e, --stderr		Log to stderr too\n"
> -		"    default: log to system logger only if started from a TTY\n"
> +		"    default: run in background\n"
>  		"  -l, --log-file PATH	Log (only) to given file\n"
>  		"  --log-size BYTES	Maximum size of log file\n"
>  		"    default: 1 MiB\n"
> @@ -1405,18 +1403,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->debug = 1;
>  			break;
>  		case 'e':
> -			if (logfile)
> -				die("Can't log to both file and stderr");
> -
> -			if (c->force_stderr)
> -				die("Multiple --stderr options given");
> -
> -			c->force_stderr = 1;
> +			warn("--stderr will be dropped soon");
>  			break;
>  		case 'l':
> -			if (c->force_stderr)
> -				die("Can't log to both stderr and file");
> -
>  			if (logfile)
>  				die("Multiple --log-file options given");
>  
> @@ -1693,9 +1682,6 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  	conf_ugid(runas, &uid, &gid);
>  
> -	if (!c->foreground && c->force_stderr)
> -		die("Can't log to standard error if not running in foreground");
> -
>  	if (logfile) {
>  		logfile_init(c->mode == MODE_PASTA ? "pasta" : "passt",
>  			     logfile, logsize);
> diff --git a/passt.1 b/passt.1
> index 7676fe3..6c8f932 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -93,13 +93,14 @@ Default is to fork into background.
>  
>  .TP
>  .BR \-e ", " \-\-stderr
> -Log to standard error too.
> -Default is to log to the system logger only, if started from an interactive
> -terminal, and to both system logger and standard error otherwise.
> +This option has no effect, and is maintained for compatibility purposes only.
> +
> +Note that this configuration option is \fBdeprecated\fR and will be removed in a
> +future version.
>  
>  .TP
>  .BR \-l ", " \-\-log-file " " \fIPATH\fR
> -Log to file \fIPATH\fR, not to standard error, and not to the system logger.
> +Log to file \fIPATH\fR, and not to the system logger.
>  
>  .TP
>  .BR \-\-log-size " " \fISIZE\fR
> diff --git a/passt.c b/passt.c
> index aa9648a..19ecd68 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -302,7 +302,7 @@ int main(int argc, char **argv)
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
>  
> -	if (!c.foreground || (!c.force_stderr && !isatty(fileno(stderr))))
> +	if (!c.foreground)
>  		__openlog(log_name, 0, LOG_DAEMON);
>  
>  	if (!c.foreground)
> diff --git a/passt.h b/passt.h
> index 46d073a..21cf4c1 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -180,7 +180,6 @@ struct ip6_ctx {
>   * @trace:		Enable tracing (extra debug) mode
>   * @quiet:		Don't print informational messages
>   * @foreground:		Run in foreground, don't log to stderr by default
> - * @force_stderr:	Force logging to stderr
>   * @nofile:		Maximum number of open files (ulimit -n)
>   * @sock_path:		Path for UNIX domain socket
>   * @pcap:		Path for packet capture file
> @@ -231,7 +230,6 @@ struct ctx {
>  	int trace;
>  	int quiet;
>  	int foreground;
> -	int force_stderr;
>  	int nofile;
>  	char sock_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];

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