Re: [PATCH iproute2-next] allow overriding color option in environment

[Andrea Claudi <aclaudi@redhat.com>]

On Fri, Sep 15, 2023 at 02:07:00PM -0700, Stephen Hemminger wrote:
> For ip, tc, and bridge command introduce a new way to enable
> automatic colorization via environment variable. The default
> is what ever is in the config.
> 
> Example:
>   $ IP_COLOR=auto ip -br show addr
> 
> The idea is that distributions can ship with the same default
> as before "none" but that users can override if needed
> without resorting to aliasing every command.
> 

I'm OK with this, but can we please use a single environment variable to
control color output for all iproute tools? I can't see why anyone would
want to have color output on by default for ip and not for tc and
bridge...

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  bridge/bridge.c   |  2 +-
>  include/color.h   |  1 +
>  ip/ip.c           |  2 +-
>  lib/color.c       | 36 +++++++++++++++++++++++++++---------
>  man/man8/bridge.8 |  7 +++++++
>  man/man8/ip.8     | 14 +++++++++-----
>  man/man8/tc.8     |  6 ++++++
>  tc/tc.c           |  2 +-
>  8 files changed, 53 insertions(+), 17 deletions(-)
> 
> diff --git a/bridge/bridge.c b/bridge/bridge.c
> index 339101a874b1..f9a245cb3670 100644
> --- a/bridge/bridge.c
> +++ b/bridge/bridge.c
> @@ -102,7 +102,7 @@ static int batch(const char *name)
>  int
>  main(int argc, char **argv)
>  {
> -	int color = CONF_COLOR;
> +	int color = default_color("BRIDGE_COLOR");
>  
>  	while (argc > 1) {
>  		const char *opt = argv[1];
> diff --git a/include/color.h b/include/color.h
> index 17ec56f3d7b4..8eea534f38e1 100644
> --- a/include/color.h
> +++ b/include/color.h
> @@ -20,6 +20,7 @@ enum color_opt {
>  	COLOR_OPT_ALWAYS = 2
>  };
>  
> +int default_color(const char *argv0);
>  bool check_enable_color(int color, int json);
>  bool matches_color(const char *arg, int *val);
>  int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...);
> diff --git a/ip/ip.c b/ip/ip.c
> index 860ff957c3b3..e15d5fe52d92 100644
> --- a/ip/ip.c
> +++ b/ip/ip.c
> @@ -168,7 +168,7 @@ int main(int argc, char **argv)
>  	const char *libbpf_version;
>  	char *batch_file = NULL;
>  	char *basename;
> -	int color = CONF_COLOR;
> +	int color = default_color("IP_COLOR");
>  
>  	/* to run vrf exec without root, capabilities might be set, drop them
>  	 * if not needed as the first thing.
> diff --git a/lib/color.c b/lib/color.c
> index 59976847295c..9a579f34977d 100644
> --- a/lib/color.c
> +++ b/lib/color.c
> @@ -93,6 +93,32 @@ bool check_enable_color(int color, int json)
>  	return false;
>  }
>  
> +static bool match_color_value(const char *arg, int *val)
> +{
> +	if (*arg == '\0' || !strcmp(arg, "always"))

This actually produces an unexpected result.

IP_COLOR= ip address

results in colorized output, while it should be colorless, in my
opinion. However this works ok when this code path is exercised by the
'--color' option, as with:

ip -c address

users actually expect to have color on the output.

> +		*val = COLOR_OPT_ALWAYS;
> +	else if (!strcmp(arg, "auto"))
> +		*val = COLOR_OPT_AUTO;
> +	else if (!strcmp(arg, "never"))
> +		*val = COLOR_OPT_NEVER;
> +	else
> +		return false;
> +	return true;
> +}
> +
> +int default_color(const char *env)
> +{
> +	char *name;
> +	int val;
> +	size_t i;
> +

You may want to get rid of the i var above:

color.c: In function ‘default_color’:
color.c:113:16: warning: unused variable ‘i’ [-Wunused-variable]
  113 |         size_t i;
      |                ^

> +	name = getenv(env);
> +	if (name && match_color_value(name, &val))
> +		return val;
> +
> +	return CONF_COLOR;
> +}
> +
>  bool matches_color(const char *arg, int *val)
>  {
>  	char *dup, *p;
> @@ -108,15 +134,7 @@ bool matches_color(const char *arg, int *val)
>  	if (matches(dup, "-color"))
>  		return false;
>  
> -	if (*p == '\0' || !strcmp(p, "always"))
> -		*val = COLOR_OPT_ALWAYS;
> -	else if (!strcmp(p, "auto"))
> -		*val = COLOR_OPT_AUTO;
> -	else if (!strcmp(p, "never"))
> -		*val = COLOR_OPT_NEVER;
> -	else
> -		return false;
> -	return true;
> +	return match_color_value(p, val);
>  }
>  
>  static void set_color_palette(void)
> diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
> index c52c9331e2c2..58bb1ddbd26a 100644
> --- a/man/man8/bridge.8
> +++ b/man/man8/bridge.8
> @@ -319,6 +319,13 @@ precedence. This flag is ignored if
>  .B \-json
>  is also given.
>  
> +
> +The default color setting is
> +.B never
> +but can be overridden by the
> +.B BRIDGE_COLOR
> +environment variable.
> +
>  .TP
>  .BR "\-j", " \-json"
>  Output results in JavaScript Object Notation (JSON).
> diff --git a/man/man8/ip.8 b/man/man8/ip.8
> index 72227d44fd30..df572f47d96d 100644
> --- a/man/man8/ip.8
> +++ b/man/man8/ip.8
> @@ -197,11 +197,15 @@ precedence. This flag is ignored if
>  .B \-json
>  is also given.
>  
> -Used color palette can be influenced by
> -.BR COLORFGBG
> -environment variable
> -(see
> -.BR ENVIRONMENT ).
> +The default color setting is
> +.B never
> +but can be overridden by the
> +.B IP_COLOR
> +environment variable.
> +
> +The color palette used can be adjusted with
> +.B COLORFGBG
> +environment variable.
>  
>  .TP
>  .BR "\-t" , " \-timestamp"
> diff --git a/man/man8/tc.8 b/man/man8/tc.8
> index d436d46472af..39ac6dcd1631 100644
> --- a/man/man8/tc.8
> +++ b/man/man8/tc.8
> @@ -805,6 +805,12 @@ precedence. This flag is ignored if
>  .B \-json
>  is also given.
>  
> +The default color setting is
> +.B never
> +but can be overridden by the
> +.B TC_COLOR
> +environment variable.
> +
>  .TP
>  .BR "\-j", " \-json"
>  Display results in JSON format.
> diff --git a/tc/tc.c b/tc/tc.c
> index 082c6677d34a..b7cd60d68a38 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -253,7 +253,7 @@ int main(int argc, char **argv)
>  {
>  	const char *libbpf_version;
>  	char *batch_file = NULL;
> -	int color = CONF_COLOR;
> +	int color = default_color("TC_COLOR");
>  	int ret;
>  
>  	while (argc > 1) {
> -- 
> 2.39.2
>