ip: Support bridge VLAN stats in `ip stats'

ip/ipstats.c +0 -0

@@ -600,15 +601,14 @@ int ipstats_stat_desc_show_xstats(struct ipstats_stat_show_attrs *attrs,
 	if (at == NULL)
 		return err;
 

ip/ipstats.c +5 -0

+		if (i->rta_type == xdesc->inner_at) {
+			print_nl();
+			xdesc->show_cb(i);
+		}
 	}
+
 	return 0;
 }
 

Signature · 1 line

2.49.0

Quoted 4 lines · Petr Machata

ip/ipstats.c +0 -0

@@ -600,15 +601,14 @@ int ipstats_stat_desc_show_xstats(struct ipstats_stat_show_attrs *attrs,
 	if (at == NULL)
 		return err;
 

ip/ipstats.c int ipstats_stat_desc_show_xstats(struct ipstats_stat_show_attrs *attrs, · line 605

Quoted 10 lines

ip/ipstats.c +2 -8

-	tb = alloca(sizeof(*tb) * (xdesc->inner_max + 1));
-	err = parse_rtattr_nested(tb, xdesc->inner_max, at);
-	if (err != 0)
-		return err;
-
-	if (tb[xdesc->inner_at] != NULL) {
-		print_nl();
-		xdesc->show_cb(tb[xdesc->inner_at]);
+	rem = RTA_PAYLOAD(at);
+	for (i = RTA_DATA(at); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {

Use rtattr_for_each_nested() ?

After the previous patch, this field is not read anymore. Drop it.

Signed-off-by Petr Machata <[email protected]>

ip stats' displays a range of bridge_slave-related statistics, but not the VLAN stats. bridge vlan' actually has code to show these. Extract the code to libutil so that it can be reused between the bridge and ip stats tools.

Rename them reasonably so as not to litter the global namespace.

Signed-off-by Petr Machata <[email protected]>

---
  bridge/vlan.c    | 50 +++++-------------------------------------------
  include/bridge.h | 11 +++++++++++
  lib/Makefile     |  3 ++-
  lib/bridge.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 65 insertions(+), 46 deletions(-)

create mode 100644 include/bridge.h
create mode 100644 lib/bridge.c

bridge/vlan.c +5 -45

index ea4aff93..14b8475d 100644
@@ -15,6 +15,7 @@
  #include "json_print.h"
  #include "libnetlink.h"
  #include "br_common.h"
+#include "bridge.h"
  #include "utils.h"
 
  static unsigned int filter_index, filter_vlan;

@@ -705,47 +706,6 @@ static int print_vlan(struct nlmsghdr *n, void *arg)
  	return 0;
  }
 
-static void print_vlan_flags(__u16 flags)
-{
-	if (flags == 0)
-		return;
-
-	open_json_array(PRINT_JSON, "flags");
-	if (flags & BRIDGE_VLAN_INFO_PVID)
-		print_string(PRINT_ANY, NULL, " %s", "PVID");
-
-	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
-		print_string(PRINT_ANY, NULL, " %s", "Egress Untagged");
-	close_json_array(PRINT_JSON, NULL);
-}
-
-static void __print_one_vlan_stats(const struct bridge_vlan_xstats *vstats)
-{
-	print_string(PRINT_FP, NULL, "%-" textify(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_string(PRINT_FP, NULL, "%-" textify(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);
-}
-
-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_vlan_flags(vstats->flags);
-	print_nl();
-	__print_one_vlan_stats(vstats);
-
-	close_json_object();
-}
-
  static void print_vlan_stats_attr(struct rtattr *attr, int ifindex)
  {
  	struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];

@@ -783,7 +743,7 @@ static void print_vlan_stats_attr(struct rtattr *attr, int ifindex)
  			print_string(PRINT_FP, NULL,
  				     "%-" textify(IFNAMSIZ) "s  ", "");
  		}
-		print_one_vlan_stats(vstats);
+		bridge_print_vlan_stats(vstats);
  	}
 
  	/* vlan_port is opened only if there are any vlan stats */

@@ -1025,7 +985,7 @@ static void print_vlan_opts(struct rtattr *a, int ifindex)
  		print_string(PRINT_FP, NULL, "%-" textify(IFNAMSIZ) "s  ", "");
  	}
  	print_range("vlan", vinfo->vid, vrange);
-	print_vlan_flags(vinfo->flags);
+	bridge_print_vlan_flags(vinfo->flags);
  	print_nl();
  	print_string(PRINT_FP, NULL, "%-" textify(IFNAMSIZ) "s    ", "");
  	print_stp_state(state);

@@ -1051,7 +1011,7 @@ static void print_vlan_opts(struct rtattr *a, int ifindex)
  	}
  	print_nl();
  	if (show_stats)
-		__print_one_vlan_stats(&vstats);
+		bridge_print_vlan_stats_only(&vstats);
  	close_json_object();
  }
 

@@ -1334,7 +1294,7 @@ static void print_vlan_info(struct rtattr *tb, int ifindex)
  		open_json_object(NULL);
  		print_range("vlan", last_vid_start, vinfo->vid);
 
-		print_vlan_flags(vinfo->flags);
+		bridge_print_vlan_flags(vinfo->flags);
  		close_json_object();
  		print_nl();
  	}

include/bridge.h new file +11 -0

new file mode 100644
index 00000000..8bcd1e38
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __BRIDGE_H__
+#define __BRIDGE_H__ 1
+
+#include <linux/if_bridge.h>
+
+void bridge_print_vlan_flags(__u16 flags);
+void bridge_print_vlan_stats_only(const struct bridge_vlan_xstats *vstats);
+void bridge_print_vlan_stats(const struct bridge_vlan_xstats *vstats);
+
+#endif /* __BRIDGE_H__ */

lib/Makefile +2 -1

index aa7bbd2e..0ba62942 100644
@@ -5,7 +5,8 @@ CFLAGS += -fPIC
 
  UTILOBJ = utils.o utils_math.o rt_names.o ll_map.o ll_types.o ll_proto.o ll_addr.o \
  	inet_proto.o namespace.o json_writer.o json_print.o json_print_math.o \
-	names.o color.o bpf_legacy.o bpf_glue.o exec.o fs.o cg_map.o ppp_proto.o
+	names.o color.o bpf_legacy.o bpf_glue.o exec.o fs.o cg_map.o \
+	ppp_proto.o bridge.o
 
  ifeq ($(HAVE_ELF),y)
  ifeq ($(HAVE_LIBBPF),y)

lib/bridge.c new file +47 -0

new file mode 100644
index 00000000..0a46b6a2
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <net/if.h>
+
+#include "bridge.h"
+#include "utils.h"
+
+void bridge_print_vlan_flags(__u16 flags)
+{
+	if (flags == 0)
+		return;
+
+	open_json_array(PRINT_JSON, "flags");
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		print_string(PRINT_ANY, NULL, " %s", "PVID");
+
+	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		print_string(PRINT_ANY, NULL, " %s", "Egress Untagged");
+	close_json_array(PRINT_JSON, NULL);
+}
+
+void bridge_print_vlan_stats_only(const struct bridge_vlan_xstats *vstats)
+{
+	print_string(PRINT_FP, NULL, "%-" textify(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_string(PRINT_FP, NULL, "%-" textify(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);
+}
+
+void bridge_print_vlan_stats(const struct bridge_vlan_xstats *vstats)
+{
+	open_json_object(NULL);
+
+	print_hu(PRINT_ANY, "vid", "%hu", vstats->vid);
+	bridge_print_vlan_flags(vstats->flags);
+	print_nl();
+	bridge_print_vlan_stats_only(vstats);
+
+	close_json_object();
+}

Quoted 6 lines · Petr Machata

ip stats' displays a range of bridge_slave-related statistics, but not the VLAN stats. bridge vlan' actually has code to show these. Extract the code to libutil so that it can be reused between the bridge and ip stats tools.
Rename them reasonably so as not to litter the global namespace.

Signed-off-by Petr Machata <[email protected]>

Quoted 9 lines

---
  bridge/vlan.c    | 50 +++++-------------------------------------------
  include/bridge.h | 11 +++++++++++
  lib/Makefile     |  3 ++-
  lib/bridge.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 65 insertions(+), 46 deletions(-)

create mode 100644 include/bridge.h
create mode 100644 lib/bridge.c

lol, Ido didn't we just have a discussion in another thread about adding a bridge lib? :-)

https://www.spinics.net/lists/netdev/msg1096403.html

I'm not strictly against adding a lib, I think this approach might actually save us from the tools going out of sync and we can avoid repeating to people they have to update both places when they forget.

ip stats' displays a range of bridge_slave-related statistics, but not the VLAN stats. bridge vlan' actually has code to show these. Extract the code to libutil so that it can be reused between the bridge and ip stats tools.

Rename them reasonably so as not to litter the global namespace.

Signed-off-by Petr Machata <[email protected]>

---
 bridge/vlan.c    | 50 +++++-------------------------------------------
 include/bridge.h | 11 +++++++++++
 lib/Makefile     |  3 ++-
 lib/bridge.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 46 deletions(-)
 create mode 100644 include/bridge.h
 create mode 100644 lib/bridge.c

Quoted 8 lines · Petr Machata

ip stats' displays a range of bridge_slave-related statistics, but not the VLAN stats. bridge vlan' actually has code to show these. Extract the code to libutil so that it can be reused between the bridge and ip stats tools.

Rename them reasonably so as not to litter the global namespace.

Signed-off-by Petr Machata <[email protected]>

Quoted 8 lines

---
 bridge/vlan.c    | 50 +++++-------------------------------------------
 include/bridge.h | 11 +++++++++++
 lib/Makefile     |  3 ++-
 lib/bridge.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 46 deletions(-)
 create mode 100644 include/bridge.h
 create mode 100644 lib/bridge.c

Add file entry to MAINTAINERS?

ip/iplink_bridge.c +4 -0

+	.link_type_at = LINK_XSTATS_TYPE_BRIDGE,
+	.inner_at = BRIDGE_XSTATS_VLAN,
+	.show_cb = &bridge_print_stats_vlan,
+};

ip/iplink_bridge.c ipstats_stat_desc_xstats_slave_bridge_mcast = { · line 1151

Quoted 9 lines · Petr Machata

ip/iplink_bridge.c +9 -0

+static const struct ipstats_stat_desc_xstats
+ipstats_stat_desc_xstats_slave_bridge_vlan = {
+	.desc = {
+		.name = "vlan",
+		.kind = IPSTATS_STAT_DESC_KIND_LEAF,
+		.show = &bridge_stat_desc_show_xstats,
+		.pack = &ipstats_stat_desc_pack_xstats,
+	},
+	.xstats_at = IFLA_STATS_LINK_XSTATS_SLAVE,

This will only show VLAN stats for bridge ports, but they also exist for the bridge itself inside the IFLA_STATS_LINK_XSTATS nest