[PATCH net-next] bridge: add vlan id to mdb notifications

[PATCH net-next] bridge: add vlan id to mdb notifications

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds vlan id to bridge mdb notifications.

I have tested it with older iproute2 and does not seem to break
compatibility.

Signed-off-by: Wilson kok <wkok@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/uapi/linux/if_bridge.h |    1 +
 net/bridge/br_mdb.c            |    4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index da17e45..db061fd 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -185,6 +185,7 @@ struct br_mdb_entry {
 			struct in6_addr ip6;
 		} u;
 		__be16		proto;
+		__be16		vid;
 	} addr;
 };
 
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 5df0526..fa28540 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -92,6 +92,7 @@ static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,
 						e.addr.u.ip6 = p->addr.u.ip6;
 #endif
 					e.addr.proto = p->addr.proto;
+					e.addr.vid = p->addr.vid;
 					if (nla_put(skb, MDBA_MDB_ENTRY_INFO, sizeof(e), &e)) {
 						nla_nest_cancel(skb, nest2);
 						err = -EMSGSIZE;
@@ -240,6 +241,7 @@ void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
 #if IS_ENABLED(CONFIG_IPV6)
 	entry.addr.u.ip6 = group->u.ip6;
 #endif
+	entry.addr.vid = group->vid;
 	__br_mdb_notify(dev, &entry, type);
 }
 
@@ -377,6 +379,7 @@ static int __br_mdb_add(struct net *net, struct net_bridge *br,
 	else
 		ip.u.ip6 = entry->addr.u.ip6;
 #endif
+	ip.vid = entry->addr.vid;
 
 	spin_lock_bh(&br->multicast_lock);
 	ret = br_mdb_add_group(br, p, &ip, entry->state);
@@ -430,6 +433,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 		ip.u.ip6 = entry->addr.u.ip6;
 #endif
 	}
+	ip.vid = entry->addr.vid;
 
 	spin_lock_bh(&br->multicast_lock);
 	mdb = mlock_dereference(br->mdb, br);
-- 
1.7.10.4

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Stephen Hemminger]

On Wed, 26 Nov 2014 05:53:33 -0800
roopa@cumulusnetworks.com wrote:

> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index da17e45..db061fd 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -185,6 +185,7 @@ struct br_mdb_entry {
>  			struct in6_addr ip6;
>  		} u;
>  		__be16		proto;
> +		__be16		vid;
>  	} addr;
>  };
>  

You can't add fields to existing binary API

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Jonathan Toppins]

On 11/26/14 1:56 PM, Stephen Hemminger wrote:
> On Wed, 26 Nov 2014 05:53:33 -0800
> roopa@cumulusnetworks.com wrote:
>
>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>> index da17e45..db061fd 100644
>> --- a/include/uapi/linux/if_bridge.h
>> +++ b/include/uapi/linux/if_bridge.h
>> @@ -185,6 +185,7 @@ struct br_mdb_entry {
>>   			struct in6_addr ip6;
>>   		} u;
>>   		__be16		proto;
>> +		__be16		vid;
>>   	} addr;
>>   };
>>
>
> You can't add fields to existing binary API
>

Roopa, maybe a description of what use case this is trying to solve 
would better justify the addition to the UAPI?

-Jon

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Roopa Prabhu]

On 11/26/14, 11:40 AM, Jonathan Toppins wrote:
> On 11/26/14 1:56 PM, Stephen Hemminger wrote:
>> On Wed, 26 Nov 2014 05:53:33 -0800
>> roopa@cumulusnetworks.com wrote:
>>
>>> diff --git a/include/uapi/linux/if_bridge.h 
>>> b/include/uapi/linux/if_bridge.h
>>> index da17e45..db061fd 100644
>>> --- a/include/uapi/linux/if_bridge.h
>>> +++ b/include/uapi/linux/if_bridge.h
>>> @@ -185,6 +185,7 @@ struct br_mdb_entry {
>>>               struct in6_addr ip6;
>>>           } u;
>>>           __be16        proto;
>>> +        __be16        vid;
>>>       } addr;
>>>   };
>>>
>>
>> You can't add fields to existing binary API
>>
>
> Roopa, maybe a description of what use case this is trying to solve 
> would better justify the addition to the UAPI?
>
I don't think a description of use case can be used to justify a UAPI 
breakage. Getting the patch out was mainly to see if this really breaks 
UAPI.
Basically to get some feedback.

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Roopa Prabhu]

On 11/26/14, 10:56 AM, Stephen Hemminger wrote:
> On Wed, 26 Nov 2014 05:53:33 -0800
> roopa@cumulusnetworks.com wrote:
>
>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>> index da17e45..db061fd 100644
>> --- a/include/uapi/linux/if_bridge.h
>> +++ b/include/uapi/linux/if_bridge.h
>> @@ -185,6 +185,7 @@ struct br_mdb_entry {
>>   			struct in6_addr ip6;
>>   		} u;
>>   		__be16		proto;
>> +		__be16		vid;
>>   	} addr;
>>   };
>>   
> You can't add fields to existing binary API

Ack,  we know the concern..., The fact that it was not changing the size 
of the struct (due to existing padding and i verified that it worked 
with an older iproute2), we wanted to get the patch out and get some 
feedback.

Getting the vlan in the notification is imp and the only other option I 
see is to add a new netlink attribute in the mdb msg.

I have always wondered, if binary netlink attributes have this 
restriction, they should be discouraged. especially when the other 
extensible option is to add them as a separate netlink attribute.

Thanks for the review.

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Thomas Graf]

On 11/26/14 at 12:40pm, Roopa Prabhu wrote:
> I have always wondered, if binary netlink attributes have this restriction,
> they should be discouraged. especially when the other extensible option is
> to add them as a separate netlink attribute.

This is pretty much exactly the reason why attributes have been
introduced ;-)

Re: [PATCH net-next] bridge: add vlan id to mdb notifications

[Roopa Prabhu]

On 11/26/14, 1:53 PM, Thomas Graf wrote:
> On 11/26/14 at 12:40pm, Roopa Prabhu wrote:
>> I have always wondered, if binary netlink attributes have this restriction,
>> they should be discouraged. especially when the other extensible option is
>> to add them as a separate netlink attribute.
> This is pretty much exactly the reason why attributes have been
> introduced ;-)

thanks for confirming that thomas :). Will resubmit the patch with vlan 
information as a separate attribute.