[PATCH iproute2 0/3] Process IFLA_BRIDGE_VLAN_INFO tlv

[PATCH iproute2 0/3] Process IFLA_BRIDGE_VLAN_INFO tlv

[Roman Mashak]

Process IFLA_BRIDGE_VLAN_INFO attribute nested in IFLA_AF_SPEC, which
contains bridge vlan table per port passed in link events.

Roman Mashak (3):
  bridge: isolate vlans  parsing code in a separate API
  bridge: dump vlan table information for link
  bridge: request vlans along with link information

 bridge/br_common.h |   1 +
 bridge/link.c      |  23 +++++++--
 bridge/vlan.c      | 145 +++++++++++++++++++++++++++--------------------------
 3 files changed, 96 insertions(+), 73 deletions(-)

-- 
1.9.1

[PATCH iproute2 1/3] bridge: isolate vlans parsing code in a separate API

[Roman Mashak]

IFLA_BRIDGE_VLAN_INFO parsing logic will be used in link and vlan
processing code, so it makes sense to move it in the separate function.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 bridge/br_common.h |   1 +
 bridge/vlan.c      | 145 +++++++++++++++++++++++++++--------------------------
 2 files changed, 76 insertions(+), 70 deletions(-)

diff --git a/bridge/br_common.h b/bridge/br_common.h
index c649e7d..01447dd 100644
--- a/bridge/br_common.h
+++ b/bridge/br_common.h
@@ -4,6 +4,7 @@
 #define MDB_RTR_RTA(r) \
 		((struct rtattr *)(((char *)(r)) + RTA_ALIGN(sizeof(__u32))))
 
+extern void print_vlan_info(FILE *fp, struct rtattr *tb, int ifindex);
 extern int print_linkinfo(const struct sockaddr_nl *who,
 			  struct nlmsghdr *n,
 			  void *arg);
diff --git a/bridge/vlan.c b/bridge/vlan.c
index ebcdace..fc361ae 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -189,7 +189,6 @@ static int print_vlan(const struct sockaddr_nl *who,
 	struct ifinfomsg *ifm = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
 	struct rtattr *tb[IFLA_MAX+1];
-	bool vlan_flags = false;
 
 	if (n->nlmsg_type != RTM_NEWLINK) {
 		fprintf(stderr, "Not RTM_NEWLINK: %08x %08x %08x\n",
@@ -218,75 +217,7 @@ static int print_vlan(const struct sockaddr_nl *who,
 				ll_index_to_name(ifm->ifi_index));
 		return 0;
 	} else {
-		struct rtattr *i, *list = tb[IFLA_AF_SPEC];
-		int rem = RTA_PAYLOAD(list);
-		__u16 last_vid_start = 0;
-
-		if (!filter_vlan)
-			print_vlan_port(fp, ifm->ifi_index);
-
-		for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
-			struct bridge_vlan_info *vinfo;
-			int vcheck_ret;
-
-			if (i->rta_type != IFLA_BRIDGE_VLAN_INFO)
-				continue;
-
-			vinfo = RTA_DATA(i);
-
-			if (!(vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END))
-				last_vid_start = vinfo->vid;
-			vcheck_ret = filter_vlan_check(vinfo);
-			if (vcheck_ret == -1)
-				break;
-			else if (vcheck_ret == 0)
-				continue;
-
-			if (filter_vlan)
-				print_vlan_port(fp, ifm->ifi_index);
-			if (jw_global) {
-				jsonw_start_object(jw_global);
-				jsonw_uint_field(jw_global, "vlan",
-						 last_vid_start);
-				if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
-					continue;
-			} else {
-				fprintf(fp, "\t %hu", last_vid_start);
-			}
-			if (last_vid_start != vinfo->vid) {
-				if (jw_global)
-					jsonw_uint_field(jw_global, "vlanEnd",
-							 vinfo->vid);
-				else
-					fprintf(fp, "-%hu", vinfo->vid);
-			}
-			if (vinfo->flags & BRIDGE_VLAN_INFO_PVID) {
-				if (jw_global) {
-					start_json_vlan_flags_array(&vlan_flags);
-					jsonw_string(jw_global, "PVID");
-				} else {
-					fprintf(fp, " PVID");
-				}
-			}
-			if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED) {
-				if (jw_global) {
-					start_json_vlan_flags_array(&vlan_flags);
-					jsonw_string(jw_global,
-						     "Egress Untagged");
-				} else {
-					fprintf(fp, " Egress Untagged");
-				}
-			}
-			if (jw_global && vlan_flags) {
-				jsonw_end_array(jw_global);
-				vlan_flags = false;
-			}
-
-			if (jw_global)
-				jsonw_end_object(jw_global);
-			else
-				fprintf(fp, "\n");
-		}
+		print_vlan_info(fp, tb[IFLA_AF_SPEC], ifm->ifi_index);
 	}
 	if (!filter_vlan) {
 		if (jw_global)
@@ -470,6 +401,80 @@ static int vlan_show(int argc, char **argv)
 	return 0;
 }
 
