* [iproute PATCH v2 0/4] A bunch of fixes regarding colored output
@ 2018-08-15 16:21 Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 1/4] tc: Fix typo in check for " Phil Sutter
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Till Maas, David Ahern
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [iproute PATCH 1/4] tc: Fix typo in check for colored output
2018-08-15 16:21 [iproute PATCH v2 0/4] A bunch of fixes regarding colored output Phil Sutter
@ 2018-08-15 16:21 ` Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 2/4] bridge: Fix " Phil Sutter
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Till Maas, David Ahern
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [iproute PATCH 2/4] bridge: Fix check for colored output
2018-08-15 16:21 [iproute PATCH v2 0/4] A bunch of fixes regarding colored output Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 1/4] tc: Fix typo in check for " Phil Sutter
@ 2018-08-15 16:21 ` Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 3/4] Merge common code for conditionally " Phil Sutter
2018-08-15 16:21 ` [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs Phil Sutter
3 siblings, 0 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Till Maas, David Ahern
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [iproute PATCH 3/4] Merge common code for conditionally colored output
2018-08-15 16:21 [iproute PATCH v2 0/4] A bunch of fixes regarding colored output Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 1/4] tc: Fix typo in check for " Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 2/4] bridge: Fix " Phil Sutter
@ 2018-08-15 16:21 ` Phil Sutter
2018-08-15 16:21 ` [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs Phil Sutter
3 siblings, 0 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Till Maas, David Ahern
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:21 [iproute PATCH v2 0/4] A bunch of fixes regarding colored output Phil Sutter
` (2 preceding siblings ...)
2018-08-15 16:21 ` [iproute PATCH 3/4] Merge common code for conditionally " Phil Sutter
@ 2018-08-15 16:21 ` Phil Sutter
2018-08-15 16:24 ` David Ahern
3 siblings, 1 reply; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Till Maas, David Ahern
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:21 ` [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs Phil Sutter
@ 2018-08-15 16:24 ` David Ahern
2018-08-15 16:39 ` Phil Sutter
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2018-08-15 16:24 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger; +Cc: netdev, Till Maas
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:24 ` David Ahern
@ 2018-08-15 16:39 ` Phil Sutter
2018-08-15 16:43 ` David Ahern
0 siblings, 1 reply; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:39 UTC (permalink / raw)
To: David Ahern; +Cc: Stephen Hemminger, netdev, Till Maas
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:39 ` Phil Sutter
@ 2018-08-15 16:43 ` David Ahern
2018-08-15 16:51 ` Phil Sutter
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2018-08-15 16:43 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger, netdev, Till Maas
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:43 ` David Ahern
@ 2018-08-15 16:51 ` Phil Sutter
2018-08-15 16:57 ` David Ahern
2018-08-15 19:30 ` Stephen Hemminger
0 siblings, 2 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 16:51 UTC (permalink / raw)
To: David Ahern; +Cc: Stephen Hemminger, netdev, Till Maas
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:51 ` Phil Sutter
@ 2018-08-15 16:57 ` David Ahern
2018-08-15 17:58 ` Phil Sutter
2018-08-15 19:30 ` Stephen Hemminger
1 sibling, 1 reply; 12+ messages in thread
From: David Ahern @ 2018-08-15 16:57 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger, netdev, Till Maas
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:57 ` David Ahern
@ 2018-08-15 17:58 ` Phil Sutter
0 siblings, 0 replies; 12+ messages in thread
From: Phil Sutter @ 2018-08-15 17:58 UTC (permalink / raw)
To: David Ahern; +Cc: Stephen Hemminger, netdev, Till Maas
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs
2018-08-15 16:51 ` Phil Sutter
2018-08-15 16:57 ` David Ahern
@ 2018-08-15 19:30 ` Stephen Hemminger
1 sibling, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2018-08-15 19:30 UTC (permalink / raw)
To: Phil Sutter; +Cc: David Ahern, netdev, Till Maas
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-08-15 22:24 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-15 16:21 [iproute PATCH v2 0/4] A bunch of fixes regarding colored output Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 1/4] tc: Fix typo in check for " Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 2/4] bridge: Fix " Phil Sutter
2018-08-15 16:21 ` [iproute PATCH 3/4] Merge common code for conditionally " Phil Sutter
2018-08-15 16:21 ` [iproute PATCH v2 4/4] lib: Enable colored output only for TTYs Phil Sutter
2018-08-15 16:24 ` David Ahern
2018-08-15 16:39 ` Phil Sutter
2018-08-15 16:43 ` David Ahern
2018-08-15 16:51 ` Phil Sutter
2018-08-15 16:57 ` David Ahern
2018-08-15 17:58 ` Phil Sutter
2018-08-15 19:30 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.