[iproute PATCH 0/4] A bunch of fixes regarding colored output

[iproute PATCH 0/4] A bunch of fixes regarding colored output

[Phil Sutter]

This series contains fixes for conditionally colored output in patches 1
and 2. Patch 3 merges the common conditionals from ip, tc and bridge
tools. Patch 4 then adds a further restriction to colored output to
prevent garbled output when redirecting into a file.

Phil Sutter (4):
  tc: Fix typo in check for colored output
  bridge: Fix check for colored output
  Merge common code for conditionally colored output
  lib: Enable colored output only for TTYs

 bridge/bridge.c |  5 ++---
 include/color.h |  1 +
 ip/ip.c         |  3 +--
 lib/color.c     | 10 ++++++++++
 tc/tc.c         |  3 +--
 5 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.18.0

[iproute PATCH 1/4] tc: Fix typo in check for colored output

[Phil Sutter]

The check used binary instead of boolean AND, which means colored output
was enabled only if the number of specified '-color' flags was odd.

Fixes: 2d165c0811058 ("tc: implement color output")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/tc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/tc.c b/tc/tc.c
index 3bb5910ffac52..3bb893756f40e 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -515,7 +515,7 @@ int main(int argc, char **argv)
 
 	_SL_ = oneline ? "\\" : "\n";
 
-	if (color & !json)
+	if (color && !json)
 		enable_color();
 
 	if (batch_file)
-- 
2.18.0

[iproute PATCH 2/4] bridge: Fix check for colored output

[Phil Sutter]

There is no point in calling enable_color() conditionally if it was
already called for each time '-color' flag was parsed. Align the
algorithm with that in ip and tc by actually making use of 'color'
variable.

Fixes: e9625d6aead11 ("Merge branch 'iproute2-master' into iproute2-next")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 bridge/bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index 7fcfe1116f6e5..289a157d37f03 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -174,7 +174,7 @@ main(int argc, char **argv)
 			if (netns_switch(argv[1]))
 				exit(-1);
 		} else if (matches(opt, "-color") == 0) {
-			enable_color();
+			++color;
 		} else if (matches(opt, "-compressvlans") == 0) {
 			++compress_vlans;
 		} else if (matches(opt, "-force") == 0) {
-- 
2.18.0

[iproute PATCH 3/4] Merge common code for conditionally colored output

[Phil Sutter]

Instead of calling enable_color() conditionally with identical check in
three places, introduce check_enable_color() which does it in one place.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 bridge/bridge.c | 3 +--
 include/color.h | 1 +
 ip/ip.c         | 3 +--
 lib/color.c     | 9 +++++++++
 tc/tc.c         | 3 +--
 5 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index 289a157d37f03..451d684e0bcfd 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -200,8 +200,7 @@ main(int argc, char **argv)
 
 	_SL_ = oneline ? "\\" : "\n";
 
-	if (color && !json)
-		enable_color();
+	check_enable_color(color, json);
 
 	if (batch_file)
 		return batch(batch_file);
diff --git a/include/color.h b/include/color.h
index c80359d3e2e95..4f2c918db7e43 100644
--- a/include/color.h
+++ b/include/color.h
@@ -13,6 +13,7 @@ enum color_attr {
 };
 
 void enable_color(void);
+int check_enable_color(int color, int json);
 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 71d5170c0cc23..38eac5ec1e17d 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -304,8 +304,7 @@ int main(int argc, char **argv)
 
 	_SL_ = oneline ? "\\" : "\n";
 
-	if (color && !json)
-		enable_color();
+	check_enable_color(color, json);
 
 	if (batch_file)
 		return batch(batch_file);
diff --git a/lib/color.c b/lib/color.c
index da1f516cb2492..edf96e5c6ecd7 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -77,6 +77,15 @@ void enable_color(void)
 	set_color_palette();
 }
 
