diff mbox

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

Message ID 1417010013-31454-1-git-send-email-roopa@cumulusnetworks.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Roopa Prabhu Nov. 26, 2014, 1:53 p.m. UTC 
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(+)

Comments

Stephen Hemminger Nov. 26, 2014, 6:56 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Toppins Nov. 26, 2014, 7:40 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roopa Prabhu Nov. 26, 2014, 8:40 p.m. UTC | #3 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roopa Prabhu Nov. 26, 2014, 8:45 p.m. UTC | #4 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Graf Nov. 26, 2014, 9:53 p.m. UTC | #5 
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 ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roopa Prabhu Nov. 28, 2014, 10:18 a.m. UTC | #6 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);