[PATCH iproute2-next 00/20] bridge: vni: UI fixes

[PATCH iproute2-next 00/20] bridge: vni: UI fixes

[Benjamin Poirier]

This series mainly contains fixes to `bridge vni` command input and output.
There are also a few adjacent changes to `bridge vlan` and `bridge vni`.

Benjamin Poirier (20):
  bridge: vni: Accept 'del' command
  bridge: vni: Remove dead code in group argument parsing
  bridge: vni: Fix duplicate group and remote error messages
  bridge: vni: Report duplicate vni argument using duparg()
  bridge: vni: Fix vni filter help strings
  bridge: vlan: Use printf() to avoid temporary buffer
  bridge: vlan: Remove paranoid check
  bridge: vni: Remove print_vnifilter_rtm_filter()
  bridge: vni: Move open_json_object() within print_vni()
  bridge: vni: Guard close_vni_port() call
  bridge: vni: Reverse the logic in print_vnifilter_rtm()
  bridge: vni: Remove stray newlines after each interface
  bridge: vni: Replace open-coded instance of print_nl()
  bridge: vni: Remove unused argument in open_vni_port()
  bridge: vni: Align output columns
  bridge: vni: Indent statistics with 2 spaces
  bridge: Deduplicate print_range()
  json_print: Output to temporary buffer in print_range() only as needed
  json_print: Rename print_range() argument
  bridge: Provide rta_type()

 bridge/bridge.c      |   2 +-
 bridge/vlan.c        |  38 +++------------
 bridge/vni.c         | 113 +++++++++++++++++--------------------------
 include/json_print.h |   2 +
 include/libnetlink.h |   4 ++
 lib/json_print.c     |  15 ++++++
 6 files changed, 75 insertions(+), 99 deletions(-)

-- 
2.43.0

[PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Benjamin Poirier]

`bridge vni help` shows "bridge vni { add | del } ..." but currently
`bridge vni del ...` errors out unexpectedly:
	# bridge vni del
	Command "del" is unknown, try "bridge vni help".

