[PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

Introduce new option -c[ompressvlans] to request
vlan ranges from kernel

(pls suggest better option names if this does not look ok)

$bridge vlan show
port	vlan ids
dummy0	 1 PVID Egress Untagged

dummy1	 1 PVID Egress Untagged
	 2
	 3
	 4
	 5
	 6
	 7
	 9
	 10
	 12

br0	 1 PVID Egress Untagged

$bridge help
Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }
where  OBJECT := { link | fdb | mdb | vlan | monitor }
       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |
                    -o[neline] | -t[imestamp] | -n[etns] name |
                    -c[ompressvlans] }
$bridge -c vlan show
port	vlan ids
dummy0	 1 PVID Egress Untagged

dummy1	 1 PVID Egress Untagged
	 2-7
	 9-10
	 12

br0	 1 PVID Egress Untagged

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 bridge/br_common.h        |    1 +
 bridge/bridge.c           |    6 +++++-
 bridge/vlan.c             |   11 +++++++++--
 include/linux/rtnetlink.h |    1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/bridge/br_common.h b/bridge/br_common.h
index 12fce3e..169a162 100644
--- a/bridge/br_common.h
+++ b/bridge/br_common.h
@@ -16,4 +16,5 @@ extern int preferred_family;
 extern int show_stats;
 extern int show_details;
 extern int timestamp;
+extern int compress_vlans;
 extern struct rtnl_handle rth;
diff --git a/bridge/bridge.c b/bridge/bridge.c
index 5fcc552..88469ca 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -21,6 +21,7 @@ int resolve_hosts;
 int oneline = 0;
 int show_stats;
 int show_details;
+int compress_vlans;
 int timestamp;
 char * _SL_ = NULL;
 
@@ -32,7 +33,8 @@ static void usage(void)
 "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
 "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
 "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
-"                    -o[neline] | -t[imestamp] | -n[etns] name }\n");
+"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
+"                    -c[ompressvlans] }\n");
 	exit(-1);
 }
 
@@ -117,6 +119,8 @@ main(int argc, char **argv)
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
 				exit(-1);
+		} else if (matches(opt, "-compressvlans") == 0) {
+			++compress_vlans;
 		} else {
 			fprintf(stderr, "Option \"%s\" is unknown, try \"bridge help\".\n", opt);
 			exit(-1);
diff --git a/bridge/vlan.c b/bridge/vlan.c
index 88992e6..9f6c84e 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -182,7 +182,12 @@ static int print_vlan(const struct sockaddr_nl *who,
 				continue;
 
 			vinfo = RTA_DATA(i);
-			fprintf(fp, "\t %hu", vinfo->vid);
+			if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END)
+				fprintf(fp, "-%hu", vinfo->vid);
+			else
+				fprintf(fp, "\t %hu", vinfo->vid);
+			if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
+				continue;
 			if (vinfo->flags & BRIDGE_VLAN_INFO_PVID)
 				fprintf(fp, " PVID");
 			if (vinfo->flags & BRIDGE_VLAN_INFO_UNTAGGED)
@@ -218,7 +223,9 @@ static int vlan_show(int argc, char **argv)
 	}
 
 	if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
-				     RTEXT_FILTER_BRVLAN) < 0) {
+				    (compress_vlans ?
+				    RTEXT_FILTER_BRVLAN_COMPRESSED :
+				    RTEXT_FILTER_BRVLAN)) < 0) {
 		perror("Cannont send dump request");
 		exit(1);
 	}
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 9aa5c2f..19eadb1 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -632,6 +632,7 @@ struct tcamsg {
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
 #define RTEXT_FILTER_BRVLAN	(1 << 1)
+#define RTEXT_FILTER_BRVLAN_COMPRESSED  (1 << 2)
 
 /* End of information exported to user level */
 
-- 
1.7.10.4

Re: [PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[Scott Feldman]

> diff --git a/bridge/bridge.c b/bridge/bridge.c
> index 5fcc552..88469ca 100644
> --- a/bridge/bridge.c
> +++ b/bridge/bridge.c
> @@ -21,6 +21,7 @@ int resolve_hosts;
>  int oneline = 0;
>  int show_stats;
>  int show_details;
> +int compress_vlans;
>  int timestamp;
>  char * _SL_ = NULL;
>
> @@ -32,7 +33,8 @@ static void usage(void)
>  "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
>  "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
>  "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
> -"                    -o[neline] | -t[imestamp] | -n[etns] name }\n");
> +"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
> +"                    -c[ompressvlans] }\n");

This option is awkward at this level since it's specific to bridge vlan cmd.