+void print_vlan_info(FILE *fp, struct rtattr *tb, int ifindex)
+{
+	struct rtattr *i, *list = tb;
+	int rem = RTA_PAYLOAD(list);
+	__u16 last_vid_start = 0;
+	bool vlan_flags = false;
+
+	if (!filter_vlan)
+		print_vlan_port(fp, ifindex);
+
+	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
+		struct bridge_vlan_info *vinfo;
+		int vcheck_ret;
+
+		if (i->rta_type != IFLA_BRIDGE_VLAN_INFO)
+			continue;
+
+		vinfo = RTA_DATA(i);
+
+		if (!(vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END))
+			last_vid_start = vinfo->vid;
+		vcheck_ret = filter_vlan_check(vinfo);
+		if (vcheck_ret == -1)
+			break;
+		else if (vcheck_ret == 0)
+			continue;
+
+		if (filter_vlan)
+			print_vlan_port(fp, ifindex);
+		if (jw_global) {
+			jsonw_start_object(jw_global);
+			jsonw_uint_field(jw_global, "vlan",
+					 last_vid_start);
+			if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
+				continue;
+		} else {
+			fprintf(fp, "\t %hu", last_vid_start);
+		}
+		if (last_vid_start != vinfo->vid) {
+			if (jw_global)
+				jsonw_uint_field(jw_global, "vlanEnd",
+						 vinfo->vid);
+			else
+				fprintf(fp, "-%hu", vinfo->vid);
+		}
+		if (vinfo->flags & BRIDGE_VLAN_INFO_PVID) {
+			if (jw_global) {
+				start_json_vlan_flags_array(&vlan_flags);
+				jsonw_string(jw_global, "PVID");
+			} else {
+				fprintf(fp, " PVID");
+			}
+		}
+		if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED) {
+			if (jw_global) {
+				start_json_vlan_flags_array(&vlan_flags);
+				jsonw_string(jw_global,
+					     "Egress Untagged");
+			} else {
+				fprintf(fp, " Egress Untagged");
+			}
+		}
+		if (jw_global && vlan_flags) {
+			jsonw_end_array(jw_global);
+			vlan_flags = false;
+		}
+
+		if (jw_global)
+			jsonw_end_object(jw_global);
+		else
+			fprintf(fp, "\n");
+	}
+}
+
 int do_vlan(int argc, char **argv)
 {
 	ll_init_map(&rth);
-- 
1.9.1

[PATCH iproute2 2/3] bridge: dump vlan table information for link

[Roman Mashak]

Kernel also reports vlans a port is member of, so print it. Since vlan
table can be quite large, dump it only when detailed information is
requested.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 bridge/link.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/bridge/link.c b/bridge/link.c
index 93472ad..60200f1 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -213,6 +213,13 @@ int print_linkinfo(const struct sockaddr_nl *who,
 
 		if (aftb[IFLA_BRIDGE_MODE])
 			print_hwmode(fp, rta_getattr_u16(aftb[IFLA_BRIDGE_MODE]));
+		if (show_details) {
+			if (aftb[IFLA_BRIDGE_VLAN_INFO]) {
+				fprintf(fp, "\n");
+				print_vlan_info(fp, tb[IFLA_AF_SPEC],
+						ifi->ifi_index);
+			}
+		}
 	}
 
 	fprintf(fp, "\n");
-- 
1.9.1

[PATCH iproute2 3/3] bridge: request vlans along with link information

[Roman Mashak]

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 bridge/link.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/bridge/link.c b/bridge/link.c
index 60200f1..9e4206f 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
 		}
 	}
 
