[PATCH iproute2 0/7] bridge vlan output fixes

[PATCH iproute2 0/7] bridge vlan output fixes

[Benjamin Poirier]

More fixes for `bridge vlan` and `bridge vlan tunnelshow` normal and JSON
mode output.

Most of the changes are cosmetic except for changes to JSON format (flag
names, no empty lists).

Benjamin Poirier (7):
  bridge: Use the same flag names in input and output
  bridge: Use consistent column names in vlan output
  bridge: Fix typo
  bridge: Fix output with empty vlan lists
  json_print: Return number of characters printed
  bridge: Align output columns
  Replace open-coded instances of print_nl()

 bridge/vlan.c                            | 115 +++++++++++++++--------
 include/json_print.h                     |  24 +++--
 lib/json_print.c                         |  95 ++++++++++++-------
 tc/m_action.c                            |  14 +--
 tc/m_connmark.c                          |   4 +-
 tc/m_ctinfo.c                            |   4 +-
 tc/m_ife.c                               |   4 +-
 tc/m_mpls.c                              |   2 +-
 tc/m_nat.c                               |   4 +-
 tc/m_sample.c                            |   4 +-
 tc/m_skbedit.c                           |   4 +-
 tc/m_tunnel_key.c                        |  16 ++--
 tc/q_taprio.c                            |   8 +-
 tc/tc_util.c                             |   4 +-
 testsuite/tests/bridge/vlan/show.t       |  30 ++++++
 testsuite/tests/bridge/vlan/tunnelshow.t |   2 +-
 16 files changed, 212 insertions(+), 122 deletions(-)
 create mode 100755 testsuite/tests/bridge/vlan/show.t

-- 
2.26.0

[PATCH iproute2 1/7] bridge: Use the same flag names in input and output

[Benjamin Poirier]

Output the same names for vlan flags as the ones accepted in command input.

Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 205851e4..08b19897 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -398,10 +398,10 @@ static void print_vlan_flags(__u16 flags)
 
 	open_json_array(PRINT_JSON, "flags");
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		print_string(PRINT_ANY, NULL, " %s", "PVID");
+		print_string(PRINT_ANY, NULL, " %s", "pvid");
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
-		print_string(PRINT_ANY, NULL, " %s", "Egress Untagged");
+		print_string(PRINT_ANY, NULL, " %s", "untagged");
 	close_json_array(PRINT_JSON, NULL);
 }
 
-- 
2.26.0

[PATCH iproute2 2/7] bridge: Use consistent column names in vlan output

[Benjamin Poirier]

Fix singular vs plural. Add a hyphen to clarify that each of those are
single fields.

Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 08b19897..a708e6d2 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -538,9 +538,9 @@ static int vlan_show(int argc, char **argv, int subject)
 		}
 
 		if (!is_json_context()) {
-			printf("port\tvlan ids");
+			printf("port\tvlan-id");
 			if (subject == VLAN_SHOW_TUNNELINFO)
-				printf("\ttunnel id");
+				printf("\ttunnel-id");
 			printf("\n");
 		}
 
@@ -559,7 +559,7 @@ static int vlan_show(int argc, char **argv, int subject)
 		}
 
 		if (!is_json_context())
-			printf("%-16s vlan id\n", "port");
+			printf("%-16s vlan-id\n", "port");
 
 		if (rtnl_dump_filter(&rth, print_vlan_stats, stdout) < 0) {
 			fprintf(stderr, "Dump terminated\n");
-- 
2.26.0

[PATCH iproute2 3/7] bridge: Fix typo

[Benjamin Poirier]

Fixes: 7abf5de677e3 ("bridge: vlan: add support to display per-vlan statistics")
Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index a708e6d2..3ed60951 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -546,7 +546,7 @@ static int vlan_show(int argc, char **argv, int subject)
 
 		ret = rtnl_dump_filter(&rth, print_vlan, &subject);
 		if (ret < 0) {
-			fprintf(stderr, "Dump ternminated\n");
+			fprintf(stderr, "Dump terminated\n");
 			exit(1);
 		}
 	} else {
-- 
2.26.0

[PATCH iproute2 4/7] bridge: Fix output with empty vlan lists

[Benjamin Poirier]

Consider this configuration:

ip link add br0 type bridge
ip link add vx0 type vxlan dstport 4789 external
ip link set dev vx0 master br0
bridge vlan del vid 1 dev vx0
ip link add vx1 type vxlan dstport 4790 external
ip link set dev vx1 master br0

	root@vsid:/src/iproute2# ./bridge/bridge vlan
	port    vlan ids
	br0      1 pvid untagged

	vx0     None
	vx1      1 pvid untagged

	root@vsid:/src/iproute2#

Note the useless and inconsistent empty lines.

	root@vsid:/src/iproute2# ./bridge/bridge vlan tunnelshow
	port    vlan ids        tunnel id
	br0
	vx0     None
	vx1

What's the difference between "None" and ""?

	root@vsid:/src/iproute2# ./bridge/bridge -j -p vlan tunnelshow
	[ {
		"ifname": "br0",
		"tunnels": [ ]
	    },{
		"ifname": "vx1",
		"tunnels": [ ]
	    } ]

Why does vx0 appear in normal output and not json output?
Why output an empty list for br0 and vx1?

Fix these inconsistencies and avoid outputting entries with no values. This
makes the behavior consistent with other iproute2 commands, for example
`ip -6 addr`: if an interface doesn't have any ipv6 addresses, it is not
part of the listing.

Fixes: 8652eeb3ab12 ("bridge: vlan: support for per vlan tunnel info")
Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c                            | 36 +++++++++++++-----------
 testsuite/tests/bridge/vlan/show.t       | 30 ++++++++++++++++++++
 testsuite/tests/bridge/vlan/tunnelshow.t |  2 +-
 3 files changed, 50 insertions(+), 18 deletions(-)
 create mode 100755 testsuite/tests/bridge/vlan/show.t

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 3ed60951..1ca7322a 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -290,8 +290,7 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 	int rem = RTA_PAYLOAD(list);
 	__u16 last_vid_start = 0;
 	__u32 last_tunid_start = 0;
-
-	open_vlan_port(ifindex, "%s", VLAN_SHOW_TUNNELINFO);
+	bool opened = false;
 
 	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
 		struct rtattr *ttb[IFLA_BRIDGE_VLAN_TUNNEL_MAX+1];
@@ -331,13 +330,20 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 		else if (vcheck_ret == 0)
 			continue;
 
+		if (!opened) {
+			open_vlan_port(ifindex, "%s", VLAN_SHOW_TUNNELINFO);
+			opened = true;
+		}
+
 		open_json_object(NULL);
 		print_range("vlan", last_vid_start, tunnel_vid);
 		print_range("tunid", last_tunid_start, tunnel_id);
 		close_json_object();
 		print_string(PRINT_FP, NULL, "%s", _SL_);
 	}