Is it even necessary?  The real user of compressed vlans isn't the
"bridge show" cmd; just keep "bridge show" simple, listing each vlan
per line.

Re: [PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[roopa]

On 1/25/15, 8:35 PM, Scott Feldman wrote:
>> diff --git a/bridge/bridge.c b/bridge/bridge.c
>> index 5fcc552..88469ca 100644
>> --- a/bridge/bridge.c
>> +++ b/bridge/bridge.c
>> @@ -21,6 +21,7 @@ int resolve_hosts;
>>   int oneline = 0;
>>   int show_stats;
>>   int show_details;
>> +int compress_vlans;
>>   int timestamp;
>>   char * _SL_ = NULL;
>>
>> @@ -32,7 +33,8 @@ static void usage(void)
>>   "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
>>   "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
>>   "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
>> -"                    -o[neline] | -t[imestamp] | -n[etns] name }\n");
>> +"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
>> +"                    -c[ompressvlans] }\n");
> This option is awkward at this level since it's specific to bridge vlan cmd.
>
> Is it even necessary?  The real user of compressed vlans isn't the
> "bridge show" cmd; just keep "bridge show" simple, listing each vlan
> per line.
without ranges it does get a bit difficult (or rather long) to read if 
there are many vlans (it was also a comment I received on v2).

All the available options whether they apply to the immediate 
sub-command or not are at the toplevel command today.
Hence the position (Not a big fan of the current name. suggestions welcome).

I would have loved to have the default output print ranges, If that is 
acceptable. But, that can break new iproute2 on older kernels (If that 
is something we should care about).

other option is keyword argument like maybe 'bridge vlan show 
compressed' or 'bridge vlan show ranges'

Thanks,
Roopa

Re: [PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[Scott Feldman]

On Sun, Jan 25, 2015 at 9:18 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 1/25/15, 8:35 PM, Scott Feldman wrote:
>>>
>>> diff --git a/bridge/bridge.c b/bridge/bridge.c
>>> index 5fcc552..88469ca 100644
>>> --- a/bridge/bridge.c
>>> +++ b/bridge/bridge.c
>>> @@ -21,6 +21,7 @@ int resolve_hosts;
>>>   int oneline = 0;
>>>   int show_stats;
>>>   int show_details;
>>> +int compress_vlans;
>>>   int timestamp;
>>>   char * _SL_ = NULL;
>>>
>>> @@ -32,7 +33,8 @@ static void usage(void)
>>>   "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
>>>   "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
>>>   "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
>>> -"                    -o[neline] | -t[imestamp] | -n[etns] name }\n");
>>> +"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
>>> +"                    -c[ompressvlans] }\n");
>>
>> This option is awkward at this level since it's specific to bridge vlan
>> cmd.
>>
>> Is it even necessary?  The real user of compressed vlans isn't the
>> "bridge show" cmd; just keep "bridge show" simple, listing each vlan
>> per line.
>
> without ranges it does get a bit difficult (or rather long) to read if there
> are many vlans (it was also a comment I received on v2).
>
> All the available options whether they apply to the immediate sub-command or
> not are at the toplevel command today.
> Hence the position (Not a big fan of the current name. suggestions welcome).
>
> I would have loved to have the default output print ranges, If that is
> acceptable. But, that can break new iproute2 on older kernels (If that is
> something we should care about).
>
> other option is keyword argument like maybe 'bridge vlan show compressed' or
> 'bridge vlan show ranges'

How about try RTEXT_FILTER_BRVLAN_COMPRESSED first, and if that fails,
try RTEXT_FILTER_BRVLAN, and don't add -c option?

-scott

