[PATCH v2] conf, log: On -h / --help, print usage to stdout, not stderr

[PATCH v2] conf, log: On -h / --help, print usage to stdout, not stderr

[Stefano Brivio]

Erik suggests that this makes it easier to grep for options, and with
--help we're anyway printing usage information as expected, not as
part of an error report.

While at it: on -h, we should exit with 0.

Reported-by: Erik Sjölund <erik.sjolund@gmail.com>
Link: https://bugs.passt.top/show_bug.cgi?id=52
Link: https://bugs.passt.top/show_bug.cgi?id=53
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2: Exit with 0, non with 1 as also reported by Erik

 conf.c | 23 ++++++++++++++++++-----
 log.c  |  8 +++++---
 log.h  |  1 +
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/conf.c b/conf.c
index ffff235..87dec50 100644
--- a/conf.c
+++ b/conf.c
@@ -717,10 +717,11 @@ static unsigned int conf_ip6(unsigned int ifi,
 }
 
 /**
- * usage() - Print usage and exit
+ * print_usage() - Print usage, exit with given status code
  * @name:	Executable name
+ * @status:	Status code for exit()
  */
-static void usage(const char *name)
+static void print_usage(const char *name, int status)
 {
 	if (strstr(name, "pasta")) {
 		info("Usage: %s [OPTION]... [COMMAND] [ARGS]...", name);
@@ -856,7 +857,7 @@ static void usage(const char *name)
 	info(   "    SPEC is as described for TCP above");
 	info(   "    default: none");
 
-	exit(EXIT_FAILURE);
+	exit(status);
 
 pasta_opts:
 
@@ -906,7 +907,16 @@ pasta_opts:
 	info(   "			Don't copy all addresses to namespace");
 	info(   "  --ns-mac-addr ADDR	Set MAC address on tap interface");
 
-	exit(EXIT_FAILURE);
+	exit(status);
+}
+
+/**
+ * usage() - Print usage and exit with failure
+ * @name:	Executable name
+ */
+static void usage(const char *name)
+{
+	print_usage(name, EXIT_FAILURE);
 }
 
 /**
@@ -1630,8 +1640,11 @@ void conf(struct ctx *c, int argc, char **argv)
 		case 'U':
 			/* Handle these later, once addresses are configured */
 			break;
-		case '?':
 		case 'h':
+			log_to_stdout = 1;
+			print_usage(argv[0], EXIT_SUCCESS);
+			break;
+		case '?':
 		default:
 			usage(argv[0]);
 			break;
diff --git a/log.c b/log.c
index 3a3d101..63d7801 100644
--- a/log.c
+++ b/log.c
@@ -43,17 +43,19 @@ static char	log_header[BUFSIZ];	/* File header, written back on cuts */
 
 static time_t	log_start;		/* Start timestamp */
 int		log_trace;		/* --trace mode enabled */
+int		log_to_stdout;		/* Print to stdout instead of stderr */
 
 #define BEFORE_DAEMON		(setlogmask(0) == LOG_MASK(LOG_EMERG))
 
 #define logfn(name, level)						\
 void name(const char *format, ...) {					\
+	FILE *out = log_to_stdout ? stdout : stderr;			\
 	struct timespec tp;						\
 	va_list args;							\
 									\
 	if (setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) {	\
 		clock_gettime(CLOCK_REALTIME, &tp);			\
-		fprintf(stderr, "%lli.%04lli: ",			\
+		fprintf(out, "%lli.%04lli: ",				\
 			(long long int)tp.tv_sec - log_start,		\
 			(long long int)tp.tv_nsec / (100L * 1000));	\
 	}								\
@@ -70,10 +72,10 @@ void name(const char *format, ...) {					\
 	if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) ||	\
 	    (BEFORE_DAEMON && !(log_opt & LOG_PERROR))) {		\
 		va_start(args, format);					\
-		(void)vfprintf(stderr, format, args); 			\
+		(void)vfprintf(out, format, args); 			\
 		va_end(args);						\
 		if (format[strlen(format)] != '\n')			\
-			fprintf(stderr, "\n");				\
+			fprintf(out, "\n");				\
 	}								\
 }
 
diff --git a/log.h b/log.h
index 3aab29d..a17171a 100644
--- a/log.h
+++ b/log.h
@@ -22,6 +22,7 @@ void debug(const char *format, ...);
 	} while (0)
 
 extern int log_trace;
+extern int log_to_stdout;
 void trace_init(int enable);
 #define trace(...)							\
 	do {								\
-- 
@@ -22,6 +22,7 @@ void debug(const char *format, ...);
 	} while (0)
 
 extern int log_trace;