Recognize 'del' as a synonym of the original 'delete' command.

Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index e804cb3f..6c0e35cd 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -419,7 +419,8 @@ int do_vni(int argc, char **argv)
 	if (argc > 0) {
 		if (strcmp(*argv, "add") == 0)
 			return vni_modify(RTM_NEWTUNNEL, argc-1, argv+1);
-		if (strcmp(*argv, "delete") == 0)
+		if (strcmp(*argv, "delete") == 0 ||
+		    strcmp(*argv, "del") == 0)
 			return vni_modify(RTM_DELTUNNEL, argc-1, argv+1);
 		if (strcmp(*argv, "show") == 0 ||
 		    strcmp(*argv, "lst") == 0 ||
-- 
2.43.0

Re: [PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:13 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> `bridge vni help` shows "bridge vni { add | del } ..." but currently
> `bridge vni del ...` errors out unexpectedly:
> 	# bridge vni del
> 	Command "del" is unknown, try "bridge vni help".
> 
> Recognize 'del' as a synonym of the original 'delete' command.
> 
> Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Please no.
We are blocking uses of matches() and now other commands will want more synonyms
Instead fix the help and doc.

Re: [PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Stephen Hemminger]

On Wed, 20 Dec 2023 19:57:08 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Mon, 11 Dec 2023 09:07:13 -0500
> Benjamin Poirier <bpoirier@nvidia.com> wrote:
> 
> > `bridge vni help` shows "bridge vni { add | del } ..." but currently
> > `bridge vni del ...` errors out unexpectedly:
> > 	# bridge vni del
> > 	Command "del" is unknown, try "bridge vni help".
> > 
> > Recognize 'del' as a synonym of the original 'delete' command.
> > 
> > Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> > Reviewed-by: Petr Machata <petrm@nvidia.com>
> > Tested-by: Petr Machata <petrm@nvidia.com>
> > Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>  
> 
> Please no.
> We are blocking uses of matches() and now other commands will want more synonyms
> Instead fix the help and doc.

I changed my mind. This is fine. The commands in iproute2 are inconsistent (no surprise)
and plenty of places take del (and not delete??)

Re: [PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Benjamin Poirier]

On 2023-12-21 08:06 -0800, Stephen Hemminger wrote:
> On Wed, 20 Dec 2023 19:57:08 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Mon, 11 Dec 2023 09:07:13 -0500
> > Benjamin Poirier <bpoirier@nvidia.com> wrote:
> > 
> > > `bridge vni help` shows "bridge vni { add | del } ..." but currently
> > > `bridge vni del ...` errors out unexpectedly:
> > > 	# bridge vni del
> > > 	Command "del" is unknown, try "bridge vni help".
> > > 
> > > Recognize 'del' as a synonym of the original 'delete' command.
> > > 
> > > Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> > > Reviewed-by: Petr Machata <petrm@nvidia.com>
> > > Tested-by: Petr Machata <petrm@nvidia.com>
> > > Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>  
> > 
> > Please no.
> > We are blocking uses of matches() and now other commands will want more synonyms
> > Instead fix the help and doc.
> 
> I changed my mind. This is fine. The commands in iproute2 are inconsistent (no surprise)
> and plenty of places take del (and not delete??)

Indeed. Thank you for the update. In that case, can you take the series
as-is or should I still reduce the overall patch count and resubmit?

Re: [PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Stephen Hemminger]

On Thu, 21 Dec 2023 11:54:49 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> On 2023-12-21 08:06 -0800, Stephen Hemminger wrote:
> > On Wed, 20 Dec 2023 19:57:08 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >   
> > > On Mon, 11 Dec 2023 09:07:13 -0500
> > > Benjamin Poirier <bpoirier@nvidia.com> wrote:
> > >   
> > > > `bridge vni help` shows "bridge vni { add | del } ..." but currently
> > > > `bridge vni del ...` errors out unexpectedly:
> > > > 	# bridge vni del
> > > > 	Command "del" is unknown, try "bridge vni help".
> > > > 
> > > > Recognize 'del' as a synonym of the original 'delete' command.
> > > > 
> > > > Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> > > > Reviewed-by: Petr Machata <petrm@nvidia.com>
> > > > Tested-by: Petr Machata <petrm@nvidia.com>
> > > > Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>    
> > > 
> > > Please no.
> > > We are blocking uses of matches() and now other commands will want more synonyms
> > > Instead fix the help and doc.  
> > 
> > I changed my mind. This is fine. The commands in iproute2 are inconsistent (no surprise)
> > and plenty of places take del (and not delete??)  
> 
> Indeed. Thank you for the update. In that case, can you take the series
> as-is or should I still reduce the overall patch count and resubmit?

I will take it as is. Ok for me to squash a few patches together?

Re: [PATCH iproute2-next 01/20] bridge: vni: Accept 'del' command

[Benjamin Poirier]

On 2023-12-21 09:24 -0800, Stephen Hemminger wrote:
[...]
> > > > 
> > > > Please no.
> > > > We are blocking uses of matches() and now other commands will want more synonyms
> > > > Instead fix the help and doc.  
> > > 
> > > I changed my mind. This is fine. The commands in iproute2 are inconsistent (no surprise)
> > > and plenty of places take del (and not delete??)  
> > 
> > Indeed. Thank you for the update. In that case, can you take the series
> > as-is or should I still reduce the overall patch count and resubmit?
> 
> I will take it as is. Ok for me to squash a few patches together?

Yes, that's ok. Thank you for taking care of it.

[PATCH iproute2-next 02/20] bridge: vni: Remove dead code in group argument parsing

[Benjamin Poirier]

is_addrtype_inet_not_multi(&daddr) may read an uninitialized "daddr". Even
if that is fixed, the error message that follows cannot be reached because
the situation would be caught by the previous test (group_present).
Therefore, remove this test on daddr.

Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 6c0e35cd..33e50d18 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -109,11 +109,6 @@ static int vni_modify(int cmd, int argc, char **argv)
 		} else if (strcmp(*argv, "group") == 0) {
 			if (group_present)
 				invarg("duplicate group", *argv);
-			if (is_addrtype_inet_not_multi(&daddr)) {
-				fprintf(stderr, "vxlan: both group and remote");
-				fprintf(stderr, " cannot be specified\n");
-				return -1;
-			}
 			NEXT_ARG();
 			get_addr(&daddr, *argv, AF_UNSPEC);
 			if (!is_addrtype_inet_multi(&daddr))
-- 
2.43.0

Re: [PATCH iproute2-next 02/20] bridge: vni: Remove dead code in group argument parsing

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:14 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> is_addrtype_inet_not_multi(&daddr) may read an uninitialized "daddr". Even
> if that is fixed, the error message that follows cannot be reached because
> the situation would be caught by the previous test (group_present).
> Therefore, remove this test on daddr.
> 
> Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---
>  bridge/vni.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/bridge/vni.c b/bridge/vni.c
> index 6c0e35cd..33e50d18 100644
> --- a/bridge/vni.c
> +++ b/bridge/vni.c
> @@ -109,11 +109,6 @@ static int vni_modify(int cmd, int argc, char **argv)
>  		} else if (strcmp(*argv, "group") == 0) {
>  			if (group_present)
>  				invarg("duplicate group", *argv);
> -			if (is_addrtype_inet_not_multi(&daddr)) {
> -				fprintf(stderr, "vxlan: both group and remote");
> -				fprintf(stderr, " cannot be specified\n");
> -				return -1;
> -			}
>  			NEXT_ARG();
>  			get_addr(&daddr, *argv, AF_UNSPEC);
>  			if (!is_addrtype_inet_multi(&daddr))


This makes sense, and why was the message split in the first place.

Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 03/20] bridge: vni: Fix duplicate group and remote error messages

[Benjamin Poirier]

Consider the following command with a duplicated "remote" argument:
$ bridge vni add vni 150 remote 10.0.0.1 remote 10.0.0.2 dev vxlan2
Error: argument "remote" is wrong: duplicate group

The error message is misleading because there is no "group" argument. Both
of the "group" and "remote" options specify a destination address and are
mutually exclusive so change the variable name and error messages
accordingly.

The result is:
$ ./bridge/bridge vni add vni 150 remote 10.0.0.1 remote 10.0.0.2 dev vxlan2
Error: duplicate "destination": "10.0.0.2" is the second value.

Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 33e50d18..56def2f7 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -92,7 +92,7 @@ static int vni_modify(int cmd, int argc, char **argv)
 		.n.nlmsg_type = cmd,
 		.tmsg.family = PF_BRIDGE,
 	};
-	bool group_present = false;
+	bool daddr_present = false;
 	inet_prefix daddr;
 	char *vni = NULL;
 	char *d = NULL;
@@ -107,19 +107,19 @@ static int vni_modify(int cmd, int argc, char **argv)
 				invarg("duplicate vni", *argv);
 			vni = *argv;
 		} else if (strcmp(*argv, "group") == 0) {
-			if (group_present)
-				invarg("duplicate group", *argv);
 			NEXT_ARG();
+			if (daddr_present)
+				duparg("destination", *argv);
 			get_addr(&daddr, *argv, AF_UNSPEC);
 			if (!is_addrtype_inet_multi(&daddr))
 				invarg("invalid group address", *argv);
-			group_present = true;
+			daddr_present = true;
 		} else if (strcmp(*argv, "remote") == 0) {
-			if (group_present)
-				invarg("duplicate group", *argv);
 			NEXT_ARG();
+			if (daddr_present)
+				duparg("destination", *argv);
 			get_addr(&daddr, *argv, AF_UNSPEC);
-			group_present = true;
+			daddr_present = true;
 		} else {
 			if (strcmp(*argv, "help") == 0)
 				usage();
@@ -133,7 +133,7 @@ static int vni_modify(int cmd, int argc, char **argv)
 	}
 
 	parse_vni_filter(vni, &req.n, sizeof(req),
-			 (group_present ? &daddr : NULL));
+			 (daddr_present ? &daddr : NULL));
 
 	req.tmsg.ifindex = ll_name_to_index(d);
 	if (req.tmsg.ifindex == 0) {
-- 
2.43.0

Re: [PATCH iproute2-next 03/20] bridge: vni: Fix duplicate group and remote error messages

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:15 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Consider the following command with a duplicated "remote" argument:
> $ bridge vni add vni 150 remote 10.0.0.1 remote 10.0.0.2 dev vxlan2
> Error: argument "remote" is wrong: duplicate group
> 
> The error message is misleading because there is no "group" argument. Both
> of the "group" and "remote" options specify a destination address and are
> mutually exclusive so change the variable name and error messages
> accordingly.
> 
> The result is:
> $ ./bridge/bridge vni add vni 150 remote 10.0.0.1 remote 10.0.0.2 dev vxlan2
> Error: duplicate "destination": "10.0.0.2" is the second value.
> 
> Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 04/20] bridge: vni: Report duplicate vni argument using duparg()