-	if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
-		perror("Cannon send dump request");
-		exit(1);
+	if (show_details) {
+		if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
+					     (compress_vlans ?
+					      RTEXT_FILTER_BRVLAN_COMPRESSED :
+					      RTEXT_FILTER_BRVLAN)) < 0) {
+			perror("Cannon send dump request");
+			exit(1);
+		}
+	} else {
+		if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
+			perror("Cannon send dump request");
+			exit(1);
+		}
 	}
 
 	if (rtnl_dump_filter(&rth, print_linkinfo, stdout) < 0) {
-- 
1.9.1

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Roopa Prabhu]

On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> ---
>  bridge/link.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/bridge/link.c b/bridge/link.c
> index 60200f1..9e4206f 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>                 }
>         }
>
> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
> -               perror("Cannon send dump request");
> -               exit(1);
> +       if (show_details) {
> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
> +                                            (compress_vlans ?
> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
> +                       perror("Cannon send dump request");
> +                       exit(1);
> +               }

vlan information is already available with `bridge vlan show`. any
specific reason why you want it in
the link dump output ?

The problem is this might just make the link dump larger and also add
too much clutter into the regular link dump output. iproute2 detailed
dump is already a bit hard to interpret. And without compression by
default, vlan info can just take over the link dump output. It will be
hard to look for other link attributes after that :). We deploy with
thousands of vlans and without compression even bridge vlan default
output is already hard to interpret.

Can you please paste a sample default detailed output with your patch
with a few hundred vlans ?

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Jamal Hadi Salim]

On 17-09-09 12:24 PM, Roopa Prabhu wrote:
> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>> ---
>>   bridge/link.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/bridge/link.c b/bridge/link.c
>> index 60200f1..9e4206f 100644
>> --- a/bridge/link.c
>> +++ b/bridge/link.c
>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>                  }
>>          }
>>
>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>> -               perror("Cannon send dump request");
>> -               exit(1);
>> +       if (show_details) {
>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>> +                                            (compress_vlans ?
>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>> +                       perror("Cannon send dump request");
>> +                       exit(1);
>> +               }
> 
> vlan information is already available with `bridge vlan show`. any
> specific reason why you want it in
> the link dump output ?
> 
 >
> The problem is this might just make the link dump larger and also add
> too much clutter into the regular link dump output. iproute2 detailed
> dump is already a bit hard to interpret. And without compression by
> default, vlan info can just take over the link dump output. It will be
> hard to look for other link attributes after that :). We deploy with
> thousands of vlans and without compression even bridge vlan default
> output is already hard to interpret.
>

Agree we should be turning on this stuff by default. i.e default stays
compressed; otherwise it a huge dump.

Having said that there is a lot of mess with this stuff.
The bridge link events _already send this IFLA_AF_SPCE info_
so not much choice  there but to print it.
At minimal we need that part because unfortunately there is no
vlanfilter event in existence which will send us summaries of just
vlans added to a port i.e both use XXXLINK.
In general, the XXXLINK interface with these master devices (bridge,
bond etc) continues to get messy. Recently started seeing events with
devices claiming to be of KIND_slave etc on the bridge and bond devices;
yet at the same time my wireless card events are also showing up on the
bridge link even though it is not enslaved there..

cheers,
jamal

> Can you please paste a sample default detailed output with your patch
> with a few hundred vlans ?
> 

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Nikolay Aleksandrov]

On 09/09/17 20:23, Jamal Hadi Salim wrote:
> On 17-09-09 12:24 PM, Roopa Prabhu wrote:
>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>> ---
>>>   bridge/link.c | 16 +++++++++++++---
>>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bridge/link.c b/bridge/link.c
>>> index 60200f1..9e4206f 100644
>>> --- a/bridge/link.c
>>> +++ b/bridge/link.c
>>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>>                  }
>>>          }
>>>
>>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>>> -               perror("Cannon send dump request");
>>> -               exit(1);
>>> +       if (show_details) {
>>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>>> +                                            (compress_vlans ?
>>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>>> +                       perror("Cannon send dump request");
>>> +                       exit(1);
>>> +               }
>>
>> vlan information is already available with `bridge vlan show`. any
>> specific reason why you want it in
>> the link dump output ?
>>
>>
>> The problem is this might just make the link dump larger and also add
>> too much clutter into the regular link dump output. iproute2 detailed
>> dump is already a bit hard to interpret. And without compression by
>> default, vlan info can just take over the link dump output. It will be
>> hard to look for other link attributes after that :). We deploy with
>> thousands of vlans and without compression even bridge vlan default
>> output is already hard to interpret.
>>
> 
> Agree we should be turning on this stuff by default. i.e default stays
> compressed; otherwise it a huge dump.