-	close_vlan_port();
+
+	if (opened)
+		close_vlan_port();
 }
 
 static int print_vlan(struct nlmsghdr *n, void *arg)
@@ -366,16 +372,8 @@ static int print_vlan(struct nlmsghdr *n, void *arg)
 		return 0;
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifm), len);
-
-	/* if AF_SPEC isn't there, vlan table is not preset for this port */
-	if (!tb[IFLA_AF_SPEC]) {
-		if (!filter_vlan && !is_json_context()) {
-			color_fprintf(stdout, COLOR_IFNAME, "%s",
-				      ll_index_to_name(ifm->ifi_index));
-			fprintf(stdout, "\tNone\n");
-		}
+	if (!tb[IFLA_AF_SPEC])
 		return 0;
-	}
 
 	switch (*subject) {
 	case VLAN_SHOW_VLAN:
@@ -385,9 +383,7 @@ static int print_vlan(struct nlmsghdr *n, void *arg)
 		print_vlan_tunnel_info(tb[IFLA_AF_SPEC], ifm->ifi_index);
 		break;
 	}
-	print_string(PRINT_FP, NULL, "%s", _SL_);
 
-	fflush(stdout);
 	return 0;
 }
 
@@ -588,8 +584,7 @@ void print_vlan_info(struct rtattr *tb, int ifindex)
 	struct rtattr *i, *list = tb;
 	int rem = RTA_PAYLOAD(list);
 	__u16 last_vid_start = 0;
-
-	open_vlan_port(ifindex, "%s", VLAN_SHOW_VLAN);
+	bool opened = false;
 
 	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
 		struct bridge_vlan_info *vinfo;
@@ -608,6 +603,11 @@ void print_vlan_info(struct rtattr *tb, int ifindex)
 		else if (vcheck_ret == 0)
 			continue;
 
+		if (!opened) {
+			open_vlan_port(ifindex, "%s", VLAN_SHOW_VLAN);
+			opened = true;
+		}
+
 		open_json_object(NULL);
 		print_range("vlan", last_vid_start, vinfo->vid);
 
@@ -615,7 +615,9 @@ void print_vlan_info(struct rtattr *tb, int ifindex)
 		close_json_object();
 		print_string(PRINT_FP, NULL, "%s", _SL_);
 	}
-	close_vlan_port();
+
+	if (opened)
+		close_vlan_port();
 }
 
 int do_vlan(int argc, char **argv)
diff --git a/testsuite/tests/bridge/vlan/show.t b/testsuite/tests/bridge/vlan/show.t
new file mode 100755
index 00000000..3def2022
--- /dev/null
+++ b/testsuite/tests/bridge/vlan/show.t
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+. lib/generic.sh
+
+ts_log "[Testing vlan show]"
+
+BR_DEV="$(rand_dev)"
+VX0_DEV="$(rand_dev)"
+VX1_DEV="$(rand_dev)"
+
+ts_ip "$0" "Add $BR_DEV bridge interface" link add $BR_DEV type bridge
+
+ts_ip "$0" "Add $VX0_DEV vxlan interface" \
+	link add $VX0_DEV type vxlan dstport 4789 external
+ts_ip "$0" "Enslave $VX0_DEV under $BR_DEV" \
+	link set dev $VX0_DEV master $BR_DEV
+ts_bridge "$0" "Delete default vlan from $VX0_DEV" \
+	vlan del dev $VX0_DEV vid 1
+ts_ip "$0" "Add $VX1_DEV vxlan interface" \
+	link add $VX1_DEV type vxlan dstport 4790 external
+ts_ip "$0" "Enslave $VX1_DEV under $BR_DEV" \
+	link set dev $VX1_DEV master $BR_DEV
+
+# Test that bridge ports without vlans do not appear in the output
+ts_bridge "$0" "Show vlan" vlan
+test_on_not "$VX0_DEV"
+
+# Test that bridge ports without tunnels do not appear in the output
+ts_bridge "$0" "Show vlan tunnel info" vlan tunnelshow
+test_lines_count 1 # header only
diff --git a/testsuite/tests/bridge/vlan/tunnelshow.t b/testsuite/tests/bridge/vlan/tunnelshow.t
index fd41bfcb..3e9c12a2 100755
--- a/testsuite/tests/bridge/vlan/tunnelshow.t
+++ b/testsuite/tests/bridge/vlan/tunnelshow.t
@@ -28,6 +28,6 @@ ts_bridge "$0" "Add tunnel with vni > 16k" \
 
 ts_bridge "$0" "Show tunnel info" vlan tunnelshow dev $VX_DEV
 test_on "1030\s+65556"