+int check_enable_color(int color, int json)
+{
+	if (color && !json) {
+		enable_color();
+		return 0;
+	}
+	return 1;
+}
+
 void set_color_palette(void)
 {
 	char *p = getenv("COLORFGBG");
diff --git a/tc/tc.c b/tc/tc.c
index 3bb893756f40e..e775550174473 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -515,8 +515,7 @@ int main(int argc, char **argv)
 
 	_SL_ = oneline ? "\\" : "\n";
 
-	if (color && !json)
-		enable_color();
+	check_enable_color(color, json);
 
 	if (batch_file)
 		return batch(batch_file);
-- 
2.18.0

[iproute PATCH 4/4] lib: Enable colored output only for TTYs

[Phil Sutter]

Add an additional prerequisite to check_enable_color() to make sure
stdout actually points to an open TTY device. Otherwise calls like

| ip -color a s >/tmp/foo

will print color escape sequences into that file.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/color.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/color.c b/lib/color.c
index edf96e5c6ecd7..500ba09682697 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -3,6 +3,7 @@
 #include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 #include <sys/socket.h>
 #include <sys/types.h>
 #include <linux/if.h>
@@ -79,7 +80,7 @@ void enable_color(void)
 
 int check_enable_color(int color, int json)
 {
-	if (color && !json) {
+	if (color && !json && isatty(fileno(stdout))) {
 		enable_color();
 		return 0;
 	}
-- 
2.18.0

Re: [iproute PATCH 4/4] lib: Enable colored output only for TTYs

[David Ahern]

On 8/15/18 3:06 AM, Phil Sutter wrote:
> Add an additional prerequisite to check_enable_color() to make sure
> stdout actually points to an open TTY device. Otherwise calls like
> 
> | ip -color a s >/tmp/foo
> 
> will print color escape sequences into that file.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  lib/color.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/color.c b/lib/color.c
> index edf96e5c6ecd7..500ba09682697 100644
> --- a/lib/color.c
> +++ b/lib/color.c
> @@ -3,6 +3,7 @@
>  #include <stdarg.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <unistd.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
>  #include <linux/if.h>
> @@ -79,7 +80,7 @@ void enable_color(void)
>  
>  int check_enable_color(int color, int json)
>  {
> -	if (color && !json) {
> +	if (color && !json && isatty(fileno(stdout))) {
>  		enable_color();
>  		return 0;
>  	}
> 

This also disables color sequence when the output is piped to a pager
such as less which with the -R argument can handle it just fine.

ie., the user needs to remove the color arg when that output is not wanted.

Re: [iproute PATCH 4/4] lib: Enable colored output only for TTYs

[Stephen Hemminger]

On Wed, 15 Aug 2018 08:40:20 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 8/15/18 3:06 AM, Phil Sutter wrote:
> > Add an additional prerequisite to check_enable_color() to make sure
> > stdout actually points to an open TTY device. Otherwise calls like
> > 
> > | ip -color a s >/tmp/foo
> > 
> > will print color escape sequences into that file.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  lib/color.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/color.c b/lib/color.c
> > index edf96e5c6ecd7..500ba09682697 100644
> > --- a/lib/color.c
> > +++ b/lib/color.c
> > @@ -3,6 +3,7 @@
> >  #include <stdarg.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <unistd.h>
> >  #include <sys/socket.h>
> >  #include <sys/types.h>
> >  #include <linux/if.h>
> > @@ -79,7 +80,7 @@ void enable_color(void)
> >  
> >  int check_enable_color(int color, int json)
> >  {
> > -	if (color && !json) {
> > +	if (color && !json && isatty(fileno(stdout))) {
> >  		enable_color();
> >  		return 0;
> >  	}
> >   
> 
> This also disables color sequence when the output is piped to a pager
> such as less which with the -R argument can handle it just fine.
> 
> ie., the user needs to remove the color arg when that output is not wanted.

If you are going to change the color enabling, why not make it compatible
with what ls does?

From man ls(1) (and  grep)

       --color[=WHEN]
              colorize  the output; WHEN can be 'always' (default if omitted),
              'auto', or 'never'; more info below
...

       Using  color  to distinguish file types is disabled both by default and
       with --color=never.  With --color=auto, ls emits color codes only  when
       standard  output is connected to a terminal.  The LS_COLORS environment
       variable can change the settings.  Use the dircolors command to set it.


Make -c be same as --color=always to keep compatiablity

RE: [iproute PATCH 4/4] lib: Enable colored output only for TTYs

[David Laight]

From: Stephen Hemminger
> Sent: 15 August 2018 16:04
...
> > This also disables color sequence when the output is piped to a pager
> > such as less which with the -R argument can handle it just fine.
> >
> > ie., the user needs to remove the color arg when that output is not wanted.
> 
> If you are going to change the color enabling, why not make it compatible
> with what ls does?

Indeed - otherwise it is very hard to debug the colour escape sequences.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)