I think this should be dumped with the getlink request only on some additional
flag. The getlink does not include these by default.

> 
> Having said that there is a lot of mess with this stuff.
> The bridge link events _already send this IFLA_AF_SPCE info_
> so not much choice  there but to print it.

Right, on NEWLINK per port notification you'll get the compressed vlan info.

> At minimal we need that part because unfortunately there is no
> vlanfilter event in existence which will send us summaries of just
> vlans added to a port i.e both use XXXLINK.

But let's either add a new flag or use -compressvlans to print it when monitoring/showing
link otherwise people who are monitoring only the port flags will start getting lists
with vlans. Even compressed these can still be quite long and confusing, especially
when monitoring.

> In general, the XXXLINK interface with these master devices (bridge,
> bond etc) continues to get messy. Recently started seeing events with
> devices claiming to be of KIND_slave etc on the bridge and bond devices;
> yet at the same time my wireless card events are also showing up on the
> bridge link even though it is not enslaved there..
> 
> cheers,
> jamal
> 
>> Can you please paste a sample default detailed output with your patch
>> with a few hundred vlans ?
>>
> 

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Roopa Prabhu]

On Sat, Sep 9, 2017 at 11:15 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 09/09/17 20:23, Jamal Hadi Salim wrote:
>> On 17-09-09 12:24 PM, Roopa Prabhu wrote:
>>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>>> ---
>

[snip]

>>>
>>> vlan information is already available with `bridge vlan show`. any
>>> specific reason why you want it in
>>> the link dump output ?
>>>
>>>
>>> The problem is this might just make the link dump larger and also add
>>> too much clutter into the regular link dump output. iproute2 detailed
>>> dump is already a bit hard to interpret. And without compression by
>>> default, vlan info can just take over the link dump output. It will be
>>> hard to look for other link attributes after that :). We deploy with
>>> thousands of vlans and without compression even bridge vlan default
>>> output is already hard to interpret.
>>>
>>
>> Agree we should be turning on this stuff by default. i.e default stays
>> compressed; otherwise it a huge dump.
>
> I think this should be dumped with the getlink request only on some additional
> flag. The getlink does not include these by default.
>
>>
>> Having said that there is a lot of mess with this stuff.
>> The bridge link events _already send this IFLA_AF_SPCE info_
>> so not much choice  there but to print it.
>
> Right, on NEWLINK per port notification you'll get the compressed vlan info.
>
>> At minimal we need that part because unfortunately there is no
>> vlanfilter event in existence which will send us summaries of just
>> vlans added to a port i.e both use XXXLINK.
>
> But let's either add a new flag or use -compressvlans to print it when monitoring/showing
> link otherwise people who are monitoring only the port flags will start getting lists
> with vlans. Even compressed these can still be quite long and confusing, especially
> when monitoring.

yes agree. It will add too much clutter to the monitor output too.

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Roopa Prabhu]