-test_lines_count 5
+test_lines_count 4
 
 ts_bridge "$0" "Dump tunnel info" -j vlan tunnelshow dev $VX_DEV
-- 
2.26.0

[PATCH iproute2 5/7] json_print: Return number of characters printed

[Benjamin Poirier]

When outputting in normal mode, forward the return value from
color_fprintf().

Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 include/json_print.h | 24 ++++++-----
 lib/json_print.c     | 95 +++++++++++++++++++++++++++-----------------
 2 files changed, 73 insertions(+), 46 deletions(-)

diff --git a/include/json_print.h b/include/json_print.h
index 34444793..50e71de4 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -44,20 +44,24 @@ void close_json_array(enum output_type type, const char *delim);
 void print_nl(void);
 
 #define _PRINT_FUNC(type_name, type)					\
-	void print_color_##type_name(enum output_type t,		\
-				     enum color_attr color,		\
-				     const char *key,			\
-				     const char *fmt,			\
-				     type value);			\
+	int print_color_##type_name(enum output_type t,			\
+				    enum color_attr color,		\
+				    const char *key,			\
+				    const char *fmt,			\
+				    type value);			\
 									\
-	static inline void print_##type_name(enum output_type t,	\
-					     const char *key,		\
-					     const char *fmt,		\
-					     type value)		\
+	static inline int print_##type_name(enum output_type t,		\
+					    const char *key,		\
+					    const char *fmt,		\
+					    type value)			\
 	{								\
-		print_color_##type_name(t, COLOR_NONE, key, fmt, value);	\
+		return print_color_##type_name(t, COLOR_NONE, key, fmt,	\
+					       value);			\
 	}
 
+/* These functions return 0 if printing to a JSON context, number of
+ * characters printed otherwise (as calculated by printf(3)).
+ */
 _PRINT_FUNC(int, int)
 _PRINT_FUNC(s64, int64_t)
 _PRINT_FUNC(bool, bool)
diff --git a/lib/json_print.c b/lib/json_print.c
index 8e7f32dc..fe0705bf 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -123,20 +123,22 @@ void close_json_array(enum output_type type, const char *str)
  */
 #define _PRINT_FUNC(type_name, type)					\
 	__attribute__((format(printf, 4, 0)))				\
-	void print_color_##type_name(enum output_type t,		\
-				     enum color_attr color,		\
-				     const char *key,			\
-				     const char *fmt,			\
-				     type value)			\
+	int print_color_##type_name(enum output_type t,			\
+				    enum color_attr color,		\
+				    const char *key,			\
+				    const char *fmt,			\
+				    type value)				\
 	{								\
+		int ret = 0;						\
 		if (_IS_JSON_CONTEXT(t)) {				\
 			if (!key)					\
 				jsonw_##type_name(_jw, value);		\
 			else						\
 				jsonw_##type_name##_field(_jw, key, value); \
 		} else if (_IS_FP_CONTEXT(t)) {				\
-			color_fprintf(stdout, color, fmt, value);          \
+			ret = color_fprintf(stdout, color, fmt, value); \
 		}							\
+		return ret;						\
 	}
 _PRINT_FUNC(int, int);
 _PRINT_FUNC(s64, int64_t);
@@ -162,12 +164,14 @@ _PRINT_NAME_VALUE_FUNC(uint, unsigned int, u);
 _PRINT_NAME_VALUE_FUNC(string, const char*, s);
 #undef _PRINT_NAME_VALUE_FUNC
 