+extern int log_to_stdout;
 void trace_init(int enable);
 #define trace(...)							\
 	do {								\
-- 
2.39.2

Re: [PATCH v2] conf, log: On -h / --help, print usage to stdout, not stderr

[David Gibson]

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

On Mon, Jun 05, 2023 at 07:19:50AM +0200, Stefano Brivio wrote:
> Erik suggests that this makes it easier to grep for options, and with
> --help we're anyway printing usage information as expected, not as
> part of an error report.
> 
> While at it: on -h, we should exit with 0.
> 
> Reported-by: Erik Sjölund <erik.sjolund@gmail.com>
> Link: https://bugs.passt.top/show_bug.cgi?id=52
> Link: https://bugs.passt.top/show_bug.cgi?id=53
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v2: Exit with 0, non with 1 as also reported by Erik

Still kinda nasty, and it'll still do.

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

> 
>  conf.c | 23 ++++++++++++++++++-----
>  log.c  |  8 +++++---
>  log.h  |  1 +
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index ffff235..87dec50 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -717,10 +717,11 @@ static unsigned int conf_ip6(unsigned int ifi,
>  }
>  
>  /**
> - * usage() - Print usage and exit
> + * print_usage() - Print usage, exit with given status code
>   * @name:	Executable name
> + * @status:	Status code for exit()
>   */
> -static void usage(const char *name)
> +static void print_usage(const char *name, int status)
>  {
>  	if (strstr(name, "pasta")) {
>  		info("Usage: %s [OPTION]... [COMMAND] [ARGS]...", name);
> @@ -856,7 +857,7 @@ static void usage(const char *name)
>  	info(   "    SPEC is as described for TCP above");
>  	info(   "    default: none");
>  
> -	exit(EXIT_FAILURE);
> +	exit(status);
>  
>  pasta_opts:
>  
> @@ -906,7 +907,16 @@ pasta_opts:
>  	info(   "			Don't copy all addresses to namespace");
>  	info(   "  --ns-mac-addr ADDR	Set MAC address on tap interface");
>  
> -	exit(EXIT_FAILURE);
> +	exit(status);
> +}
> +
> +/**
> + * usage() - Print usage and exit with failure
> + * @name:	Executable name
> + */
> +static void usage(const char *name)
> +{
> +	print_usage(name, EXIT_FAILURE);
>  }
>  
>  /**
> @@ -1630,8 +1640,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  		case 'U':
>  			/* Handle these later, once addresses are configured */
>  			break;
> -		case '?':
>  		case 'h':
> +			log_to_stdout = 1;
> +			print_usage(argv[0], EXIT_SUCCESS);
> +			break;
> +		case '?':
>  		default:
>  			usage(argv[0]);
>  			break;
> diff --git a/log.c b/log.c
> index 3a3d101..63d7801 100644
> --- a/log.c
> +++ b/log.c
> @@ -43,17 +43,19 @@ static char	log_header[BUFSIZ];	/* File header, written back on cuts */
>  
>  static time_t	log_start;		/* Start timestamp */
>  int		log_trace;		/* --trace mode enabled */
> +int		log_to_stdout;		/* Print to stdout instead of stderr */
>  
>  #define BEFORE_DAEMON		(setlogmask(0) == LOG_MASK(LOG_EMERG))
>  
>  #define logfn(name, level)						\
>  void name(const char *format, ...) {					\
> +	FILE *out = log_to_stdout ? stdout : stderr;			\
>  	struct timespec tp;						\
>  	va_list args;							\
>  									\
>  	if (setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) {	\
>  		clock_gettime(CLOCK_REALTIME, &tp);			\
> -		fprintf(stderr, "%lli.%04lli: ",			\
> +		fprintf(out, "%lli.%04lli: ",				\
>  			(long long int)tp.tv_sec - log_start,		\
>  			(long long int)tp.tv_nsec / (100L * 1000));	\
>  	}								\
> @@ -70,10 +72,10 @@ void name(const char *format, ...) {					\
>  	if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) ||	\
>  	    (BEFORE_DAEMON && !(log_opt & LOG_PERROR))) {		\
>  		va_start(args, format);					\
> -		(void)vfprintf(stderr, format, args); 			\
> +		(void)vfprintf(out, format, args); 			\
>  		va_end(args);						\
>  		if (format[strlen(format)] != '\n')			\
> -			fprintf(stderr, "\n");				\
> +			fprintf(out, "\n");				\
>  	}								\
>  }
>  
> diff --git a/log.h b/log.h
> index 3aab29d..a17171a 100644
> --- a/log.h
> +++ b/log.h
> @@ -22,6 +22,7 @@ void debug(const char *format, ...);
>  	} while (0)
>  
>  extern int log_trace;
> +extern int log_to_stdout;
>  void trace_init(int enable);
>  #define trace(...)							\
>  	do {								\

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