Re: [iproute PATCH v5 1/2] Make colored output configurable

2018-08-17 Thread David Ahern
On 8/17/18 10:38 AM, Phil Sutter 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 
> ---
> 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.
> 
> Changes since v4:
> - Make matches_color() return boolean.
> ---
>  bridge/bridge.c   |  3 +--
>  include/color.h   |  9 +
>  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, 77 insertions(+), 13 deletions(-)
> 

LGTM.

Reviewed-by: David Ahern 



[iproute PATCH v5 1/2] Make colored output configurable

2018-08-17 Thread Phil Sutter
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 
---
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.

Changes since v4:
- Make matches_color() return boolean.
---
 bridge/bridge.c   |  3 +--
 include/color.h   |  9 +
 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, 77 insertions(+), 13 deletions(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index b3cab717ead30..663a35b2b2e46 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, )) {
} 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..a22a00c2277e0 100644
--- a/include/color.h
+++ b/include/color.h
@@ -2,6 +2,8 @@
 #ifndef __COLOR_H__
 #define __COLOR_H__ 1
 
+#include 
+
 enum color_attr {
COLOR_IFNAME,
COLOR_MAC,
@@ -12,8 +14,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);
+bool 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 72e858eed50d5..58c643df8a366 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, )) {
} 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..9c9023587748f 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -3,11 +3,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 
 #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;
 }
 
+bool matches_color(const char *arg, int *val)
+{
+   char *dup, *p;
+
+   if (!val)
+   return false;
+
+   dup = strdupa(arg);
+   p = strchrnul(dup, '=');
+   if (*p)
+   *(p++) = '\0';
+
+   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;
+}
+
 void set_color_palette(void)
 {
char *p = getenv("COLORFGBG");
diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index 1d10cb2b6a72c..53cd3d0a3d933 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -172,8 +172,17 @@ If there were any errors during execution of the commands, 
the application
 return code will be non zero.
 
 .TP
-.BR "\-c" , " -color"
-Use color output.
+.BR \-c [ color ][ = { always | auto | never }
+Configure color output. If parameter is omitted or
+.BR always ,
+color output is enabled regardless of stdout state. If parameter is
+.BR auto ,
+stdout is checked to be a terminal before enabling color output. If parameter 
is
+.BR never ,
+color output is disabled. If specified multiple times, the last one takes
+precedence. 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..1d358879ec39c 100644
--- a/man/man8/ip.8
+++