-void print_color_string(enum output_type type,
-			enum color_attr color,
-			const char *key,
-			const char *fmt,
-			const char *value)
+int print_color_string(enum output_type type,
+		       enum color_attr color,
+		       const char *key,
+		       const char *fmt,
+		       const char *value)
 {
+	int ret = 0;
+
 	if (_IS_JSON_CONTEXT(type)) {
 		if (key && !value)
 			jsonw_name(_jw, key);
@@ -176,8 +180,10 @@ void print_color_string(enum output_type type,
 		else
 			jsonw_string_field(_jw, key, value);
 	} else if (_IS_FP_CONTEXT(type)) {
-		color_fprintf(stdout, color, fmt, value);
+		ret = color_fprintf(stdout, color, fmt, value);
 	}
+
+	return ret;
 }
 
 /*
@@ -185,47 +191,58 @@ void print_color_string(enum output_type type,
  * a value to it, you will need to use "is_json_context()" to have different
  * branch for json and regular output. grep -r "print_bool" for example
  */
-void print_color_bool(enum output_type type,
-		      enum color_attr color,
-		      const char *key,
-		      const char *fmt,
-		      bool value)
+int print_color_bool(enum output_type type,
+		     enum color_attr color,
+		     const char *key,
+		     const char *fmt,
+		     bool value)
 {
+	int ret = 0;
+
 	if (_IS_JSON_CONTEXT(type)) {
 		if (key)
 			jsonw_bool_field(_jw, key, value);
 		else
 			jsonw_bool(_jw, value);
 	} else if (_IS_FP_CONTEXT(type)) {
-		color_fprintf(stdout, color, fmt, value ? "true" : "false");
+		ret = color_fprintf(stdout, color, fmt,
+				    value ? "true" : "false");
 	}
+
+	return ret;
 }
 
 /*
  * In JSON context uses hardcode %#x format: 42 -> 0x2a
  */
-void print_color_0xhex(enum output_type type,
-		       enum color_attr color,
-		       const char *key,
-		       const char *fmt,
-		       unsigned long long hex)
+int print_color_0xhex(enum output_type type,
+		      enum color_attr color,
+		      const char *key,
+		      const char *fmt,
+		      unsigned long long hex)
 {
+	int ret = 0;
+
 	if (_IS_JSON_CONTEXT(type)) {
 		SPRINT_BUF(b1);
 
 		snprintf(b1, sizeof(b1), "%#llx", hex);
 		print_string(PRINT_JSON, key, NULL, b1);
 	} else if (_IS_FP_CONTEXT(type)) {
-		color_fprintf(stdout, color, fmt, hex);
+		ret = color_fprintf(stdout, color, fmt, hex);
 	}
+
+	return ret;
 }
 
-void print_color_hex(enum output_type type,
-		     enum color_attr color,
-		     const char *key,
-		     const char *fmt,
-		     unsigned int hex)
+int print_color_hex(enum output_type type,
+		    enum color_attr color,
+		    const char *key,
+		    const char *fmt,
+		    unsigned int hex)
 {
+	int ret = 0;
+
 	if (_IS_JSON_CONTEXT(type)) {
 		SPRINT_BUF(b1);
 
@@ -235,28 +252,34 @@ void print_color_hex(enum output_type type,
 		else
 			jsonw_string(_jw, b1);
 	} else if (_IS_FP_CONTEXT(type)) {
-		color_fprintf(stdout, color, fmt, hex);
+		ret = color_fprintf(stdout, color, fmt, hex);
 	}
+
+	return ret;
 }
 
 /*
  * In JSON context we don't use the argument "value" we simply call jsonw_null
  * whereas FP context can use "value" to output anything
  */
-void print_color_null(enum output_type type,
-		      enum color_attr color,
-		      const char *key,
-		      const char *fmt,
-		      const char *value)
+int print_color_null(enum output_type type,
+		     enum color_attr color,
+		     const char *key,
+		     const char *fmt,
+		     const char *value)
 {
+	int ret = 0;
+
 	if (_IS_JSON_CONTEXT(type)) {
 		if (key)
 			jsonw_null_field(_jw, key);
 		else
 			jsonw_null(_jw);
 	} else if (_IS_FP_CONTEXT(type)) {
-		color_fprintf(stdout, color, fmt, value);
+		ret = color_fprintf(stdout, color, fmt, value);
 	}
+
+	return ret;
 }
 
 /* Print line separator (if not in JSON mode) */
-- 
2.26.0

[PATCH iproute2 6/7] bridge: Align output columns

[Benjamin Poirier]

Use fixed column widths to improve readability.

Before:
root@vsid:/src/iproute2# ./bridge/bridge vlan tunnelshow
port    vlan-id tunnel-id
vx0      1000    1000
         1010-1020       1010-1020
         1030    65556
vx-longname      10      10

After:
root@vsid:/src/iproute2# ./bridge/bridge vlan tunnelshow
port              vlan-id    tunnel-id
vx0               1000       1000
                  1010-1020  1010-1020
                  1030       65556
vx-longname       10         10

Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c | 73 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index 1ca7322a..a50a4fc9 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -22,6 +22,11 @@ enum vlan_show_subject {
 	VLAN_SHOW_TUNNELINFO,
 };
 
+#define VLAN_ID_LEN 9
+
+#define __stringify_1(x...) #x
+#define __stringify(x...) __stringify_1(x)
+
 static void usage(void)
 {
 	fprintf(stderr,
@@ -256,11 +261,11 @@ static int filter_vlan_check(__u16 vid, __u16 flags)
 	return 1;
 }
 
-static void open_vlan_port(int ifi_index, const char *fmt,
-			   enum vlan_show_subject subject)
+static void open_vlan_port(int ifi_index, enum vlan_show_subject subject)
 {
 	open_json_object(NULL);
-	print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", fmt,
+	print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname",
+			   "%-" __stringify(IFNAMSIZ) "s  ",
 			   ll_index_to_name(ifi_index));
 	open_json_array(PRINT_JSON,
 			subject == VLAN_SHOW_VLAN ? "vlans": "tunnels");
@@ -272,16 +277,18 @@ static void close_vlan_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, "\t %u", start);
+	width = print_uint(PRINT_ANY, name, "%u", start);
 	if (start != id)
-		print_uint(PRINT_ANY, end, "-%u", id);
+		width += print_uint(PRINT_ANY, end, "-%u", id);
 
+	return width;
 }
 
 static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
@@ -297,6 +304,7 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 		__u32 tunnel_id = 0;
 		__u16 tunnel_vid = 0;
 		__u16 tunnel_flags = 0;
+		unsigned int width;
 		int vcheck_ret;
 
 		if (i->rta_type != IFLA_BRIDGE_VLAN_TUNNEL_INFO)
@@ -331,12 +339,25 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 			continue;
 
 		if (!opened) {
-			open_vlan_port(ifindex, "%s", VLAN_SHOW_TUNNELINFO);
+			open_vlan_port(ifindex, VLAN_SHOW_TUNNELINFO);
 			opened = true;
+		} else {
+			print_string(PRINT_FP, NULL,
+				     "%-" __stringify(IFNAMSIZ) "s  ", "");
 		}
 
 		open_json_object(NULL);
-		print_range("vlan", last_vid_start, tunnel_vid);
+		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);
+		} else {
+			fprintf(stderr, "BUG: vlan range too wide, %u\n",
+				width);
+		}
 		print_range("tunid", last_tunid_start, tunnel_id);
 		close_json_object();
 		print_string(PRINT_FP, NULL, "%s", _SL_);
