Re: [iproute PATCH v4] Make colored output configurable

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 16 Aug 2018 11:37:03 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Allow for -color={never,auto,always} to have colored output disabled,
> enabled only if stdout is a terminal or enabled regardless of stdout
> state.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Allow to override isatty() check by specifying '-color' flag more than
>   once.
> - Document new behaviour in man pages.
> 
> Changes since v2:
> - Implement new -color=foo syntax.
> - Update commit message and man page texts accordingly.
> 
> Changes since v3:
> - Fix typo in tc/tc.c causing compile error.
> ---
>  bridge/bridge.c   |  3 +--
>  include/color.h   |  7 +++++++
>  ip/ip.c           |  3 +--
>  lib/color.c       | 33 ++++++++++++++++++++++++++++++++-
>  man/man8/bridge.8 | 13 +++++++++++--
>  man/man8/ip.8     | 13 +++++++++++--
>  man/man8/tc.8     | 13 +++++++++++--
>  tc/tc.c           |  3 +--
>  8 files changed, 75 insertions(+), 13 deletions(-)
> 
> diff --git a/bridge/bridge.c b/bridge/bridge.c
> index 451d684e0bcfd..e35e5bdf7fb30 100644
> --- a/bridge/bridge.c
> +++ b/bridge/bridge.c
> @@ -173,8 +173,7 @@ main(int argc, char **argv)
>  			NEXT_ARG();
>  			if (netns_switch(argv[1]))
>  				exit(-1);
> -		} else if (matches(opt, "-color") == 0) {
> -			++color;
> +		} else if (matches_color(opt, &color) == 0) {
>  		} else if (matches(opt, "-compressvlans") == 0) {
>  			++compress_vlans;
>  		} else if (matches(opt, "-force") == 0) {
> diff --git a/include/color.h b/include/color.h
> index 4f2c918db7e43..42038dc2e7f87 100644
> --- a/include/color.h
> +++ b/include/color.h
> @@ -12,8 +12,15 @@ enum color_attr {
>  	COLOR_NONE
>  };
>  
> +enum color_opt {
> +	COLOR_OPT_NEVER = 0,
> +	COLOR_OPT_AUTO = 1,
> +	COLOR_OPT_ALWAYS = 2
> +};
> +
>  void enable_color(void);
>  int check_enable_color(int color, int json);
> +int matches_color(const char *arg, int *val);
>  void set_color_palette(void);
>  int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...);
>  enum color_attr ifa_family_color(__u8 ifa_family);
> diff --git a/ip/ip.c b/ip/ip.c
> index 38eac5ec1e17d..893c3c43ef99a 100644
> --- a/ip/ip.c
> +++ b/ip/ip.c
> @@ -283,8 +283,7 @@ int main(int argc, char **argv)
>  				exit(-1);
>  			}
>  			rcvbuf = size;
> -		} else if (matches(opt, "-color") == 0) {
> -			++color;
> +		} else if (matches_color(opt, &color) == 0) {
>  		} else if (matches(opt, "-help") == 0) {
>  			usage();
>  		} else if (matches(opt, "-netns") == 0) {
> diff --git a/lib/color.c b/lib/color.c
> index edf96e5c6ecd7..3ad1d6d647722 100644
> --- a/lib/color.c
> +++ b/lib/color.c
> @@ -3,11 +3,13 @@
>  #include <stdarg.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <unistd.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
>  #include <linux/if.h>
>  
>  #include "color.h"
> +#include "utils.h"
>  
>  enum color {
>  	C_RED,
> @@ -79,13 +81,42 @@ void enable_color(void)
>  
>  int check_enable_color(int color, int json)
>  {
> -	if (color && !json) {
> +	if (json || color == COLOR_OPT_NEVER)
> +		return 1;
> +
> +	if (color == COLOR_OPT_ALWAYS || isatty(fileno(stdout))) {
>  		enable_color();
>  		return 0;
>  	}
>  	return 1;
>  }
>  
> +int matches_color(const char *arg, int *val)
> +{
> +	char *dup, *p;
> +
> +	if (!val)
> +		return 1;

I am fine with this even for current version (not next).
Minor nit, is that these functions should be bool.
Most of the iproute2 uses int for booleans for historical reasons
but lets try use bool for new code.