[Benjamin Poirier]

When there is a duplicate 'vni' option, report the error using duparg()
instead of the generic invarg().

Before:
$ bridge vni add vni 100 vni 101 dev vxlan2
Error: argument "101" is wrong: duplicate vni

After:
$ ./bridge/bridge vni add vni 100 vni 101 dev vxlan2
Error: duplicate "vni": "101" is the second value.

Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 56def2f7..ecd4c2b5 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -104,7 +104,7 @@ static int vni_modify(int cmd, int argc, char **argv)
 		} else if (strcmp(*argv, "vni") == 0) {
 			NEXT_ARG();
 			if (vni)
-				invarg("duplicate vni", *argv);
+				duparg("vni", *argv);
 			vni = *argv;
 		} else if (strcmp(*argv, "group") == 0) {
 			NEXT_ARG();
-- 
2.43.0

Re: [PATCH iproute2-next 04/20] bridge: vni: Report duplicate vni argument using duparg()

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:16 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> When there is a duplicate 'vni' option, report the error using duparg()
> instead of the generic invarg().
> 
> Before:
> $ bridge vni add vni 100 vni 101 dev vxlan2
> Error: argument "101" is wrong: duplicate vni
> 
> After:
> $ ./bridge/bridge vni add vni 100 vni 101 dev vxlan2
> Error: duplicate "vni": "101" is the second value.
> 
> Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 05/20] bridge: vni: Fix vni filter help strings

[Benjamin Poirier]

Add the missing 'vni' subcommand to the top level `bridge help`.
For `bridge vni { add | del } ...`, 'dev' is a mandatory argument.
For `bridge vni show`, 'dev' is an optional argument.

Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/bridge.c | 2 +-
 bridge/vni.c    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index 339101a8..f4805092 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -36,7 +36,7 @@ static void usage(void)
 	fprintf(stderr,
 "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
 "       bridge [ -force ] -batch filename\n"
-"where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
+"where  OBJECT := { link | fdb | mdb | vlan | vni | monitor }\n"
 "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
 "                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
 "                    -c[ompressvlans] -color -p[retty] -j[son] }\n");
diff --git a/bridge/vni.c b/bridge/vni.c
index ecd4c2b5..74668156 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -33,8 +33,8 @@ static void usage(void)
 	fprintf(stderr,
 		"Usage: bridge vni { add | del } vni VNI\n"
 		"		[ { group | remote } IP_ADDRESS ]\n"
-		"		[ dev DEV ]\n"
-		"       bridge vni { show }\n"
+		"		dev DEV\n"
+		"       bridge vni { show } [ dev DEV ]\n"
 		"\n"
 		"Where:	VNI	:= 0-16777215\n"
 	       );
-- 
2.43.0

Re: [PATCH iproute2-next 05/20] bridge: vni: Fix vni filter help strings

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:17 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Add the missing 'vni' subcommand to the top level `bridge help`.
> For `bridge vni { add | del } ...`, 'dev' is a mandatory argument.
> For `bridge vni show`, 'dev' is an optional argument.
> 
> Fixes: 45cd32f9f7d5 ("bridge: vxlan device vnifilter support")
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 06/20] bridge: vlan: Use printf() to avoid temporary buffer

[Benjamin Poirier]