@@ -404,20 +425,23 @@ static void print_vlan_flags(__u16 flags)
 static void print_one_vlan_stats(const struct bridge_vlan_xstats *vstats)
 {
 	open_json_object(NULL);
-	print_hu(PRINT_ANY, "vid", " %hu", vstats->vid);
 
+	print_hu(PRINT_ANY, "vid", "%hu", vstats->vid);
 	print_vlan_flags(vstats->flags);
+	print_nl();
 
-	print_lluint(PRINT_ANY, "rx_bytes",
-		     "\n                   RX: %llu bytes",
+	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s    ", "");
+	print_lluint(PRINT_ANY, "rx_bytes", "RX: %llu bytes",
 		     vstats->rx_bytes);
 	print_lluint(PRINT_ANY, "rx_packets", " %llu packets\n",
-		vstats->rx_packets);
-	print_lluint(PRINT_ANY, "tx_bytes",
-		     "                   TX: %llu bytes",
+		     vstats->rx_packets);
+
+	print_string(PRINT_FP, NULL, "%-" __stringify(IFNAMSIZ) "s    ", "");
+	print_lluint(PRINT_ANY, "tx_bytes", "TX: %llu bytes",
 		     vstats->tx_bytes);
 	print_lluint(PRINT_ANY, "tx_packets", " %llu packets\n",
-		vstats->tx_packets);
+		     vstats->tx_packets);
+
 	close_json_object();
 }
 
@@ -452,10 +476,11 @@ static void print_vlan_stats_attr(struct rtattr *attr, int ifindex)
 
 		/* found vlan stats, first time print the interface name */
 		if (!found_vlan) {
-			open_vlan_port(ifindex, "%-16s", VLAN_SHOW_VLAN);
+			open_vlan_port(ifindex, VLAN_SHOW_VLAN);
 			found_vlan = true;
 		} else {
-			print_string(PRINT_FP, NULL, "%-16s", "");
+			print_string(PRINT_FP, NULL,
+				     "%-" __stringify(IFNAMSIZ) "s  ", "");
 		}
 		print_one_vlan_stats(vstats);
 	}
@@ -534,9 +559,11 @@ static int vlan_show(int argc, char **argv, int subject)
 		}
 
 		if (!is_json_context()) {
-			printf("port\tvlan-id");
+			printf("%-" __stringify(IFNAMSIZ) "s  %-"
+			       __stringify(VLAN_ID_LEN) "s", "port",
+			       "vlan-id");
 			if (subject == VLAN_SHOW_TUNNELINFO)
-				printf("\ttunnel-id");
+				printf("  tunnel-id");
 			printf("\n");
 		}
 
@@ -555,7 +582,8 @@ static int vlan_show(int argc, char **argv, int subject)
 		}
 
 		if (!is_json_context())
-			printf("%-16s vlan-id\n", "port");
+			printf("%-" __stringify(IFNAMSIZ) "s  vlan-id\n",
+			       "port");
 
 		if (rtnl_dump_filter(&rth, print_vlan_stats, stdout) < 0) {
 			fprintf(stderr, "Dump terminated\n");
@@ -604,8 +632,11 @@ void print_vlan_info(struct rtattr *tb, int ifindex)
 			continue;
 
 		if (!opened) {
-			open_vlan_port(ifindex, "%s", VLAN_SHOW_VLAN);
+			open_vlan_port(ifindex, VLAN_SHOW_VLAN);
 			opened = true;
+		} else {
+			print_string(PRINT_FP, NULL, "%-"
+				     __stringify(IFNAMSIZ) "s  ", "");
 		}
 
 		open_json_object(NULL);
-- 
2.26.0

[PATCH iproute2 7/7] Replace open-coded instances of print_nl()

[Benjamin Poirier]

Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
---
 bridge/vlan.c     |  4 ++--
 tc/m_action.c     | 14 +++++++-------
 tc/m_connmark.c   |  4 ++--
 tc/m_ctinfo.c     |  4 ++--
 tc/m_ife.c        |  4 ++--
 tc/m_mpls.c       |  2 +-
 tc/m_nat.c        |  4 ++--
 tc/m_sample.c     |  4 ++--
 tc/m_skbedit.c    |  4 ++--
 tc/m_tunnel_key.c | 16 ++++++++--------
 tc/q_taprio.c     |  8 ++++----
 tc/tc_util.c      |  4 ++--
 12 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/bridge/vlan.c b/bridge/vlan.c
index a50a4fc9..ff27ea36 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -360,7 +360,7 @@ static void print_vlan_tunnel_info(struct rtattr *tb, int ifindex)
 		}
 		print_range("tunid", last_tunid_start, tunnel_id);
 		close_json_object();
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 
 	if (opened)
@@ -644,7 +644,7 @@ void print_vlan_info(struct rtattr *tb, int ifindex)
 
 		print_vlan_flags(vinfo->flags);
 		close_json_object();
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 
 	if (opened)
