[PATCH iproute2-next v2 0/3] bridge: fixes regarding the colorized output

[PATCH iproute2-next v2 0/3] bridge: fixes regarding the colorized output

[Gokul Sivakumar]

v2:
 - Replace the 2 newly introduced fprintf() func calls with print_string()
   to address Stephen's suggestion.

For the bridge cmd, the "colorize output" cmd line option's usage and the
corresponding output does not seem to be consistent with the usage and the
output of the "ip" cmds.

Example 1: As per the "colorize output" cmd line option documented in the
man pages man/man8/ip.8 & man/man8/bridge.8, using "-c" with "ip" or the
"bridge" should colorize the output. But this is working only with the "ip"
cmd and for the "bridge" cmd, the option "-col" / "-colo" / "-color" needs
to be used instead of just "-c".

Example 2: After fixing the inconsistency mentioned in Example 1,
- Running the cmd "$ ip -c neigh", will give the following output where the
  "172.16.12.250", "bridge0" & "04:d9:f5:c1:0c:74" fields are highlighted
  and not the fixed keywords "dev" & "lladdr".

  172.16.12.250 dev bridge0 lladdr 04:d9:f5:c1:0c:74 REACHABLE

- But running the cmd "$ bridge -c fdb", will give the following output
  where "00:00:00:00:00:00", "dev", "vxlan100", "dst" & "2001:db8:2::1" are
  highlighted, even though "dev" & "dst" keywords are static.

  00:00:00:00:00:00 dev vxlan100 dst 2001:db8:2::1 self permanent

Also fix a typo in man/man8/bridge.8 to change "-c[lor]" into "-c[olor]".

Gokul Sivakumar (3):
  bridge: reorder cmd line arg parsing to let "-c" detected as "color"
    option
  bridge: fdb: don't colorize the "dev" & "dst" keywords in "bridge -c
    fdb"
  man: bridge: fix the typo to change "-c[lor]" into "-c[olor]" in man
    page

 bridge/bridge.c   |  2 +-
 bridge/fdb.c      | 13 ++++++++++---
 man/man8/bridge.8 |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH iproute2-next v2 1/3] bridge: reorder cmd line arg parsing to let "-c" detected as "color" option

[Gokul Sivakumar]

As per the man/man8/bridge.8 page, the shorthand cmd line arg "-c" can be
used to colorize the bridge cmd output. But while parsing the args in while
loop, matches() detects "-c" as "-compressedvlans" instead of "-color", so
fix this by doing the check for "-color" option first before checking for
"-compressedvlans".

Signed-off-by: Gokul Sivakumar <gokulkumar792@gmail.com>
---
 bridge/bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index f7bfe0b5..48b0e7f8 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -149,9 +149,9 @@ main(int argc, char **argv)
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
 				exit(-1);
+		} else if (matches_color(opt, &color)) {
 		} else if (matches(opt, "-compressvlans") == 0) {
 			++compress_vlans;
-		} else if (matches_color(opt, &color)) {
 		} else if (matches(opt, "-force") == 0) {
 			++force;
 		} else if (matches(opt, "-json") == 0) {
-- 
2.25.1

[PATCH iproute2-next v2 2/3] bridge: fdb: don't colorize the "dev" & "dst" keywords in "bridge -c fdb"

[Gokul Sivakumar]

To be consistent with the colorized output of "ip" command and to increase
readability, stop highlighting the "dev" & "dst" keywords in the colorized
output of "bridge -c fdb" cmd.

Example: in the following "bridge -c fdb" entry, only "00:00:00:00:00:00",
"vxlan100" and "2001:db8:2::1" fields should be highlighted in color.

00:00:00:00:00:00 dev vxlan100 dst 2001:db8:2::1 self permanent

Signed-off-by: Gokul Sivakumar <gokulkumar792@gmail.com>
---
 bridge/fdb.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/bridge/fdb.c b/bridge/fdb.c
index 37465e46..ea61f65a 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -192,10 +192,14 @@ int print_fdb(struct nlmsghdr *n, void *arg)
 				   "mac", "%s ", lladdr);
 	}
 
-	if (!filter_index && r->ndm_ifindex)
+	if (!filter_index && r->ndm_ifindex) {
+		if (!is_json_context())
+			print_string(PRINT_FP, NULL, "dev ", NULL);
+
 		print_color_string(PRINT_ANY, COLOR_IFNAME,
-				   "ifname", "dev %s ",
+				   "ifname", "%s ",
 				   ll_index_to_name(r->ndm_ifindex));
+	}
 
 	if (tb[NDA_DST]) {
 		int family = AF_INET;
@@ -208,9 +212,12 @@ int print_fdb(struct nlmsghdr *n, void *arg)
 				  RTA_PAYLOAD(tb[NDA_DST]),
 				  RTA_DATA(tb[NDA_DST]));
 
+		if (!is_json_context())
+			print_string(PRINT_FP, NULL, "dst ", NULL);
+
 		print_color_string(PRINT_ANY,
 				   ifa_family_color(family),
-				    "dst", "dst %s ", dst);
+				   "dst", "%s ", dst);
 	}
 
 	if (vid)
-- 
2.25.1

[PATCH iproute2-next v2 3/3] man: bridge: fix the typo to change "-c[lor]" into "-c[olor]" in man page

[Gokul Sivakumar]