Currently, print_vlan_tunnel_info() is first outputting a formatted string
to a temporary buffer in order to use print_string() which can handle json
or normal text mode. Since this specific string is only output in normal
text mode, by calling printf() directly, we can avoid the need to first
output to a temporary string buffer.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vlan.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index dfc62f83..797b7802 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -662,11 +662,8 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 		open_json_object(NULL);
 		width = print_range("vlan", last_vid_start, tunnel_vid);
 		if (width <= VLAN_ID_LEN) {
-			char buf[VLAN_ID_LEN + 1];
-
-			snprintf(buf, sizeof(buf), "%-*s",
-				 VLAN_ID_LEN - width, "");
-			print_string(PRINT_FP, NULL, "%s  ", buf);
+			if (!is_json_context())
+				printf("%-*s  ", VLAN_ID_LEN - width, "");
 		} else {
 			fprintf(stderr, "BUG: vlan range too wide, %u\n",
 				width);
-- 
2.43.0

Re: [PATCH iproute2-next 06/20] bridge: vlan: Use printf() to avoid temporary buffer

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:18 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Currently, print_vlan_tunnel_info() is first outputting a formatted string
> to a temporary buffer in order to use print_string() which can handle json
> or normal text mode. Since this specific string is only output in normal
> text mode, by calling printf() directly, we can avoid the need to first
> output to a temporary string buffer.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---
>  bridge/vlan.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/bridge/vlan.c b/bridge/vlan.c
> index dfc62f83..797b7802 100644
> --- a/bridge/vlan.c
> +++ b/bridge/vlan.c
> @@ -662,11 +662,8 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
>  		open_json_object(NULL);
>  		width = print_range("vlan", last_vid_start, tunnel_vid);
>  		if (width <= VLAN_ID_LEN) {
> -			char buf[VLAN_ID_LEN + 1];
> -
> -			snprintf(buf, sizeof(buf), "%-*s",
> -				 VLAN_ID_LEN - width, "");
> -			print_string(PRINT_FP, NULL, "%s  ", buf);
> +			if (!is_json_context())
> +				printf("%-*s  ", VLAN_ID_LEN - width, "");
>  		} else {

I think the fix needs to be deeper here.
In JSON the width doesn't matter.

[PATCH iproute2-next 07/20] bridge: vlan: Remove paranoid check

[Benjamin Poirier]

To make the code lighter, remove the check on the actual print_range()
output width. In the odd case that an out-of-range, wide vlan id is
printed, printf() will treat the negative field width as positive and the
output will simply be further misaligned.

Suggested-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vlan.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 797b7802..7a175b04 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -661,13 +661,8 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 
 		open_json_object(NULL);
 		width = print_range("vlan", last_vid_start, tunnel_vid);
-		if (width <= VLAN_ID_LEN) {
-			if (!is_json_context())
-				printf("%-*s  ", VLAN_ID_LEN - width, "");
-		} else {
-			fprintf(stderr, "BUG: vlan range too wide, %u\n",
-				width);
-		}
+		if (!is_json_context())
+			printf("%-*s  ", VLAN_ID_LEN - width, "");
 		print_range("tunid", last_tunid_start, tunnel_id);
 		close_json_object();
 		print_nl();
-- 
2.43.0

Re: [PATCH iproute2-next 07/20] bridge: vlan: Remove paranoid check

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:19 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> To make the code lighter, remove the check on the actual print_range()
> output width. In the odd case that an out-of-range, wide vlan id is
> printed, printf() will treat the negative field width as positive and the
> output will simply be further misaligned.
> 
> Suggested-by: Petr Machata <petrm@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Why truncate the output anyway?

[PATCH iproute2-next 08/20] bridge: vni: Remove print_vnifilter_rtm_filter()

[Benjamin Poirier]

print_vnifilter_rtm_filter() adds an unnecessary level of indirection so
remove it to simplify the code.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 74668156..51e65b89 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -350,11 +350,6 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 	return 0;
 }
 
-static int print_vnifilter_rtm_filter(struct nlmsghdr *n, void *arg)
-{
-	return print_vnifilter_rtm(n, arg);
-}
-
 static int vni_show(int argc, char **argv)
 {
 	char *filter_dev = NULL;
@@ -395,7 +390,7 @@ static int vni_show(int argc, char **argv)
 		printf("\n");
 	}
 
-	ret = rtnl_dump_filter(&rth, print_vnifilter_rtm_filter, NULL);
+	ret = rtnl_dump_filter(&rth, print_vnifilter_rtm, NULL);
 	if (ret < 0) {
 		fprintf(stderr, "Dump ternminated\n");
 		exit(1);
-- 
2.43.0

Re: [PATCH iproute2-next 08/20] bridge: vni: Remove print_vnifilter_rtm_filter()

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:20 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> print_vnifilter_rtm_filter() adds an unnecessary level of indirection so
> remove it to simplify the code.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 09/20] bridge: vni: Move open_json_object() within print_vni()

[Benjamin Poirier]

print_vni() is used to output one vni or vni range which, in json output
mode, looks like
      {
        "vni": 100
      }

Currently, the closing bracket is handled within the function but the
opening bracket is handled by open_json_object() before calling the
function. For consistency, move the call to open_json_object() within
print_vni().

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 51e65b89..8a6ac245 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -242,6 +242,7 @@ static void print_vni(struct rtattr *t, int ifindex)
 	if (ttb[VXLAN_VNIFILTER_ENTRY_END])
 		vni_end = rta_getattr_u32(ttb[VXLAN_VNIFILTER_ENTRY_END]);
 
+	open_json_object(NULL);
 	if (vni_end)
 		print_range("vni", vni_start, vni_end);
 	else
@@ -333,10 +334,8 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 			continue;
 		if (first) {
 			open_vni_port(tmsg->ifindex, "%s");
-			open_json_object(NULL);
 			first = false;
 		} else {
-			open_json_object(NULL);
 			print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s  ", "");
 		}
 
-- 
2.43.0

Re: [PATCH iproute2-next 09/20] bridge: vni: Move open_json_object() within print_vni()

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:21 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> print_vni() is used to output one vni or vni range which, in json output
> mode, looks like
>       {
>         "vni": 100
>       }
> 
> Currently, the closing bracket is handled within the function but the
> opening bracket is handled by open_json_object() before calling the
> function. For consistency, move the call to open_json_object() within
> print_vni().
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 10/20] bridge: vni: Guard close_vni_port() call

[Benjamin Poirier]

Currently, the call to open_vni_port() within print_vnifilter_rtm() is
written in a way that is safe if there is a RTM_{NEW,DEL,GET}TUNNEL message
without any VXLAN_VNIFILTER_ENTRY attribute. However the close_vni_port()
call is written in a way that assumes there is always at least one
VXLAN_VNIFILTER_ENTRY attribute within every RTM_*TUNNEL message. At this
time, this assumption is correct. However, the code should be consistent in
its assumptions. Choose the safe approach and fix the asymmetry between the
open_vni_port() and close_vni_port() calls by guarding the latter call with
a check.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 8a6ac245..ca5d2e43 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -341,7 +341,9 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 
 		print_vni(t, tmsg->ifindex);
 	}
-	close_vni_port();
+
+	if (!first)
+		close_vni_port();
 
 	print_string(PRINT_FP, NULL, "%s", _SL_);
 
-- 
2.43.0

Re: [PATCH iproute2-next 10/20] bridge: vni: Guard close_vni_port() call

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:22 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

>  	print_string(PRINT_FP, NULL, "%s", _SL_);

In same place.
That could just be print_nl()

[PATCH iproute2-next 11/20] bridge: vni: Reverse the logic in print_vnifilter_rtm()

[Benjamin Poirier]

print_vnifilter_rtm() is structured similarly to print_vlan_tunnel_info()
except that in the former, the open_vni_port() call is guarded by a "if
(first)" check whereas in the latter, the open_vlan_port() call is guarded
by a "if (!opened)" check.

Reverse the logic in one of the functions to have the same structure in
both. Since the calls being guarded are "open_...()", "close_...()", use
the "opened" logic structure.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index ca5d2e43..b597a916 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -296,7 +296,7 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 {
 	struct tunnel_msg *tmsg = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
-	bool first = true;
+	bool opened = false;
 	struct rtattr *t;
 	FILE *fp = arg;
 	int rem;
@@ -332,9 +332,10 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 
 		if (rta_type != VXLAN_VNIFILTER_ENTRY)
 			continue;
-		if (first) {
+
+		if (!opened) {
 			open_vni_port(tmsg->ifindex, "%s");
-			first = false;
+			opened = true;
 		} else {
 			print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s  ", "");
 		}
@@ -342,7 +343,7 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 		print_vni(t, tmsg->ifindex);
 	}
 
-	if (!first)
+	if (opened)
 		close_vni_port();
 
 	print_string(PRINT_FP, NULL, "%s", _SL_);
-- 
2.43.0

[PATCH iproute2-next 12/20] bridge: vni: Remove stray newlines after each interface

[Benjamin Poirier]

Currently, `bridge vni` outputs an empty line after each interface. This is
not consistent with the output style of other iproute2 commands, in
particular `bridge vlan`. Therefore, remove the empty lines.

If there are scripts that parse the normal text output of `bridge vni`,
those scripts might be broken by the removal of the empty lines. This is a
secondary concern because those scripts should consume the JSON output
instead.

Before:
$ bridge vni
dev               vni              group/remote
vxlan1             4001
                   5000-5010

vxlan2             100

$

After:
$ ./bridge/bridge vni
dev               vni              group/remote
vxlan1             4001
                   5000-5010
vxlan2             100
$

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index b597a916..8f88a706 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -346,8 +346,6 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 	if (opened)
 		close_vni_port();
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
-
 	fflush(stdout);
 	return 0;
 }