On Sat, Sep 9, 2017 at 10:23 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 17-09-09 12:24 PM, Roopa Prabhu wrote:
>>
>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>
>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>> ---
>>>   bridge/link.c | 16 +++++++++++++---
>>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bridge/link.c b/bridge/link.c
>>> index 60200f1..9e4206f 100644
>>> --- a/bridge/link.c
>>> +++ b/bridge/link.c
>>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>>                  }
>>>          }
>>>
>>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>>> -               perror("Cannon send dump request");
>>> -               exit(1);
>>> +       if (show_details) {
>>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE,
>>> RTM_GETLINK,
>>> +                                            (compress_vlans ?
>>> +
>>> RTEXT_FILTER_BRVLAN_COMPRESSED :
>>> +                                             RTEXT_FILTER_BRVLAN)) < 0)
>>> {
>>> +                       perror("Cannon send dump request");
>>> +                       exit(1);
>>> +               }
>>
>>
>> vlan information is already available with `bridge vlan show`. any
>> specific reason why you want it in
>> the link dump output ?
>>
>>
>>
>> The problem is this might just make the link dump larger and also add
>> too much clutter into the regular link dump output. iproute2 detailed
>> dump is already a bit hard to interpret. And without compression by
>> default, vlan info can just take over the link dump output. It will be
>> hard to look for other link attributes after that :). We deploy with
>> thousands of vlans and without compression even bridge vlan default
>> output is already hard to interpret.
>>
>
> Agree we should be turning on this stuff by default. i.e default stays
> compressed; otherwise it a huge dump.
>
> Having said that there is a lot of mess with this stuff.
> The bridge link events _already send this IFLA_AF_SPCE info_
> so not much choice  there but to print it.
> At minimal we need that part because unfortunately there is no
> vlanfilter event in existence which will send us summaries of just
> vlans added to a port i.e both use XXXLINK.
> In general, the XXXLINK interface with these master devices (bridge,
> bond etc) continues to get messy. Recently started seeing events with
> devices claiming to be of KIND_slave etc on the bridge and bond devices;

yes, its a bit messy and redundant.

> yet at the same time my wireless card events are also showing up on the
> bridge link even though it is not enslaved there..

that is strange. It should not.

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Nikolay Aleksandrov]

On 09/09/17 20:23, Jamal Hadi Salim wrote:
> On 17-09-09 12:24 PM, Roopa Prabhu wrote:
>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
[snip]
> 
> Agree we should be turning on this stuff by default. i.e default stays
> compressed; otherwise it a huge dump.
> 
> Having said that there is a lot of mess with this stuff.
> The bridge link events _already send this IFLA_AF_SPCE info_
> so not much choice  there but to print it.
> At minimal we need that part because unfortunately there is no
> vlanfilter event in existence which will send us summaries of just
> vlans added to a port i.e both use XXXLINK.
> In general, the XXXLINK interface with these master devices (bridge,
> bond etc) continues to get messy. Recently started seeing events with
> devices claiming to be of KIND_slave etc on the bridge and bond devices;
> yet at the same time my wireless card events are also showing up on the
> bridge link even though it is not enslaved there..

Out of curiousity about this one, do you see the wifi card when doing bridge monitor ?
Or are you specifically watching AF_BRIDGE events, because bridge monitor link also can
include AF_UNSPEC ?

> 
> cheers,
> jamal
> 

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Roman Mashak]

Roopa Prabhu <roopa@cumulusnetworks.com> writes:

> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>> ---
>>  bridge/link.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/bridge/link.c b/bridge/link.c
>> index 60200f1..9e4206f 100644
>> --- a/bridge/link.c
>> +++ b/bridge/link.c
>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>                 }
>>         }
>>
>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>> -               perror("Cannon send dump request");
>> -               exit(1);
>> +       if (show_details) {
>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>> +                                            (compress_vlans ?
>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>> +                       perror("Cannon send dump request");
>> +                       exit(1);
>> +               }
>
> vlan information is already available with `bridge vlan show`. any
> specific reason why you want it in
> the link dump output ?

Since VLAN info is already in link messages, there is no other option but
dump it in monitor or "bridge link show". Yes, the output may be lengthy
and hard to digest for a human, that is why I put it under "show_details"
kludge - if one is requesting details, he/she should be prepared for
large volumes of data to be shown :)

As Nikolay suggested, it'll make sense to request compressed
output by default, this will be addresses in v2 patch.

[...]

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Nikolay Aleksandrov]

