* Re: [PATCH] bridge: dump mcast querier state per vlan
2025-06-05 9:42 ` Ido Schimmel
@ 2025-06-06 15:11 ` Nikolay Aleksandrov
2025-06-11 12:15 ` Fabian Pfitzner
1 sibling, 0 replies; 4+ messages in thread
From: Nikolay Aleksandrov @ 2025-06-06 15:11 UTC (permalink / raw)
To: Ido Schimmel, Fabian Pfitzner; +Cc: netdev, dsahern, bridge, entwicklung
On 6/5/25 12:42, Ido Schimmel wrote:
> + Nik
>
Thanks
> On Wed, Jun 04, 2025 at 12:53:23PM +0200, Fabian Pfitzner wrote:
>> Dump the multicast querier state per vlan.
>> This commit is almost identical to [1].
>>
>> The querier state can be seen with:
>>
>> bridge -d vlan global
>>
>> The options for vlan filtering and vlan mcast snooping have to be enabled
>> in order to see the output:
>>
>> ip link set [dev] type bridge mcast_vlan_snooping 1 vlan_filtering 1
>>
>> The querier state shows the following information for IPv4 and IPv6
>> respectively:
>>
>> 1) The ip address of the current querier in the network. This could be
>> ourselves or an external querier.
>> 2) The port on which the querier was seen
>> 3) Querier timeout in seconds
>>
>> [1] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=16aa4494d7fc6543e5e92beb2ce01648b79f8fa2
>>
>> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
>> ---
>>
>> This patch is a bit redundant compared to [1]. It makes sense to put it
>> into a helper function, but i am not sure where to place this function.
>> Maybe somewhere under /lib?
>
> Not sure it's appropriate to put this in lib. Given this duplication is
> not new (see ip/iplink_bridge_slave.c and bridge/link.c, for example)
> and that the code isn't complex, I would keep it as-is.
>
+1
just do what we've already been doing, copy what you need
Cheers,
Nik
>>
>> bridge/vlan.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>>
>> diff --git a/bridge/vlan.c b/bridge/vlan.c
>> index ea4aff93..b928c653 100644
>> --- a/bridge/vlan.c
>> +++ b/bridge/vlan.c
>> @@ -892,6 +892,64 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex)
>> print_uint(PRINT_ANY, "mcast_querier", "mcast_querier %u ",
>> rta_getattr_u8(vattr));
>> }
>> + if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]) {
>> + struct rtattr *bqtb[BRIDGE_QUERIER_MAX + 1];
>> + SPRINT_BUF(other_time);
>> +
>> + parse_rtattr_nested(bqtb, BRIDGE_QUERIER_MAX, vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]);
>> + memset(other_time, 0, sizeof(other_time));
>> +
>> + open_json_object("mcast_querier_state_ipv4");
>> + if (bqtb[BRIDGE_QUERIER_IP_ADDRESS]) {
>> + print_string(PRINT_FP,
>> + NULL,
>> + "%s ",
>> + "mcast_querier_ipv4_addr");
>
> Is there a reason for this misalignment and the overly long lines?
> How about something like [1] instead (compile tested only)?
>
>> + print_color_string(PRINT_ANY,
>> + COLOR_INET,
>> + "mcast_querier_ipv4_addr",
>> + "%s ",
>> + format_host_rta(AF_INET, bqtb[BRIDGE_QUERIER_IP_ADDRESS]));
>> + }
>> + if (bqtb[BRIDGE_QUERIER_IP_PORT])
>> + print_uint(PRINT_ANY,
>> + "mcast_querier_ipv4_port",
>> + "mcast_querier_ipv4_port %u ",
>> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IP_PORT]));
>> + if (bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER])
>> + print_string(PRINT_ANY,
>> + "mcast_querier_ipv4_other_timer",
>> + "mcast_querier_ipv4_other_timer %s ",
>> + sprint_time64(
>> + rta_getattr_u64(bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]),
>> + other_time));
>> + close_json_object();
>> + open_json_object("mcast_querier_state_ipv6");
>> + if (bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]) {
>> + print_string(PRINT_FP,
>> + NULL,
>> + "%s ",
>> + "mcast_querier_ipv6_addr");
>> + print_color_string(PRINT_ANY,
>> + COLOR_INET6,
>> + "mcast_querier_ipv6_addr",
>> + "%s ",
>> + format_host_rta(AF_INET6, bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]));
>> + }
>> + if (bqtb[BRIDGE_QUERIER_IPV6_PORT])
>> + print_uint(PRINT_ANY,
>> + "mcast_querier_ipv6_port",
>> + "mcast_querier_ipv6_port %u ",
>> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IPV6_PORT]));
>> + if (bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER])
>> + print_string(PRINT_ANY,
>> + "mcast_querier_ipv6_other_timer",
>> + "mcast_querier_ipv6_other_timer %s ",
>> + sprint_time64(
>> + rta_getattr_u64(bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]),
>> + other_time));
>> + close_json_object();
>> + }
>> if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]) {
>> vattr = vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION];
>> print_uint(PRINT_ANY, "mcast_igmp_version",
>
> [1]
> diff --git a/bridge/vlan.c b/bridge/vlan.c
> index ea4aff931a22..2afdc7c72890 100644
> --- a/bridge/vlan.c
> +++ b/bridge/vlan.c
> @@ -892,6 +892,61 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex)
> print_uint(PRINT_ANY, "mcast_querier", "mcast_querier %u ",
> rta_getattr_u8(vattr));
> }
> + if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]) {
> + struct rtattr *bqtb[BRIDGE_QUERIER_MAX + 1];
> + const char *querier_ip;
> + SPRINT_BUF(other_time);
> + __u64 tval;
> +
> + parse_rtattr_nested(bqtb, BRIDGE_QUERIER_MAX,
> + vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]);
> + memset(other_time, 0, sizeof(other_time));
> +
> + open_json_object("mcast_querier_state_ipv4");
> + if (bqtb[BRIDGE_QUERIER_IP_ADDRESS]) {
> + querier_ip = format_host_rta(AF_INET,
> + bqtb[BRIDGE_QUERIER_IP_ADDRESS]);
> + print_string(PRINT_FP, NULL, "%s ",
> + "mcast_querier_ipv4_addr");
> + print_color_string(PRINT_ANY, COLOR_INET,
> + "mcast_querier_ipv4_addr", "%s ",
> + querier_ip);
> + }
> + if (bqtb[BRIDGE_QUERIER_IP_PORT])
> + print_uint(PRINT_ANY, "mcast_querier_ipv4_port",
> + "mcast_querier_ipv4_port %u ",
> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IP_PORT]));
> + if (bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]) {
> + tval = rta_getattr_u64(bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]);
> + print_string(PRINT_ANY,
> + "mcast_querier_ipv4_other_timer",
> + "mcast_querier_ipv4_other_timer %s ",
> + sprint_time64(tval, other_time));
> + }
> + close_json_object();
> + open_json_object("mcast_querier_state_ipv6");
> + if (bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]) {
> + querier_ip = format_host_rta(AF_INET6,
> + bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]);
> + print_string(PRINT_FP, NULL, "%s ",
> + "mcast_querier_ipv6_addr");
> + print_color_string(PRINT_ANY, COLOR_INET6,
> + "mcast_querier_ipv6_addr", "%s ",
> + querier_ip);
> + }
> + if (bqtb[BRIDGE_QUERIER_IPV6_PORT])
> + print_uint(PRINT_ANY, "mcast_querier_ipv6_port",
> + "mcast_querier_ipv6_port %u ",
> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IPV6_PORT]));
> + if (bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]) {
> + tval = rta_getattr_u64(bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]);
> + print_string(PRINT_ANY,
> + "mcast_querier_ipv6_other_timer",
> + "mcast_querier_ipv6_other_timer %s ",
> + sprint_time64(tval, other_time));
> + }
> + close_json_object();
> + }
> if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]) {
> vattr = vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION];
> print_uint(PRINT_ANY, "mcast_igmp_version",
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] bridge: dump mcast querier state per vlan
2025-06-05 9:42 ` Ido Schimmel
2025-06-06 15:11 ` Nikolay Aleksandrov
@ 2025-06-11 12:15 ` Fabian Pfitzner
1 sibling, 0 replies; 4+ messages in thread
From: Fabian Pfitzner @ 2025-06-11 12:15 UTC (permalink / raw)
To: Ido Schimmel, razor; +Cc: netdev, dsahern, bridge, entwicklung
On 6/5/25 11:42, Ido Schimmel wrote:
> + Nik
>
> On Wed, Jun 04, 2025 at 12:53:23PM +0200, Fabian Pfitzner wrote:
>> Dump the multicast querier state per vlan.
>> This commit is almost identical to [1].
>>
>> The querier state can be seen with:
>>
>> bridge -d vlan global
>>
>> The options for vlan filtering and vlan mcast snooping have to be enabled
>> in order to see the output:
>>
>> ip link set [dev] type bridge mcast_vlan_snooping 1 vlan_filtering 1
>>
>> The querier state shows the following information for IPv4 and IPv6
>> respectively:
>>
>> 1) The ip address of the current querier in the network. This could be
>> ourselves or an external querier.
>> 2) The port on which the querier was seen
>> 3) Querier timeout in seconds
>>
>> [1] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=16aa4494d7fc6543e5e92beb2ce01648b79f8fa2
>>
>> Signed-off-by: Fabian Pfitzner <f.pfitzner@pengutronix.de>
>> ---
>>
>> This patch is a bit redundant compared to [1]. It makes sense to put it
>> into a helper function, but i am not sure where to place this function.
>> Maybe somewhere under /lib?
> Not sure it's appropriate to put this in lib. Given this duplication is
> not new (see ip/iplink_bridge_slave.c and bridge/link.c, for example)
> and that the code isn't complex, I would keep it as-is.
>
>> bridge/vlan.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>>
>> diff --git a/bridge/vlan.c b/bridge/vlan.c
>> index ea4aff93..b928c653 100644
>> --- a/bridge/vlan.c
>> +++ b/bridge/vlan.c
>> @@ -892,6 +892,64 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex)
>> print_uint(PRINT_ANY, "mcast_querier", "mcast_querier %u ",
>> rta_getattr_u8(vattr));
>> }
>> + if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]) {
>> + struct rtattr *bqtb[BRIDGE_QUERIER_MAX + 1];
>> + SPRINT_BUF(other_time);
>> +
>> + parse_rtattr_nested(bqtb, BRIDGE_QUERIER_MAX, vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]);
>> + memset(other_time, 0, sizeof(other_time));
>> +
>> + open_json_object("mcast_querier_state_ipv4");
>> + if (bqtb[BRIDGE_QUERIER_IP_ADDRESS]) {
>> + print_string(PRINT_FP,
>> + NULL,
>> + "%s ",
>> + "mcast_querier_ipv4_addr");
> Is there a reason for this misalignment and the overly long lines?
> How about something like [1] instead (compile tested only)?
Thanks. I tested your changes successfully.
Made a v2:
https://lore.kernel.org/netdev/20250611121151.1660231-1-f.pfitzner@pengutronix.de/
>
>> + print_color_string(PRINT_ANY,
>> + COLOR_INET,
>> + "mcast_querier_ipv4_addr",
>> + "%s ",
>> + format_host_rta(AF_INET, bqtb[BRIDGE_QUERIER_IP_ADDRESS]));
>> + }
>> + if (bqtb[BRIDGE_QUERIER_IP_PORT])
>> + print_uint(PRINT_ANY,
>> + "mcast_querier_ipv4_port",
>> + "mcast_querier_ipv4_port %u ",
>> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IP_PORT]));
>> + if (bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER])
>> + print_string(PRINT_ANY,
>> + "mcast_querier_ipv4_other_timer",
>> + "mcast_querier_ipv4_other_timer %s ",
>> + sprint_time64(
>> + rta_getattr_u64(bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]),
>> + other_time));
>> + close_json_object();
>> + open_json_object("mcast_querier_state_ipv6");
>> + if (bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]) {
>> + print_string(PRINT_FP,
>> + NULL,
>> + "%s ",
>> + "mcast_querier_ipv6_addr");
>> + print_color_string(PRINT_ANY,
>> + COLOR_INET6,
>> + "mcast_querier_ipv6_addr",
>> + "%s ",
>> + format_host_rta(AF_INET6, bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]));
>> + }
>> + if (bqtb[BRIDGE_QUERIER_IPV6_PORT])
>> + print_uint(PRINT_ANY,
>> + "mcast_querier_ipv6_port",
>> + "mcast_querier_ipv6_port %u ",
>> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IPV6_PORT]));
>> + if (bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER])
>> + print_string(PRINT_ANY,
>> + "mcast_querier_ipv6_other_timer",
>> + "mcast_querier_ipv6_other_timer %s ",
>> + sprint_time64(
>> + rta_getattr_u64(bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]),
>> + other_time));
>> + close_json_object();
>> + }
>> if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]) {
>> vattr = vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION];
>> print_uint(PRINT_ANY, "mcast_igmp_version",
> [1]
> diff --git a/bridge/vlan.c b/bridge/vlan.c
> index ea4aff931a22..2afdc7c72890 100644
> --- a/bridge/vlan.c
> +++ b/bridge/vlan.c
> @@ -892,6 +892,61 @@ static void print_vlan_global_opts(struct rtattr *a, int ifindex)
> print_uint(PRINT_ANY, "mcast_querier", "mcast_querier %u ",
> rta_getattr_u8(vattr));
> }
> + if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]) {
> + struct rtattr *bqtb[BRIDGE_QUERIER_MAX + 1];
> + const char *querier_ip;
> + SPRINT_BUF(other_time);
> + __u64 tval;
> +
> + parse_rtattr_nested(bqtb, BRIDGE_QUERIER_MAX,
> + vtb[BRIDGE_VLANDB_GOPTS_MCAST_QUERIER_STATE]);
> + memset(other_time, 0, sizeof(other_time));
> +
> + open_json_object("mcast_querier_state_ipv4");
> + if (bqtb[BRIDGE_QUERIER_IP_ADDRESS]) {
> + querier_ip = format_host_rta(AF_INET,
> + bqtb[BRIDGE_QUERIER_IP_ADDRESS]);
> + print_string(PRINT_FP, NULL, "%s ",
> + "mcast_querier_ipv4_addr");
> + print_color_string(PRINT_ANY, COLOR_INET,
> + "mcast_querier_ipv4_addr", "%s ",
> + querier_ip);
> + }
> + if (bqtb[BRIDGE_QUERIER_IP_PORT])
> + print_uint(PRINT_ANY, "mcast_querier_ipv4_port",
> + "mcast_querier_ipv4_port %u ",
> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IP_PORT]));
> + if (bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]) {
> + tval = rta_getattr_u64(bqtb[BRIDGE_QUERIER_IP_OTHER_TIMER]);
> + print_string(PRINT_ANY,
> + "mcast_querier_ipv4_other_timer",
> + "mcast_querier_ipv4_other_timer %s ",
> + sprint_time64(tval, other_time));
> + }
> + close_json_object();
> + open_json_object("mcast_querier_state_ipv6");
> + if (bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]) {
> + querier_ip = format_host_rta(AF_INET6,
> + bqtb[BRIDGE_QUERIER_IPV6_ADDRESS]);
> + print_string(PRINT_FP, NULL, "%s ",
> + "mcast_querier_ipv6_addr");
> + print_color_string(PRINT_ANY, COLOR_INET6,
> + "mcast_querier_ipv6_addr", "%s ",
> + querier_ip);
> + }
> + if (bqtb[BRIDGE_QUERIER_IPV6_PORT])
> + print_uint(PRINT_ANY, "mcast_querier_ipv6_port",
> + "mcast_querier_ipv6_port %u ",
> + rta_getattr_u32(bqtb[BRIDGE_QUERIER_IPV6_PORT]));
> + if (bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]) {
> + tval = rta_getattr_u64(bqtb[BRIDGE_QUERIER_IPV6_OTHER_TIMER]);
> + print_string(PRINT_ANY,
> + "mcast_querier_ipv6_other_timer",
> + "mcast_querier_ipv6_other_timer %s ",
> + sprint_time64(tval, other_time));
> + }
> + close_json_object();
> + }
> if (vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION]) {
> vattr = vtb[BRIDGE_VLANDB_GOPTS_MCAST_IGMP_VERSION];
> print_uint(PRINT_ANY, "mcast_igmp_version",
>
--
Pengutronix e.K. | Fabian Pfitzner |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
^ permalink raw reply [flat|nested] 4+ messages in thread