-- 
2.43.0

[PATCH iproute2-next 13/20] bridge: vni: Replace open-coded instance of print_nl()

[Benjamin Poirier]

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 8f88a706..0d8c1d34 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -289,7 +289,7 @@ static void print_vni(struct rtattr *t, int ifindex)
 		print_vnifilter_entry_stats(ttb[VXLAN_VNIFILTER_ENTRY_STATS]);
 
 	close_json_object();
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 }
 
 int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
-- 
2.43.0

Re: [PATCH iproute2-next 13/20] bridge: vni: Replace open-coded instance of print_nl()

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:25 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Tested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH iproute2-next 14/20] bridge: vni: Remove unused argument in open_vni_port()

[Benjamin Poirier]

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 0d8c1d34..44781b01 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -147,7 +147,7 @@ static int vni_modify(int cmd, int argc, char **argv)
 	return 0;
 }
 
-static void open_vni_port(int ifi_index, const char *fmt)
+static void open_vni_port(int ifi_index)
 {
 	open_json_object(NULL);
 	print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname",
@@ -334,7 +334,7 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 			continue;
 
 		if (!opened) {
-			open_vni_port(tmsg->ifindex, "%s");
+			open_vni_port(tmsg->ifindex);
 			opened = true;
 		} else {
 			print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s  ", "");
-- 
2.43.0

[PATCH iproute2-next 15/20] bridge: vni: Align output columns

[Benjamin Poirier]

Use fixed column widths to improve readability.

These changes are similar to commit e0c457b1a5a2 ("bridge: Align output
columns").

Before:
$ bridge vni
dev               vni              group/remote
vxlan1             4001
                   4002           10.0.0.1
                   5000-5010
                   16777214-16777215        10.0.0.2
vxlan2             100

After:
$ bridge vni
dev               vni                group/remote
vxlan1            4001
                  4002               10.0.0.1
                  5000-5010
                  16777214-16777215  10.0.0.2
vxlan2            100

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index 44781b01..e9943872 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -23,7 +23,8 @@
 
 static unsigned int filter_index;
 
-#define VXLAN_ID_LEN 15
+/* max len of "<start>-<end>" */
+#define VXLAN_ID_LEN 17
 
 #define __stringify_1(x...) #x
 #define __stringify(x...) __stringify_1(x)
@@ -162,16 +163,18 @@ static void close_vni_port(void)
 	close_json_object();
 }
 
-static void print_range(const char *name, __u32 start, __u32 id)
+static unsigned int print_range(const char *name, __u32 start, __u32 id)
 {
 	char end[64];
+	int width;
 
 	snprintf(end, sizeof(end), "%sEnd", name);
 
-	print_uint(PRINT_ANY, name, " %u", start);
+	width = print_uint(PRINT_ANY, name, "%u", start);
 	if (start != id)
-		print_uint(PRINT_ANY, end, "-%-14u ", id);
+		width += print_uint(PRINT_ANY, end, "-%u", id);
 
+	return width;
 }
 
 static void print_vnifilter_entry_stats(struct rtattr *stats_attr)
@@ -231,7 +234,8 @@ static void print_vni(struct rtattr *t, int ifindex)
 {
 	struct rtattr *ttb[VXLAN_VNIFILTER_ENTRY_MAX+1];
 	__u32 vni_start = 0;
-	__u32 vni_end = 0;
+	unsigned int width;
+	__u32 vni_end;
 
 	parse_rtattr_flags(ttb, VXLAN_VNIFILTER_ENTRY_MAX, RTA_DATA(t),
 			   RTA_PAYLOAD(t), NLA_F_NESTED);
@@ -241,12 +245,13 @@ static void print_vni(struct rtattr *t, int ifindex)
 
 	if (ttb[VXLAN_VNIFILTER_ENTRY_END])
 		vni_end = rta_getattr_u32(ttb[VXLAN_VNIFILTER_ENTRY_END]);
+	else
+		vni_end = vni_start;
 
 	open_json_object(NULL);
-	if (vni_end)
-		print_range("vni", vni_start, vni_end);
-	else
-		print_uint(PRINT_ANY, "vni", " %-14u", vni_start);
+	width = print_range("vni", vni_start, vni_end);
+	if (!is_json_context())
+		printf("%-*s  ", VXLAN_ID_LEN - width, "");
 
 	if (ttb[VXLAN_VNIFILTER_ENTRY_GROUP]) {
 		__be32 addr = rta_getattr_u32(ttb[VXLAN_VNIFILTER_ENTRY_GROUP]);
@@ -255,12 +260,12 @@ static void print_vni(struct rtattr *t, int ifindex)
 			if (IN_MULTICAST(ntohl(addr)))
 				print_string(PRINT_ANY,
 					     "group",
-					     " %s",
+					     "%s",
 					     format_host(AF_INET, 4, &addr));
 			else
 				print_string(PRINT_ANY,
 					     "remote",
-					     " %s",
+					     "%s",
 					     format_host(AF_INET, 4, &addr));
 		}
 	} else if (ttb[VXLAN_VNIFILTER_ENTRY_GROUP6]) {
@@ -271,14 +276,14 @@ static void print_vni(struct rtattr *t, int ifindex)
 			if (IN6_IS_ADDR_MULTICAST(&addr))
 				print_string(PRINT_ANY,
 					     "group",
-					     " %s",
+					     "%s",
 					     format_host(AF_INET6,
 							 sizeof(struct in6_addr),
 							 &addr));
 			else
 				print_string(PRINT_ANY,
 					     "remote",
-					     " %s",
+					     "%s",
 					     format_host(AF_INET6,
 							 sizeof(struct in6_addr),
 							 &addr));
@@ -382,13 +387,10 @@ static int vni_show(int argc, char **argv)
 		exit(1);
 	}
 
-	if (!is_json_context()) {
+	if (!is_json_context())
 		printf("%-" __stringify(IFNAMSIZ) "s  %-"
-		       __stringify(VXLAN_ID_LEN) "s  %-"
-		       __stringify(15) "s",
-		       "dev", "vni", "group/remote");
-		printf("\n");
-	}
+		       __stringify(VXLAN_ID_LEN) "s  group/remote\n", "dev",
+		       "vni");
 
 	ret = rtnl_dump_filter(&rth, print_vnifilter_rtm, NULL);
 	if (ret < 0) {
-- 
2.43.0

[PATCH iproute2-next 16/20] bridge: vni: Indent statistics with 2 spaces

[Benjamin Poirier]

`bridge -s vlan` indents statistics with 2 spaces compared to the vlan id
column while `bridge -s vni` indents them with 1 space. Change `bridge vni`
to match the behavior of `bridge vlan` since that second command predates
`bridge vni`.

Before:
$ bridge -s vni
dev               vni                group/remote
vxlan1            4001
                   RX: bytes 0 pkts 0 drops 0 errors 0
                   TX: bytes 0 pkts 0 drops 0 errors 0
                  4002               10.0.0.1
                   RX: bytes 0 pkts 0 drops 0 errors 0
                   TX: bytes 0 pkts 0 drops 0 errors 0
vxlan2            100
                   RX: bytes 0 pkts 0 drops 0 errors 0
                   TX: bytes 0 pkts 0 drops 0 errors 0

After:
$ bridge -s vni
dev               vni                group/remote
vxlan1            4001
                    RX: bytes 0 pkts 0 drops 0 errors 0
                    TX: bytes 0 pkts 0 drops 0 errors 0
                  4002               10.0.0.1
                    RX: bytes 0 pkts 0 drops 0 errors 0
                    TX: bytes 0 pkts 0 drops 0 errors 0
vxlan2            100
                    RX: bytes 0 pkts 0 drops 0 errors 0
                    TX: bytes 0 pkts 0 drops 0 errors 0

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vni.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bridge/vni.c b/bridge/vni.c
index e9943872..2c6d506a 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -187,8 +187,8 @@ static void print_vnifilter_entry_stats(struct rtattr *stats_attr)
 			   RTA_PAYLOAD(stats_attr), NLA_F_NESTED);
 
 	print_nl();
-	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s   ", "");
-	print_string(PRINT_FP, NULL, "RX: ", "");
+	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s    RX: ",
+		     "");
 
 	if (stb[VNIFILTER_ENTRY_STATS_RX_BYTES]) {
 		stat = rta_getattr_u64(stb[VNIFILTER_ENTRY_STATS_RX_BYTES]);
@@ -208,8 +208,8 @@ static void print_vnifilter_entry_stats(struct rtattr *stats_attr)
 	}
 
 	print_nl();