On 10/09/17 16:38, Roman Mashak wrote:
> Roopa Prabhu <roopa@cumulusnetworks.com> writes:
> 
>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>> ---
>>>  bridge/link.c | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bridge/link.c b/bridge/link.c
>>> index 60200f1..9e4206f 100644
>>> --- a/bridge/link.c
>>> +++ b/bridge/link.c
>>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>>                 }
>>>         }
>>>
>>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>>> -               perror("Cannon send dump request");
>>> -               exit(1);
>>> +       if (show_details) {
>>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>>> +                                            (compress_vlans ?
>>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>>> +                       perror("Cannon send dump request");
>>> +                       exit(1);
>>> +               }
>>
>> vlan information is already available with `bridge vlan show`. any
>> specific reason why you want it in
>> the link dump output ?
> 
> Since VLAN info is already in link messages, there is no other option but
> dump it in monitor or "bridge link show". Yes, the output may be lengthy

To make sure there's no misunderstanding - on GETLINK the vlan info is _not_
dumped by default (not compressed or otherwise).
It is only dumped compressed on port notifications, that is why I suggested to
use the already present flag (-compressvlans, -c) in order to include that info
when monitoring or dumping link info, so people watching for the port flags
(show_details) would not suddenly start getting a lot more and it makes sense
to dump it only on specific request.

> and hard to digest for a human, that is why I put it under "show_details"
> kludge - if one is requesting details, he/she should be prepared for
> large volumes of data to be shown :)
> 
> As Nikolay suggested, it'll make sense to request compressed
> output by default, this will be addresses in v2 patch.
> 
> [...]
> 

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Roman Mashak]

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

> On 10/09/17 16:38, Roman Mashak wrote:

[...]

>> Since VLAN info is already in link messages, there is no other option but
>> dump it in monitor or "bridge link show". Yes, the output may be lengthy
>
> To make sure there's no misunderstanding - on GETLINK the vlan info is _not_
> dumped by default (not compressed or otherwise).

Yes, I understand it, I was referring to link events in the previous
comment. However, why not to report compressed vlans for GETLINK by
default if show_details is true, this would be consistent with events monitor.

It seems to be more clear and intuitive from the user's perspective, the
same set of command line options for \monitor' or 'show' commands.

> It is only dumped compressed on port notifications, that is why I suggested to
> use the already present flag (-compressvlans, -c) in order to include that info
> when monitoring or dumping link info, so people watching for the port flags
> (show_details) would not suddenly start getting a lot more and it makes sense
> to dump it only on specific request.
>
>> and hard to digest for a human, that is why I put it under "show_details"
>> kludge - if one is requesting details, he/she should be prepared for
>> large volumes of data to be shown :)
>> 
>> As Nikolay suggested, it'll make sense to request compressed
>> output by default, this will be addresses in v2 patch.
>> 
>> [...]
>> 

Re: [PATCH iproute2 3/3] bridge: request vlans along with link information

[Nikolay Aleksandrov]

On 10/09/17 21:18, Roman Mashak wrote:
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> 
>> On 10/09/17 16:38, Roman Mashak wrote:
> 
> [...]
> 
>>> Since VLAN info is already in link messages, there is no other option but
>>> dump it in monitor or "bridge link show". Yes, the output may be lengthy
>>
>> To make sure there's no misunderstanding - on GETLINK the vlan info is _not_
>> dumped by default (not compressed or otherwise).
> 
> Yes, I understand it, I was referring to link events in the previous
> comment. However, why not to report compressed vlans for GETLINK by
> default if show_details is true, this would be consistent with events monitor.
> 

So my point is to only print the compressed vlans when the -c flag is present on
both occasions. I don't have any strong feeling about this, my only concern is that
people using -d with monitor will start suddenly seeing a lot more than they expect.
The -c (which already exists) will give them the option to skip it. Also the getlink
with lots of vlans and ports will grow by default if there's no way to remove the
vlan dumps (be it compressed or not).

In general I'd prefer to have only one way to do things, e.g. in this particular
case I'd advise people to use bridge vlan show, but I get it that monitor doesn't
show them anywhere. Maybe all of this should go under bridge monitor vlan or
bridge -compressvlans monitor link. Anyway, if other people are fine with extending
the 'show_details' only, then I won't stay in the way.

> It seems to be more clear and intuitive from the user's perspective, the
> same set of command line options for \monitor' or 'show' commands.
> 

That I completely agree with. :-)

>> It is only dumped compressed on port notifications, that is why I suggested to
>> use the already present flag (-compressvlans, -c) in order to include that info
>> when monitoring or dumping link info, so people watching for the port flags
>> (show_details) would not suddenly start getting a lot more and it makes sense
>> to dump it only on specific request.
>>
>>> and hard to digest for a human, that is why I put it under "show_details"
>>> kludge - if one is requesting details, he/she should be prepared for
>>> large volumes of data to be shown :)
>>>
>>> As Nikolay suggested, it'll make sense to request compressed
>>> output by default, this will be addresses in v2 patch.
>>>
>>> [...]
>>>