| Message ID | 20230918152910.5325-1-stephen@networkplumber.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v3] allow overriding color option in environment | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Sep 18, 2023 at 08:29:10AM -0700, Stephen Hemminger wrote: > For ip, tc, and bridge command introduce IPROUTE_COLORS to enable > automatic colorization via environment variable. > Similar to how grep handles color flag. > > Example: > $ IPROUTE_COLORS=auto ip -br addr > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > v3 - drop unneccessary check for NULL in match_colors > all three callers pass valid pointer. > drop unnecessary check for NULL in default_color The NULL check in default_color is necessary, because getenv may return NULL if there is no env variable with the desired name. Indeed it seems to me this check is maintained in this patch. However the null string check in default_color is also necessary because, as I pointed out in the review of the RFC version of this patch: IPROUTE_COLORS= ip address results in colorized output, while I would expect it to produce colorless output. This happens because we are effectively passing a null string to default_color using the above syntax, and match_color_value() treat the null string as 'always'. Please note that this is indeed correct when calling match_color_value when the '-c / --color' option is provided, but not when the env variable is used to determine the color.
On 9/18/23 2:20 PM, Andrea Claudi wrote: > On Mon, Sep 18, 2023 at 08:29:10AM -0700, Stephen Hemminger wrote: >> For ip, tc, and bridge command introduce IPROUTE_COLORS to enable >> automatic colorization via environment variable. >> Similar to how grep handles color flag. >> >> Example: >> $ IPROUTE_COLORS=auto ip -br addr >> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >> --- >> v3 - drop unneccessary check for NULL in match_colors >> all three callers pass valid pointer. >> drop unnecessary check for NULL in default_color > > The NULL check in default_color is necessary, because getenv may return > NULL if there is no env variable with the desired name. Indeed it seems > to me this check is maintained in this patch. > > However the null string check in default_color is also necessary > because, as I pointed out in the review of the RFC version of this > patch: > > IPROUTE_COLORS= ip address > > results in colorized output, while I would expect it to produce > colorless output. > > This happens because we are effectively passing a null string to > default_color using the above syntax, and match_color_value() treat the > null string as 'always'. > > Please note that this is indeed correct when calling match_color_value > when the '-c / --color' option is provided, but not when the env > variable is used to determine the color. > FYI: I did not get this patch, and it is not showing in patchworks either.
diff --git a/bridge/bridge.c b/bridge/bridge.c index 339101a874b1..d506f75ebc46 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(); while (argc > 1) { const char *opt = argv[1]; diff --git a/include/color.h b/include/color.h index 17ec56f3d7b4..1ddd1bda5797 100644 --- a/include/color.h +++ b/include/color.h @@ -20,6 +20,7 @@ enum color_opt { COLOR_OPT_ALWAYS = 2 }; +int default_color(void); 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..0befe14e3d66 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(); /* 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..7f58e107773b 100644 --- a/lib/color.c +++ b/lib/color.c @@ -93,13 +93,36 @@ 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")) + *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(void) +{ + const char *name; + int val; + + name = getenv("IPROUTE_COLORS"); + if (name && match_color_value(name, &val)) + return val; + + /* default is from config */ + return CONF_COLOR; +} + bool matches_color(const char *arg, int *val) { char *dup, *p; - if (!val) - return false; - dup = strdupa(arg); p = strchrnul(dup, '='); if (*p) @@ -108,15 +131,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..6ad34e4d704a 100644 --- a/man/man8/bridge.8 +++ b/man/man8/bridge.8 @@ -315,10 +315,14 @@ color output is enabled regardless of stdout state. If parameter is 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 +precedence. +The default color setting is +.B never +but can be overridden by the +.B IPROUTE_COLORS +environment variable. This flag is ignored if .B \-json is also given. - .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..63858edc318d 100644 --- a/man/man8/ip.8 +++ b/man/man8/ip.8 @@ -193,15 +193,17 @@ 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 +precedence.The default color setting is +.B never +but can be overridden by the +.B IPROUTE_COLORS +environment variable. 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 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..e47817704e4c 100644 --- a/man/man8/tc.8 +++ b/man/man8/tc.8 @@ -801,10 +801,14 @@ color output is enabled regardless of stdout state. If parameter is 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 +precedence. +The default color setting is +.B never +but can be overridden by the +.B IPROUTE_COLORS +environment variable. This flag is ignored if .B \-json is also given. - .TP .BR "\-j", " \-json" Display results in JSON format. diff --git a/tc/tc.c b/tc/tc.c index 082c6677d34a..e8b214802d1f 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(); int ret; while (argc > 1) {
For ip, tc, and bridge command introduce IPROUTE_COLORS to enable automatic colorization via environment variable. Similar to how grep handles color flag. Example: $ IPROUTE_COLORS=auto ip -br addr Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- v3 - drop unneccessary check for NULL in match_colors all three callers pass valid pointer. drop unnecessary check for NULL in default_color bridge/bridge.c | 2 +- include/color.h | 1 + ip/ip.c | 2 +- lib/color.c | 39 +++++++++++++++++++++++++++------------ man/man8/bridge.8 | 8 ++++++-- man/man8/ip.8 | 14 ++++++++------ man/man8/tc.8 | 8 ++++++-- tc/tc.c | 2 +- 8 files changed, 51 insertions(+), 25 deletions(-)