-	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s   ", "");
-	print_string(PRINT_FP, NULL, "TX: ", "");
+	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s    TX: ",
+		     "");
 
 	if (stb[VNIFILTER_ENTRY_STATS_TX_BYTES]) {
 		stat = rta_getattr_u64(stb[VNIFILTER_ENTRY_STATS_TX_BYTES]);
-- 
2.43.0

[PATCH iproute2-next 17/20] bridge: Deduplicate print_range()

[Benjamin Poirier]

The two implementations are now identical so keep only one instance and
move it to json_print.c where there are already a few other specialized
printing functions.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vlan.c        | 14 --------------
 bridge/vni.c         | 14 --------------
 include/json_print.h |  2 ++
 lib/json_print.c     | 14 ++++++++++++++
 4 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 7a175b04..05e6a620 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -590,20 +590,6 @@ static void close_vlan_port(void)
 	close_json_object();
 }
 
-static unsigned int print_range(const char *name, __u32 start, __u32 id)
-{
-	char end[64];
-	int width;
-
-	snprintf(end, sizeof(end), "%sEnd", name);
-
-	width = print_uint(PRINT_ANY, name, "%u", start);
-	if (start != id)
-		width += print_uint(PRINT_ANY, end, "-%u", id);
-
-	return width;
-}
-
 static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 {
 	struct rtattr *i, *list = tb;
diff --git a/bridge/vni.c b/bridge/vni.c
index 2c6d506a..ffc3e188 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -163,20 +163,6 @@ static void close_vni_port(void)
 	close_json_object();
 }
 
-static unsigned int print_range(const char *name, __u32 start, __u32 id)
-{
-	char end[64];
-	int width;
-
-	snprintf(end, sizeof(end), "%sEnd", name);
-
-	width = print_uint(PRINT_ANY, name, "%u", start);
-	if (start != id)
-		width += print_uint(PRINT_ANY, end, "-%u", id);
-
-	return width;
-}
-
 static void print_vnifilter_entry_stats(struct rtattr *stats_attr)
 {
 	struct rtattr *stb[VNIFILTER_ENTRY_STATS_MAX+1];
diff --git a/include/json_print.h b/include/json_print.h
index 0b1d84f7..86dc5f16 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -97,6 +97,8 @@ static inline int print_rate(bool use_iec, enum output_type t,
 	return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate);
 }
 
+unsigned int print_range(const char *name, __u32 start, __u32 id);
+
 int print_color_bool_opt(enum output_type type, enum color_attr color,
 			 const char *key, bool value, bool show);
 
diff --git a/lib/json_print.c b/lib/json_print.c
index 602de027..072105c0 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -374,3 +374,17 @@ int print_color_rate(bool use_iec, enum output_type type, enum color_attr color,
 	free(buf);
 	return rc;
 }
+
+unsigned int print_range(const char *name, __u32 start, __u32 id)
+{
+	char end[64];
+	int width;
+
+	snprintf(end, sizeof(end), "%sEnd", name);
+
+	width = print_uint(PRINT_ANY, name, "%u", start);
+	if (start != id)
+		width += print_uint(PRINT_ANY, end, "-%u", id);
+
+	return width;
+}
-- 
2.43.0

