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

[iproute PATCH v2 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.

Changes since v1:
- Adjusted last patch according to feedback. Details given in changelog
  of that patch.

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       | 13 +++++++++++++
 man/man8/bridge.8 |  5 ++++-
 man/man8/ip.8     |  5 ++++-
 man/man8/tc.8     |  5 ++++-
 tc/tc.c           |  3 +--
 8 files changed, 30 insertions(+), 10 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 v2 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. Allow to override this
check by specifying '-color' flag more than once.

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.
---
 lib/color.c       | 6 +++++-
 man/man8/bridge.8 | 5 ++++-
 man/man8/ip.8     | 5 ++++-
 man/man8/tc.8     | 5 ++++-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/lib/color.c b/lib/color.c
index edf96e5c6ecd7..3a66d8ccb4b00 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,10 @@ void enable_color(void)
 
 int check_enable_color(int color, int json)
 {
-	if (color && !json) {
+	if (json || !color)
+		return 1;
+
+	if (color > 1 || isatty(fileno(stdout))) {
 		enable_color();
 		return 0;
 	}
diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index e7f7148315e19..b865e8b6cd771 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -171,7 +171,10 @@ return code will be non zero.
 
 .TP
 .BR "\-c" , " -color"
-Use color output.
+Use color output if stdout is a terminal. Specify twice to enable color output
+irrespective of stdout state. This flag is ignored if
+.B \-json
+is also given.
 
 .TP
 .BR "\-j", " \-json"
diff --git a/man/man8/ip.8 b/man/man8/ip.8
index 0087d18b74706..c5484bbc11483 100644
--- a/man/man8/ip.8
+++ b/man/man8/ip.8
@@ -188,7 +188,10 @@ executes specified command over all objects, it depends if command supports this
 
 .TP
 .BR "\-c" , " -color"
-Use color output.
+Use color output if stdout is a terminal. Specify twice to enable color output
+irrespective of stdout state. This flag is ignored if
+.B \-json
+is also given.
 
 .TP
 .BR "\-t" , " \-timestamp"
diff --git a/man/man8/tc.8 b/man/man8/tc.8
index 840880fbdba63..cbe7ac1847bbb 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -730,7 +730,10 @@ option.
 
 .TP
 .BR "\ -color"
-Use color output.
+Use color output if stdout is a terminal. Specify twice to enable color output
+irrespective of stdout state. This flag is ignored if
+.B \-json
+is also given.
 
 .TP
 .BR "\-j", " \-json"
-- 
2.18.0

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

[David Ahern]

On 8/15/18 10:21 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. Allow to override this
> check by specifying '-color' flag more than once.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Allow to override isatty() check by specifying '-color' flag more than
>   once.

That adds overhead to my workflow where I almost always have to pipe the
output of ip to a pager.

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

[Phil Sutter]

On Wed, Aug 15, 2018 at 10:24:31AM -0600, David Ahern wrote:
> On 8/15/18 10:21 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. Allow to override this
> > check by specifying '-color' flag more than once.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v1:
> > - Allow to override isatty() check by specifying '-color' flag more than
> >   once.
> 
> That adds overhead to my workflow where I almost always have to pipe the
> output of ip to a pager.

alias ip='ip -color -color'

Another alternative may be to introduce -autocolor flag. Establishing
the same syntax as used by 'ls' is not as trivial due to the simple
commandline parsing used in 'ip'.

Cheers, Phil

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

[David Ahern]

On 8/15/18 10:39 AM, Phil Sutter wrote:
> On Wed, Aug 15, 2018 at 10:24:31AM -0600, David Ahern wrote:
>> On 8/15/18 10:21 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. Allow to override this
>>> check by specifying '-color' flag more than once.
>>>
>>> Signed-off-by: Phil Sutter <phil@nwl.cc>
>>> ---
>>> Changes since v1:
>>> - Allow to override isatty() check by specifying '-color' flag more than
>>>   once.
>>
>> That adds overhead to my workflow where I almost always have to pipe the
>> output of ip to a pager.
> 
> alias ip='ip -color -color'

no. Don't impact existing users.

> 
> Another alternative may be to introduce -autocolor flag. Establishing
> the same syntax as used by 'ls' is not as trivial due to the simple
> commandline parsing used in 'ip'.

I disagree with ignoring or overriding an argument a user passes in. You
are guessing what is the correct output and you are guessing wrong.
There is nothing wrong with piping output to a file and the viewing that
file through 'less -R'.

If a user does not want the color codes in the file, then that user can
drop the -color arg. iproute2 commands should not be guessing.

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

[Phil Sutter]

On Wed, Aug 15, 2018 at 10:43:25AM -0600, David Ahern wrote:
> On 8/15/18 10:39 AM, Phil Sutter wrote:
> > On Wed, Aug 15, 2018 at 10:24:31AM -0600, David Ahern wrote:
> >> On 8/15/18 10:21 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. Allow to override this
> >>> check by specifying '-color' flag more than once.
> >>>
> >>> Signed-off-by: Phil Sutter <phil@nwl.cc>
> >>> ---
> >>> Changes since v1:
> >>> - Allow to override isatty() check by specifying '-color' flag more than
> >>>   once.
> >>
> >> That adds overhead to my workflow where I almost always have to pipe the
> >> output of ip to a pager.
> > 
> > alias ip='ip -color -color'
> 
> no. Don't impact existing users.

That's a possible fix for *your* workflow. If applied to the shell
handling that workflow, it won't impact existing users.

> > Another alternative may be to introduce -autocolor flag. Establishing
> > the same syntax as used by 'ls' is not as trivial due to the simple
> > commandline parsing used in 'ip'.
> 
> I disagree with ignoring or overriding an argument a user passes in. You
> are guessing what is the correct output and you are guessing wrong.
> There is nothing wrong with piping output to a file and the viewing that
> file through 'less -R'.
> 
> If a user does not want the color codes in the file, then that user can
> drop the -color arg. iproute2 commands should not be guessing.

OK, I got it. Should I respin the fixes or will you apply the series
partially?

Thanks, Phil

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

[David Ahern]

On 8/15/18 10:51 AM, Phil Sutter wrote:
> Should I respin the fixes or will you apply the series
> partially?

Stephen has released 4.18 but not merged -next to master yet, so I
applied the first 3 to -next.

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

[Phil Sutter]

On Wed, Aug 15, 2018 at 10:57:13AM -0600, David Ahern wrote:
> On 8/15/18 10:51 AM, Phil Sutter wrote:
> > Should I respin the fixes or will you apply the series
> > partially?
> 
> Stephen has released 4.18 but not merged -next to master yet, so I
> applied the first 3 to -next.

OK, thanks!

Cheers, Phil

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

[Stephen Hemminger]

On Wed, 15 Aug 2018 18:51:15 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Wed, Aug 15, 2018 at 10:43:25AM -0600, David Ahern wrote:
> > On 8/15/18 10:39 AM, Phil Sutter wrote:  
> > > On Wed, Aug 15, 2018 at 10:24:31AM -0600, David Ahern wrote:  
> > >> On 8/15/18 10:21 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. Allow to override this
> > >>> check by specifying '-color' flag more than once.
> > >>>
> > >>> Signed-off-by: Phil Sutter <phil@nwl.cc>
> > >>> ---
> > >>> Changes since v1:
> > >>> - Allow to override isatty() check by specifying '-color' flag more than
> > >>>   once.  
> > >>
> > >> That adds overhead to my workflow where I almost always have to pipe the
> > >> output of ip to a pager.  
> > > 
> > > alias ip='ip -color -color'  
> > 
> > no. Don't impact existing users.  
> 
> That's a possible fix for *your* workflow. If applied to the shell
> handling that workflow, it won't impact existing users.
> 
> > > Another alternative may be to introduce -autocolor flag. Establishing
> > > the same syntax as used by 'ls' is not as trivial due to the simple
> > > commandline parsing used in 'ip'.  
> > 
> > I disagree with ignoring or overriding an argument a user passes in. You
> > are guessing what is the correct output and you are guessing wrong.
> > There is nothing wrong with piping output to a file and the viewing that
> > file through 'less -R'.
> > 
> > If a user does not want the color codes in the file, then that user can
> > drop the -color arg. iproute2 commands should not be guessing.  
> 
> OK, I got it. Should I respin the fixes or will you apply the series
> partially?
> 
> Thanks, Phil

Please follow the color conventions of grep and ls to have
consistent user experience.
	-c  == --color=always
	
and add never and auto.

[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