Re: [PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[roopa]

On 1/25/15, 9:57 PM, Scott Feldman wrote:
> On Sun, Jan 25, 2015 at 9:18 PM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 1/25/15, 8:35 PM, Scott Feldman wrote:
>>>> diff --git a/bridge/bridge.c b/bridge/bridge.c
>>>> index 5fcc552..88469ca 100644
>>>> --- a/bridge/bridge.c
>>>> +++ b/bridge/bridge.c
>>>> @@ -21,6 +21,7 @@ int resolve_hosts;
>>>>    int oneline = 0;
>>>>    int show_stats;
>>>>    int show_details;
>>>> +int compress_vlans;
>>>>    int timestamp;
>>>>    char * _SL_ = NULL;
>>>>
>>>> @@ -32,7 +33,8 @@ static void usage(void)
>>>>    "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
>>>>    "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
>>>>    "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
>>>> -"                    -o[neline] | -t[imestamp] | -n[etns] name }\n");
>>>> +"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
>>>> +"                    -c[ompressvlans] }\n");
>>> This option is awkward at this level since it's specific to bridge vlan
>>> cmd.
>>>
>>> Is it even necessary?  The real user of compressed vlans isn't the
>>> "bridge show" cmd; just keep "bridge show" simple, listing each vlan
>>> per line.
>> without ranges it does get a bit difficult (or rather long) to read if there
>> are many vlans (it was also a comment I received on v2).
>>
>> All the available options whether they apply to the immediate sub-command or
>> not are at the toplevel command today.
>> Hence the position (Not a big fan of the current name. suggestions welcome).
>>
>> I would have loved to have the default output print ranges, If that is
>> acceptable. But, that can break new iproute2 on older kernels (If that is
>> something we should care about).
>>
>> other option is keyword argument like maybe 'bridge vlan show compressed' or
>> 'bridge vlan show ranges'
> How about try RTEXT_FILTER_BRVLAN_COMPRESSED first, and if that fails,
> try RTEXT_FILTER_BRVLAN, and don't add -c option?
>
>
That would have been nice, except that  it will not fail. It will pass 
but not return any vlans.

Better would be to send both flags. I think that will work. New kernel 
looks at RTEXT_FILTER_BRVLAN_COMPRESSED first.
v4  soon ....after some testing ...  ;)

Re: [PATCH net-next v3 2/2] iproute2: bridge vlan show new option to print ranges

[roopa]

On 1/25/15, 10:09 PM, roopa wrote:
> On 1/25/15, 9:57 PM, Scott Feldman wrote:
>> On Sun, Jan 25, 2015 at 9:18 PM, roopa <roopa@cumulusnetworks.com> 
>> wrote:
>>> On 1/25/15, 8:35 PM, Scott Feldman wrote:
>>>>> diff --git a/bridge/bridge.c b/bridge/bridge.c
>>>>> index 5fcc552..88469ca 100644
>>>>> --- a/bridge/bridge.c
>>>>> +++ b/bridge/bridge.c
>>>>> @@ -21,6 +21,7 @@ int resolve_hosts;
>>>>>    int oneline = 0;
>>>>>    int show_stats;
>>>>>    int show_details;
>>>>> +int compress_vlans;
>>>>>    int timestamp;
>>>>>    char * _SL_ = NULL;
>>>>>
>>>>> @@ -32,7 +33,8 @@ static void usage(void)
>>>>>    "Usage: bridge [ OPTIONS ] OBJECT { COMMAND | help }\n"
>>>>>    "where  OBJECT := { link | fdb | mdb | vlan | monitor }\n"
>>>>>    "       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] |\n"
>>>>> -"                    -o[neline] | -t[imestamp] | -n[etns] name 
>>>>> }\n");
>>>>> +"                    -o[neline] | -t[imestamp] | -n[etns] name |\n"
>>>>> +"                    -c[ompressvlans] }\n");
>>>> This option is awkward at this level since it's specific to bridge 
>>>> vlan
>>>> cmd.
>>>>
>>>> Is it even necessary?  The real user of compressed vlans isn't the
>>>> "bridge show" cmd; just keep "bridge show" simple, listing each vlan
>>>> per line.
>>> without ranges it does get a bit difficult (or rather long) to read 
>>> if there
>>> are many vlans (it was also a comment I received on v2).
>>>
>>> All the available options whether they apply to the immediate 
>>> sub-command or
>>> not are at the toplevel command today.
>>> Hence the position (Not a big fan of the current name. suggestions 
>>> welcome).
>>>
>>> I would have loved to have the default output print ranges, If that is
>>> acceptable. But, that can break new iproute2 on older kernels (If 
>>> that is
>>> something we should care about).
>>>
>>> other option is keyword argument like maybe 'bridge vlan show 
>>> compressed' or
>>> 'bridge vlan show ranges'
>> How about try RTEXT_FILTER_BRVLAN_COMPRESSED first, and if that fails,
>> try RTEXT_FILTER_BRVLAN, and don't add -c option?
>>
>>
> That would have been nice, except that  it will not fail. It will pass 
> but not return any vlans.
>
> Better would be to send both flags. I think that will work. New kernel 
> looks at RTEXT_FILTER_BRVLAN_COMPRESSED first.
> v4  soon ....after some testing ...  ;)
This still means i will end up changing the default/current output into 
vlan ranges on newer kernels. If that is acceptable then i will resubmit 
with v4.
If not, v3 looks ok to me so far.

thanks,
Roopa