[PATCH iproute2-next 18/20] json_print: Output to temporary buffer in print_range() only as needed

[Benjamin Poirier]

The string that's formatted in the "end" buffer is only needed when
outputting a range so move the snprintf() call within the condition.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 lib/json_print.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/json_print.c b/lib/json_print.c
index 072105c0..f38171ff 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -377,14 +377,15 @@ int print_color_rate(bool use_iec, enum output_type type, enum color_attr color,
 
 unsigned int print_range(const char *name, __u32 start, __u32 id)
 {
-	char end[64];
 	int width;
 
-	snprintf(end, sizeof(end), "%sEnd", name);
-
 	width = print_uint(PRINT_ANY, name, "%u", start);
-	if (start != id)
+	if (start != id) {
+		char end[64];
+
+		snprintf(end, sizeof(end), "%sEnd", name);
 		width += print_uint(PRINT_ANY, end, "-%u", id);
+	}
 
 	return width;
 }
-- 
2.43.0

[PATCH iproute2-next 19/20] json_print: Rename print_range() argument

[Benjamin Poirier]

The second argument's purpose is better conveyed by calling it "end" rather
than "id" so rename it.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 include/json_print.h |  2 +-
 lib/json_print.c     | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 86dc5f16..daebcf5d 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -97,7 +97,7 @@ static inline int print_rate(bool use_iec, enum output_type t,
 	return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate);
 }
 
-unsigned int print_range(const char *name, __u32 start, __u32 id);
+unsigned int print_range(const char *name, __u32 start, __u32 end);
 
 int print_color_bool_opt(enum output_type type, enum color_attr color,
 			 const char *key, bool value, bool show);
diff --git a/lib/json_print.c b/lib/json_print.c
index f38171ff..7b3b6c3f 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -375,16 +375,16 @@ int print_color_rate(bool use_iec, enum output_type type, enum color_attr color,
 	return rc;
 }
 