Fixes: 3a1ca9a5b ("bridge: update man page for new color and json changes")
Signed-off-by: Gokul Sivakumar <gokulkumar792@gmail.com>
---
 man/man8/bridge.8 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index eec7df43..db83a2a6 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -22,7 +22,7 @@ bridge \- show / manipulate bridge addresses and devices
 \fB\-s\fR[\fItatistics\fR] |
 \fB\-n\fR[\fIetns\fR] name |
 \fB\-b\fR[\fIatch\fR] filename |
-\fB\-c\fR[\folor\fR] |
+\fB\-c\fR[\fIolor\fR] |
 \fB\-p\fR[\fIretty\fR] |
 \fB\-j\fR[\fIson\fR] |
 \fB\-o\fR[\fIneline\fr] }
-- 
2.25.1

Re: [PATCH iproute2-next v2 1/3] bridge: reorder cmd line arg parsing to let "-c" detected as "color" option

[David Ahern]

On 8/14/21 12:47 PM, Gokul Sivakumar wrote:
> As per the man/man8/bridge.8 page, the shorthand cmd line arg "-c" can be
> used to colorize the bridge cmd output. But while parsing the args in while
> loop, matches() detects "-c" as "-compressedvlans" instead of "-color", so
> fix this by doing the check for "-color" option first before checking for
> "-compressedvlans".
> 
> Signed-off-by: Gokul Sivakumar <gokulkumar792@gmail.com>
> ---
>  bridge/bridge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/bridge/bridge.c b/bridge/bridge.c
> index f7bfe0b5..48b0e7f8 100644
> --- a/bridge/bridge.c
> +++ b/bridge/bridge.c
> @@ -149,9 +149,9 @@ main(int argc, char **argv)
>  			NEXT_ARG();
>  			if (netns_switch(argv[1]))
>  				exit(-1);
> +		} else if (matches_color(opt, &color)) {
>  		} else if (matches(opt, "-compressvlans") == 0) {
>  			++compress_vlans;
> -		} else if (matches_color(opt, &color)) {
>  		} else if (matches(opt, "-force") == 0) {
>  			++force;
>  		} else if (matches(opt, "-json") == 0) {
> 

Another example of why matches needs to be deprecated.

Re-assigned the set to Stephen for main tree.

Re: [PATCH iproute2-next v2 2/3] bridge: fdb: don't colorize the "dev" & "dst" keywords in "bridge -c fdb"

[Stephen Hemminger]

On Sun, 15 Aug 2021 00:17:26 +0530
Gokul Sivakumar <gokulkumar792@gmail.com> wrote:

> +		if (!is_json_context())
> +			print_string(PRINT_FP, NULL, "dev ", NULL);

Why not the check for is_json_context is unnecessary here.
That is what PRINT_FP does.

Re: [PATCH iproute2-next v2 1/3] bridge: reorder cmd line arg parsing to let "-c" detected as "color" option

[Gokul Sivakumar]

On Tue, 2021-08-17 at 09:11 -0600, David Ahern wrote:
> On 8/14/21 12:47 PM, Gokul Sivakumar wrote:
> > As per the man/man8/bridge.8 page, the shorthand cmd line arg "-c" can be
> > used to colorize the bridge cmd output. But while parsing the args in while
> > loop, matches() detects "-c" as "-compressedvlans" instead of "-color", so
> > fix this by doing the check for "-color" option first before checking for
> > "-compressedvlans".
> > 
> > Signed-off-by: Gokul Sivakumar <gokulkumar792@gmail.com>
> > ---
> >  bridge/bridge.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/bridge/bridge.c b/bridge/bridge.c
> > index f7bfe0b5..48b0e7f8 100644
> > --- a/bridge/bridge.c
> > +++ b/bridge/bridge.c
> > @@ -149,9 +149,9 @@ main(int argc, char **argv)
> >  			NEXT_ARG();
> >  			if (netns_switch(argv[1]))
> >  				exit(-1);
> > +		} else if (matches_color(opt, &color)) {
> >  		} else if (matches(opt, "-compressvlans") == 0) {
> >  			++compress_vlans;
> > -		} else if (matches_color(opt, &color)) {
> >  		} else if (matches(opt, "-force") == 0) {
> >  			++force;
> >  		} else if (matches(opt, "-json") == 0) {
> > 
> 
> Another example of why matches needs to be deprecated.
> 
> Re-assigned the set to Stephen for main tree.

Thanks, i will send a v3 patchset with the subject prefix "PATCH iproute2"
instead of "PATCH iproute2-next" after addressing Stephen's recent suggestion 
to remove the unnecessary is_json_context() condition checks from patch 2/3.

Gokul

Re: [PATCH iproute2-next v2 2/3] bridge: fdb: don't colorize the "dev" & "dst" keywords in "bridge -c fdb"

[Gokul Sivakumar]

On Tue, 2021-08-17 at 08:27 -0700, Stephen Hemminger wrote:
> On Sun, 15 Aug 2021 00:17:26 +0530
> Gokul Sivakumar <gokulkumar792@gmail.com> wrote:
> 
> > +		if (!is_json_context())
> > +			print_string(PRINT_FP, NULL, "dev ", NULL);
> 
> Why not the check for is_json_context is unnecessary here.
> That is what PRINT_FP does.

Thanks for pointing it out, will remove those two unnecessary
condition checks and send a v3 patchset now.

Gokul