diff --git a/tc/m_action.c b/tc/m_action.c
index b41782de..66e67245 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -177,7 +177,7 @@ static void print_hw_stats(const struct rtattr *arg, bool print_used)
 			print_string(PRINT_ANY, NULL, " %s", item->str);
 	}
 	close_json_array(PRINT_JSON, NULL);
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 }
 
 static int parse_hw_stats(const char *str, struct nlmsghdr *n)
@@ -376,11 +376,11 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg)
 
 	if (show_stats && tb[TCA_ACT_STATS]) {
 		print_string(PRINT_FP, NULL, "\tAction statistics:", NULL);
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 		open_json_object("stats");
 		print_tcstats2_attr(f, tb[TCA_ACT_STATS], "\t", NULL);
 		close_json_object();
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 	if (tb[TCA_ACT_COOKIE]) {
 		int strsz = RTA_PAYLOAD(tb[TCA_ACT_COOKIE]);
@@ -389,7 +389,7 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg)
 		print_string(PRINT_ANY, "cookie", "\tcookie %s",
 			     hexstring_n2a(RTA_DATA(tb[TCA_ACT_COOKIE]),
 					   strsz, b1, sizeof(b1)));
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 	if (tb[TCA_ACT_FLAGS]) {
 		struct nla_bitfield32 *flags = RTA_DATA(tb[TCA_ACT_FLAGS]);
@@ -398,7 +398,7 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg)
 			print_bool(PRINT_ANY, "no_percpu", "\tno_percpu",
 				   flags->value &
 				   TCA_ACT_FLAGS_NO_PERCPU_STATS);
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 	if (tb[TCA_ACT_HW_STATS])
 		print_hw_stats(tb[TCA_ACT_HW_STATS], false);
@@ -458,7 +458,7 @@ tc_print_action(FILE *f, const struct rtattr *arg, unsigned short tot_acts)
 	for (i = 0; i <= tot_acts; i++) {
 		if (tb[i]) {
 			open_json_object(NULL);
-			print_string(PRINT_FP, NULL, "%s", _SL_);
+			print_nl();
 			print_uint(PRINT_ANY, "order",
 				   "\taction order %u: ", i);
 			if (tc_print_one_action(f, tb[i]) < 0) {
@@ -497,7 +497,7 @@ int print_action(struct nlmsghdr *n, void *arg)
 	open_json_object(NULL);
 	print_uint(PRINT_ANY, "total acts", "total acts %u",
 		   tot_acts ? *tot_acts : 0);
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	close_json_object();
 	if (tb[TCA_ACT_TAB] == NULL) {
 		if (n->nlmsg_type != RTM_GETACTION)
diff --git a/tc/m_connmark.c b/tc/m_connmark.c
index eac23489..4b2dc4e2 100644
--- a/tc/m_connmark.c
+++ b/tc/m_connmark.c
@@ -125,7 +125,7 @@ static int print_connmark(struct action_util *au, FILE *f, struct rtattr *arg)
 	print_uint(PRINT_ANY, "zone", "zone %u", ci->zone);
 	print_action_control(f, " ", ci->action, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
 	print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
@@ -137,7 +137,7 @@ static int print_connmark(struct action_util *au, FILE *f, struct rtattr *arg)
 			print_tm(f, tm);
 		}
 	}
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
index 5e451f87..e5c1b436 100644
--- a/tc/m_ctinfo.c
+++ b/tc/m_ctinfo.c
@@ -238,7 +238,7 @@ static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
 	print_hu(PRINT_ANY, "zone", "zone %u", zone);
 	print_action_control(f, " ", ci->action, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
 	print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
@@ -256,7 +256,7 @@ static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
 	if (show_stats)
 		print_ctinfo_stats(f, tb);
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_ife.c b/tc/m_ife.c
index 7c612c02..6a85e087 100644
--- a/tc/m_ife.c
+++ b/tc/m_ife.c
@@ -311,7 +311,7 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
 					 sizeof(b2)));
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", p->index);
 	print_int(PRINT_ANY, "ref", " ref %d", p->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", p->bindcnt);
@@ -324,7 +324,7 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_mpls.c b/tc/m_mpls.c
index 50eba01c..3d5d9b25 100644
--- a/tc/m_mpls.c
+++ b/tc/m_mpls.c
@@ -265,7 +265,7 @@ static int print_mpls(struct action_util *au, FILE *f, struct rtattr *arg)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_nat.c b/tc/m_nat.c
index c4b02a83..56e8f47c 100644
--- a/tc/m_nat.c
+++ b/tc/m_nat.c
@@ -172,7 +172,7 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
 		     format_host_r(AF_INET, 4, &sel->new_addr, buf1, sizeof(buf1)));
 
 	print_action_control(f, " ", sel->action, "");
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", sel->index);
 	print_int(PRINT_ANY, "ref", " ref %d", sel->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", sel->bindcnt);
@@ -185,7 +185,7 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_sample.c b/tc/m_sample.c
index c068e632..4a30513a 100644
--- a/tc/m_sample.c
+++ b/tc/m_sample.c
@@ -167,7 +167,7 @@ static int print_sample(struct action_util *au, FILE *f, struct rtattr *arg)
 
 	print_action_control(f, " ", p->action, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", p->index);
 	print_int(PRINT_ANY, "ref", " ref %d", p->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", p->bindcnt);
@@ -179,7 +179,7 @@ static int print_sample(struct action_util *au, FILE *f, struct rtattr *arg)
 			print_tm(f, tm);
 		}
 	}
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	return 0;
 }
 
diff --git a/tc/m_skbedit.c b/tc/m_skbedit.c
index 761cad58..9afe2f0c 100644
--- a/tc/m_skbedit.c
+++ b/tc/m_skbedit.c
@@ -254,7 +254,7 @@ static int print_skbedit(struct action_util *au, FILE *f, struct rtattr *arg)
 
 	print_action_control(f, " ", p->action, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", p->index);
 	print_int(PRINT_ANY, "ref", " ref %d", p->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", p->bindcnt);
@@ -267,7 +267,7 @@ static int print_skbedit(struct action_util *au, FILE *f, struct rtattr *arg)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/m_tunnel_key.c b/tc/m_tunnel_key.c
index 8fde6891..1f6921f3 100644
--- a/tc/m_tunnel_key.c
+++ b/tc/m_tunnel_key.c
@@ -367,7 +367,7 @@ static void tunnel_key_print_ip_addr(FILE *f, const char *name,
 	else
 		return;
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	if (matches(name, "src_ip") == 0)
 		print_string(PRINT_ANY, "src_ip", "\tsrc_ip %s",
 			     rt_addr_n2a_rta(family, attr));
@@ -381,7 +381,7 @@ static void tunnel_key_print_key_id(FILE *f, const char *name,
 {
 	if (!attr)
 		return;
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "key_id", "\tkey_id %u", rta_getattr_be32(attr));
 }
 
@@ -390,7 +390,7 @@ static void tunnel_key_print_dst_port(FILE *f, char *name,
 {
 	if (!attr)
 		return;
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "dst_port", "\tdst_port %u",
 		   rta_getattr_be16(attr));
 }
@@ -401,7 +401,7 @@ static void tunnel_key_print_flag(FILE *f, const char *name_on,
 {
 	if (!attr)
 		return;
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_string(PRINT_ANY, "flag", "\t%s",
 		     rta_getattr_u8(attr) ? name_on : name_off);
 }
@@ -473,11 +473,11 @@ static void tunnel_key_print_tos_ttl(FILE *f, char *name,
 		return;
 
 	if (matches(name, "tos") == 0 && rta_getattr_u8(attr) != 0) {
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 		print_uint(PRINT_ANY, "tos", "\ttos 0x%x",
 			   rta_getattr_u8(attr));
 	} else if (matches(name, "ttl") == 0 && rta_getattr_u8(attr) != 0) {
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 		print_uint(PRINT_ANY, "ttl", "\tttl %u",
 			   rta_getattr_u8(attr));
 	}
@@ -531,7 +531,7 @@ static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
 	}
 	print_action_control(f, " ", parm->action, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_uint(PRINT_ANY, "index", "\t index %u", parm->index);
 	print_int(PRINT_ANY, "ref", " ref %d", parm->refcnt);
 	print_int(PRINT_ANY, "bind", " bind %d", parm->bindcnt);
@@ -544,7 +544,7 @@ static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	return 0;
 }
diff --git a/tc/q_taprio.c b/tc/q_taprio.c
index b9954436..e43db9d0 100644
--- a/tc/q_taprio.c
+++ b/tc/q_taprio.c
@@ -368,7 +368,7 @@ static int print_sched_list(FILE *f, struct rtattr *list)
 
 	open_json_array(PRINT_JSON, "schedule");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	for (item = RTA_DATA(list); RTA_OK(item, rem); item = RTA_NEXT(item, rem)) {
 		struct rtattr *tb[TCA_TAPRIO_SCHED_ENTRY_MAX + 1];
@@ -396,7 +396,7 @@ static int print_sched_list(FILE *f, struct rtattr *list)
 		print_uint(PRINT_ANY, "interval", " interval %u", interval);
 		close_json_object();
 
-		print_string(PRINT_FP, NULL, "%s", _SL_);
+		print_nl();
 	}
 
 	close_json_array(PRINT_ANY, "");
@@ -454,7 +454,7 @@ static int taprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		print_uint(PRINT_ANY, NULL, " %u", qopt->prio_tc_map[i]);
 	close_json_array(PRINT_ANY, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	open_json_array(PRINT_ANY, "queues");
 	for (i = 0; i < qopt->num_tc; i++) {
@@ -465,7 +465,7 @@ static int taprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	}
 	close_json_array(PRINT_ANY, "");
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 
 	if (tb[TCA_TAPRIO_ATTR_SCHED_CLOCKID])
 		clockid = rta_getattr_s32(tb[TCA_TAPRIO_ATTR_SCHED_CLOCKID]);
diff --git a/tc/tc_util.c b/tc/tc_util.c
index 68938fb0..12f865cc 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -788,7 +788,7 @@ static void print_tcstats_basic_hw(struct rtattr **tbs, char *prefix)
 			   sizeof(bs)));
 
 		if (bs.bytes >= bs_hw.bytes && bs.packets >= bs_hw.packets) {
-			print_string(PRINT_FP, NULL, "%s", _SL_);
+			print_nl();
 			print_string(PRINT_FP, NULL, "%s", prefix);
 			print_lluint(PRINT_ANY, "sw_bytes",
 				     "Sent software %llu bytes",
@@ -798,7 +798,7 @@ static void print_tcstats_basic_hw(struct rtattr **tbs, char *prefix)
 		}
 	}
 
-	print_string(PRINT_FP, NULL, "%s", _SL_);
+	print_nl();
 	print_string(PRINT_FP, NULL, "%s", prefix);
 	print_lluint(PRINT_ANY, "hw_bytes", "Sent hardware %llu bytes",
 		     bs_hw.bytes);
-- 
2.26.0

Re: [PATCH iproute2 1/7] bridge: Use the same flag names in input and output

[Roopa Prabhu]

On Mon, Apr 27, 2020 at 4:51 PM Benjamin Poirier
<bpoirier@cumulusnetworks.com> wrote:
>
> Output the same names for vlan flags as the ones accepted in command input.
>
> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
> ---

Benjamin, It's a good change,  but this will break existing users ?.


>  bridge/vlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bridge/vlan.c b/bridge/vlan.c
> index 205851e4..08b19897 100644
> --- a/bridge/vlan.c
> +++ b/bridge/vlan.c
> @@ -398,10 +398,10 @@ static void print_vlan_flags(__u16 flags)
>
>         open_json_array(PRINT_JSON, "flags");
>         if (flags & BRIDGE_VLAN_INFO_PVID)
> -               print_string(PRINT_ANY, NULL, " %s", "PVID");
> +               print_string(PRINT_ANY, NULL, " %s", "pvid");
>
>         if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
> -               print_string(PRINT_ANY, NULL, " %s", "Egress Untagged");
> +               print_string(PRINT_ANY, NULL, " %s", "untagged");
>         close_json_array(PRINT_JSON, NULL);
>  }
>
> --
> 2.26.0
>

Re: [PATCH iproute2 1/7] bridge: Use the same flag names in input and output

[Benjamin Poirier]

On 2020-04-29 08:12 -0700, Roopa Prabhu wrote:
> On Mon, Apr 27, 2020 at 4:51 PM Benjamin Poirier
> <bpoirier@cumulusnetworks.com> wrote:
> >
> > Output the same names for vlan flags as the ones accepted in command input.
> >
> > Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
> > ---
> 
> Benjamin, It's a good change,  but this will break existing users ?.

Nikolay voiced the same concern. The current output looks like

ben@f3:~$ bridge vlan
port    vlan ids
br0     None
tap0     1 PVID Egress Untagged

tap1     1 PVID Egress Untagged

docker0  1 PVID Egress Untagged

ben@f3:~$

"PVID Egress Untagged" look like 3 flags to me. Anything we can do to
improve it?

Re: [PATCH iproute2 1/7] bridge: Use the same flag names in input and output

[Nikolay Aleksandrov]

On 4/30/20 3:22 AM, Benjamin Poirier wrote:
> On 2020-04-29 08:12 -0700, Roopa Prabhu wrote:
>> On Mon, Apr 27, 2020 at 4:51 PM Benjamin Poirier
>> <bpoirier@cumulusnetworks.com> wrote:
>>>
>>> Output the same names for vlan flags as the ones accepted in command input.
>>>
>>> Signed-off-by: Benjamin Poirier <bpoirier@cumulusnetworks.com>
>>> ---
>>
>> Benjamin, It's a good change,  but this will break existing users ?.
> 
> Nikolay voiced the same concern. The current output looks like
> 
> ben@f3:~$ bridge vlan
> port    vlan ids
> br0     None
> tap0     1 PVID Egress Untagged
> 
> tap1     1 PVID Egress Untagged
> 
> docker0  1 PVID Egress Untagged
> 
> ben@f3:~$
> 
> "PVID Egress Untagged" look like 3 flags to me. Anything we can do to
> improve it?
> 

Put a "," after PVID ? :-)
The bigger problem is that "Egress Untagged" is also used as a flag in the json output.
Anyone parsing that and looking at the flags would be broken. In addition
this has been described in many of HowTos and docs over the years.

I'd just drop this change.

Thanks,
  Nik

Re: [PATCH iproute2 0/7] bridge vlan output fixes

[Stephen Hemminger]

On Tue, 28 Apr 2020 08:50:44 +0900
Benjamin Poirier <bpoirier@cumulusnetworks.com> wrote:

> More fixes for `bridge vlan` and `bridge vlan tunnelshow` normal and JSON
> mode output.
> 
> Most of the changes are cosmetic except for changes to JSON format (flag
> names, no empty lists).
> 
> Benjamin Poirier (7):
>   bridge: Use the same flag names in input and output
>   bridge: Use consistent column names in vlan output
>   bridge: Fix typo
>   bridge: Fix output with empty vlan lists
>   json_print: Return number of characters printed
>   bridge: Align output columns
>   Replace open-coded instances of print_nl()
> 
>  bridge/vlan.c                            | 115 +++++++++++++++--------
>  include/json_print.h                     |  24 +++--
>  lib/json_print.c                         |  95 ++++++++++++-------
>  tc/m_action.c                            |  14 +--
>  tc/m_connmark.c                          |   4 +-
>  tc/m_ctinfo.c                            |   4 +-
>  tc/m_ife.c                               |   4 +-
>  tc/m_mpls.c                              |   2 +-
>  tc/m_nat.c                               |   4 +-
>  tc/m_sample.c                            |   4 +-
>  tc/m_skbedit.c                           |   4 +-
>  tc/m_tunnel_key.c                        |  16 ++--
>  tc/q_taprio.c                            |   8 +-
>  tc/tc_util.c                             |   4 +-
>  testsuite/tests/bridge/vlan/show.t       |  30 ++++++
>  testsuite/tests/bridge/vlan/tunnelshow.t |   2 +-
>  16 files changed, 212 insertions(+), 122 deletions(-)
>  create mode 100755 testsuite/tests/bridge/vlan/show.t
> 

Most of these look fine. Resend after the the first patch discussion
has resolved.