-unsigned int print_range(const char *name, __u32 start, __u32 id)
+unsigned int print_range(const char *name, __u32 start, __u32 end)
 {
 	int width;
 
 	width = print_uint(PRINT_ANY, name, "%u", start);
-	if (start != id) {
-		char end[64];
+	if (start != end) {
+		char buf[64];
 
-		snprintf(end, sizeof(end), "%sEnd", name);
-		width += print_uint(PRINT_ANY, end, "-%u", id);
+		snprintf(buf, sizeof(buf), "%sEnd", name);
+		width += print_uint(PRINT_ANY, buf, "-%u", end);
 	}
 
 	return width;
-- 
2.43.0

[PATCH iproute2-next 20/20] bridge: Provide rta_type()

[Benjamin Poirier]

Factor out the repeated code pattern
rta_type = attr->rta_type & NLA_TYPE_MASK
into a helper which is similar to the existing kernel function nla_type().

Reviewed-by: Petr Machata <petrm@nvidia.com>
Tested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/vlan.c        | 12 ++++++------
 bridge/vni.c         |  4 +---
 include/libnetlink.h |  4 ++++
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 05e6a620..5352eb24 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -851,7 +851,7 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex)
 	struct rtattr *vtb[BRIDGE_VLANDB_GOPTS_MAX + 1], *vattr;
 	__u16 vid, vrange = 0;
 
-	if ((a->rta_type & NLA_TYPE_MASK) != BRIDGE_VLANDB_GLOBAL_OPTIONS)
+	if (rta_type(a) != BRIDGE_VLANDB_GLOBAL_OPTIONS)
 		return;
 
 	parse_rtattr_flags(vtb, BRIDGE_VLANDB_GOPTS_MAX, RTA_DATA(a),
@@ -960,7 +960,7 @@ static void print_vlan_opts(struct rtattr *a, int ifindex)
 	__u16 vrange = 0;
 	__u8 state = 0;
 
-	if ((a->rta_type & NLA_TYPE_MASK) != BRIDGE_VLANDB_ENTRY)
+	if (rta_type(a) != BRIDGE_VLANDB_ENTRY)
 		return;
 
 	parse_rtattr_flags(vtb, BRIDGE_VLANDB_ENTRY_MAX, RTA_DATA(a),
@@ -1086,14 +1086,14 @@ int print_vlan_rtm(struct nlmsghdr *n, void *arg, bool monitor, bool global_only
 
 	rem = len;
 	for (a = BRVLAN_RTA(bvm); RTA_OK(a, rem); a = RTA_NEXT(a, rem)) {
-		unsigned short rta_type = a->rta_type & NLA_TYPE_MASK;
+		unsigned short attr_type = rta_type(a);
 
 		/* skip unknown attributes */
-		if (rta_type > BRIDGE_VLANDB_MAX ||
-		    (global_only && rta_type != BRIDGE_VLANDB_GLOBAL_OPTIONS))
+		if (attr_type > BRIDGE_VLANDB_MAX ||
+		    (global_only && attr_type != BRIDGE_VLANDB_GLOBAL_OPTIONS))
 			continue;
 
-		switch (rta_type) {
+		switch (attr_type) {
 		case BRIDGE_VLANDB_ENTRY:
 			print_vlan_opts(a, bvm->ifindex);
 			break;
diff --git a/bridge/vni.c b/bridge/vni.c
index ffc3e188..a7abe6de 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -319,9 +319,7 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg)
 
 	rem = len;
 	for (t = TUNNEL_RTA(tmsg); RTA_OK(t, rem); t = RTA_NEXT(t, rem)) {
-		unsigned short rta_type = t->rta_type & NLA_TYPE_MASK;
-
-		if (rta_type != VXLAN_VNIFILTER_ENTRY)
+		if (rta_type(t) != VXLAN_VNIFILTER_ENTRY)
 			continue;
 
 		if (!opened) {
diff --git a/include/libnetlink.h b/include/libnetlink.h
index 39ed87a7..ad7e7127 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -275,6 +275,10 @@ static inline const char *rta_getattr_str(const struct rtattr *rta)
 {
 	return (const char *)RTA_DATA(rta);
 }
+static inline int rta_type(const struct rtattr *rta)
+{
+	return rta->rta_type & NLA_TYPE_MASK;
+}
 
 int rtnl_listen_all_nsid(struct rtnl_handle *);
 int rtnl_listen(struct rtnl_handle *, rtnl_listen_filter_t handler,
-- 
2.43.0

Re: [PATCH iproute2-next 00/20] bridge: vni: UI fixes

[Stephen Hemminger]

On Mon, 11 Dec 2023 09:07:12 -0500
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> This series mainly contains fixes to `bridge vni` command input and output.
> There are also a few adjacent changes to `bridge vlan` and `bridge vni`.
> 
> Benjamin Poirier (20):
>   bridge: vni: Accept 'del' command
>   bridge: vni: Remove dead code in group argument parsing
>   bridge: vni: Fix duplicate group and remote error messages
>   bridge: vni: Report duplicate vni argument using duparg()
>   bridge: vni: Fix vni filter help strings
>   bridge: vlan: Use printf() to avoid temporary buffer
>   bridge: vlan: Remove paranoid check
>   bridge: vni: Remove print_vnifilter_rtm_filter()
>   bridge: vni: Move open_json_object() within print_vni()
>   bridge: vni: Guard close_vni_port() call
>   bridge: vni: Reverse the logic in print_vnifilter_rtm()
>   bridge: vni: Remove stray newlines after each interface
>   bridge: vni: Replace open-coded instance of print_nl()
>   bridge: vni: Remove unused argument in open_vni_port()
>   bridge: vni: Align output columns
>   bridge: vni: Indent statistics with 2 spaces
>   bridge: Deduplicate print_range()
>   json_print: Output to temporary buffer in print_range() only as needed
>   json_print: Rename print_range() argument
>   bridge: Provide rta_type()
> 
>  bridge/bridge.c      |   2 +-
>  bridge/vlan.c        |  38 +++------------
>  bridge/vni.c         | 113 +++++++++++++++++--------------------------
>  include/json_print.h |   2 +
>  include/libnetlink.h |   4 ++
>  lib/json_print.c     |  15 ++++++
>  6 files changed, 75 insertions(+), 99 deletions(-)
> 

These are all ok except the first one.

Please resubmit, and consider consolidating some of the patches.
Better to have 10 patches than 20.

Re: [PATCH iproute2-next 00/20] bridge: vni: UI fixes

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> consider consolidating some of the patches.
> Better to have 10 patches than 20.

The original that I reviewed internally was 7 patches. I asked Benjamin
to split some of it more, because it was tricky to figure out that all
the changes in an individual patch cancel out exactly right to deliver
what the commit message promised. But yeah, I bet there's a middle
ground.

Re: [PATCH iproute2-next 00/20] bridge: vni: UI fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2.git (main)
by Stephen Hemminger <stephen@networkplumber.org>:

On Mon, 11 Dec 2023 09:07:12 -0500 you wrote:
> This series mainly contains fixes to `bridge vni` command input and output.
> There are also a few adjacent changes to `bridge vlan` and `bridge vni`.
> 
> Benjamin Poirier (20):
>   bridge: vni: Accept 'del' command
>   bridge: vni: Remove dead code in group argument parsing
>   bridge: vni: Fix duplicate group and remote error messages
>   bridge: vni: Report duplicate vni argument using duparg()
>   bridge: vni: Fix vni filter help strings
>   bridge: vlan: Use printf() to avoid temporary buffer
>   bridge: vlan: Remove paranoid check
>   bridge: vni: Remove print_vnifilter_rtm_filter()
>   bridge: vni: Move open_json_object() within print_vni()
>   bridge: vni: Guard close_vni_port() call
>   bridge: vni: Reverse the logic in print_vnifilter_rtm()
>   bridge: vni: Remove stray newlines after each interface
>   bridge: vni: Replace open-coded instance of print_nl()
>   bridge: vni: Remove unused argument in open_vni_port()
>   bridge: vni: Align output columns
>   bridge: vni: Indent statistics with 2 spaces
>   bridge: Deduplicate print_range()
>   json_print: Output to temporary buffer in print_range() only as needed
>   json_print: Rename print_range() argument
>   bridge: Provide rta_type()
> 
> [...]

Here is the summary with links:
  - [iproute2-next,01/20] bridge: vni: Accept 'del' command
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=e8177094d515
  - [iproute2-next,02/20] bridge: vni: Remove dead code in group argument parsing
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=ba1e68f04be3
  - [iproute2-next,03/20] bridge: vni: Fix duplicate group and remote error messages
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=0b8c01b4058e
  - [iproute2-next,04/20] bridge: vni: Report duplicate vni argument using duparg()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=aeb7ee297361
  - [iproute2-next,05/20] bridge: vni: Fix vni filter help strings
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=58c8a0817558
  - [iproute2-next,06/20] bridge: vlan: Use printf() to avoid temporary buffer
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=cf7b528a21f6
  - [iproute2-next,07/20] bridge: vlan: Remove paranoid check
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=0b8508f44d85
  - [iproute2-next,08/20] bridge: vni: Remove print_vnifilter_rtm_filter()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=060eac10e764
  - [iproute2-next,09/20] bridge: vni: Move open_json_object() within print_vni()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=b627c387eb5c
  - [iproute2-next,10/20] bridge: vni: Guard close_vni_port() call
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=7418335b4b43
  - [iproute2-next,11/20] bridge: vni: Reverse the logic in print_vnifilter_rtm()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=14c9845f05f8
  - [iproute2-next,12/20] bridge: vni: Remove stray newlines after each interface
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=4f11b21e0257
  - [iproute2-next,13/20] bridge: vni: Replace open-coded instance of print_nl()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=7d8509d84c12
  - [iproute2-next,14/20] bridge: vni: Remove unused argument in open_vni_port()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=562b208a7b4d
  - [iproute2-next,15/20] bridge: vni: Align output columns
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=717f2f82f1da
  - [iproute2-next,16/20] bridge: vni: Indent statistics with 2 spaces
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=dd4e1749a977
  - [iproute2-next,17/20] bridge: Deduplicate print_range()
    (no matching commit)
  - [iproute2-next,18/20] json_print: Output to temporary buffer in print_range() only as needed
    (no matching commit)
  - [iproute2-next,19/20] json_print: Rename print_range() argument
    (no matching commit)
  - [iproute2-next,20/20] bridge: Provide rta_type()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=d205b5cf3877

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html