[PATCH net-next 0/6] net: bridge: factorize MDB new and del functions

[PATCH net-next 0/6] net: bridge: factorize MDB new and del functions

[Vivien Didelot]

The br_mdb.c file implements both br_mdb_add and br_mdb_del functions, to
handle respectively the RTM_NEWMDB and RTM_DELMDB message types.

But these two functions are really similar. This patch series factorizes
them into a single br_mdb_do rtnl_doit_func function to reduce
boilerplate and simplify the MDB handling code.

Vivien Didelot (6):
  net: bridge: pass net_bridge_port to __br_mdb_add
  net: bridge: check multicast bridge only once
  net: bridge: break if __br_mdb_del fails
  net: bridge: add __br_mdb_do
  net: bridge: get msgtype from nlmsghdr in mdb ops
  net: bridge: add br_mdb_do

 net/bridge/br_mdb.c | 107 +++++++++++++++-------------------------------------
 1 file changed, 31 insertions(+), 76 deletions(-)

-- 
2.13.0

[PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add

[Vivien Didelot]

The __br_mdb_add function uses exactly the same mechanism as its caller
br_mdb_add to retrieve the net_bridge_port pointer from br_mdb_entry.

Remove it and pass directly the net_bridge_port pointer to __br_mdb_add.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index b0845480a3ae..bef0331f46a4 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -542,25 +542,15 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
 	return 0;
 }
 
-static int __br_mdb_add(struct net *net, struct net_bridge *br,
-			struct br_mdb_entry *entry)
+static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 {
+	struct net_bridge *br = p->br;
 	struct br_ip ip;
-	struct net_device *dev;
-	struct net_bridge_port *p;
 	int ret;
 
 	if (!netif_running(br->dev) || br->multicast_disabled)
 		return -EINVAL;
 
-	dev = __dev_get_by_index(net, entry->ifindex);
-	if (!dev)
-		return -ENODEV;
-
-	p = br_port_get_rtnl(dev);
-	if (!p || p->br != br || p->state == BR_STATE_DISABLED)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -602,13 +592,13 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_add(net, br, entry);
+			err = __br_mdb_add(p, entry);
 			if (err)
 				break;
 			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 		}
 	} else {
-		err = __br_mdb_add(net, br, entry);
+		err = __br_mdb_add(p, entry);
 		if (!err)
 			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 	}
-- 
2.13.0

[Bridge] [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add

[Vivien Didelot]

The __br_mdb_add function uses exactly the same mechanism as its caller
br_mdb_add to retrieve the net_bridge_port pointer from br_mdb_entry.

Remove it and pass directly the net_bridge_port pointer to __br_mdb_add.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index b0845480a3ae..bef0331f46a4 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -542,25 +542,15 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
 	return 0;
 }
 
-static int __br_mdb_add(struct net *net, struct net_bridge *br,
-			struct br_mdb_entry *entry)
+static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 {
+	struct net_bridge *br = p->br;
 	struct br_ip ip;
-	struct net_device *dev;
-	struct net_bridge_port *p;
 	int ret;
 
 	if (!netif_running(br->dev) || br->multicast_disabled)
 		return -EINVAL;
 
-	dev = __dev_get_by_index(net, entry->ifindex);
-	if (!dev)
-		return -ENODEV;
-
-	p = br_port_get_rtnl(dev);
-	if (!p || p->br != br || p->state == BR_STATE_DISABLED)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -602,13 +592,13 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_add(net, br, entry);
+			err = __br_mdb_add(p, entry);
 			if (err)
 				break;
 			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 		}
 	} else {
-		err = __br_mdb_add(net, br, entry);
+		err = __br_mdb_add(p, entry);
 		if (!err)
 			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 	}
-- 
2.13.0

[PATCH net-next 2/6] net: bridge: check multicast bridge only once

__br_mdb_add and __br_mdb_del both check that the bridge is up and
running and that multicast is enabled on it before doing any operation.

Since they can be called multiple times by br_mdb_add and br_mdb_del,
move the check in their caller functions so that it is done just once.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index bef0331f46a4..d20a01622b20 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -548,9 +548,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	struct br_ip ip;
 	int ret;
 
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -577,6 +574,9 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	br = netdev_priv(dev);
 
+	if (!netif_running(br->dev) || br->multicast_disabled)
+		return -EINVAL;
+
 	/* If vlan filtering is enabled and VLAN is not specified
 	 * install mdb entry on all vlans configured on the port.
 	 */
@@ -615,9 +615,6 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	struct br_ip ip;
 	int err = -EINVAL;
 
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -672,6 +669,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	br = netdev_priv(dev);
 
+	if (!netif_running(br->dev) || br->multicast_disabled)
+		return -EINVAL;
+
 	/* If vlan filtering is enabled and VLAN is not specified
 	 * delete mdb entry on all vlans configured on the port.
 	 */
-- 
2.13.0

[Bridge] [PATCH net-next 2/6] net: bridge: check multicast bridge only once

[Vivien Didelot]

__br_mdb_add and __br_mdb_del both check that the bridge is up and
running and that multicast is enabled on it before doing any operation.

Since they can be called multiple times by br_mdb_add and br_mdb_del,
move the check in their caller functions so that it is done just once.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index bef0331f46a4..d20a01622b20 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -548,9 +548,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	struct br_ip ip;
 	int ret;
 
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -577,6 +574,9 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	br = netdev_priv(dev);
 
+	if (!netif_running(br->dev) || br->multicast_disabled)
+		return -EINVAL;
+
 	/* If vlan filtering is enabled and VLAN is not specified
 	 * install mdb entry on all vlans configured on the port.
 	 */
@@ -615,9 +615,6 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	struct br_ip ip;
 	int err = -EINVAL;
 
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
 	__mdb_entry_to_br_ip(entry, &ip);
 
 	spin_lock_bh(&br->multicast_lock);
@@ -672,6 +669,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	br = netdev_priv(dev);
 
+	if (!netif_running(br->dev) || br->multicast_disabled)
+		return -EINVAL;
+
 	/* If vlan filtering is enabled and VLAN is not specified
 	 * delete mdb entry on all vlans configured on the port.
 	 */
-- 
2.13.0

[PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d20a01622b20..24eeeefb4179 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
 			err = __br_mdb_del(br, entry);
-			if (!err)
-				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
+			if (err)
+				break;
+			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
 		}
 	} else {
 		err = __br_mdb_del(br, entry);
-- 
2.13.0

[Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d20a01622b20..24eeeefb4179 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
 			err = __br_mdb_del(br, entry);
-			if (!err)
-				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
+			if (err)
+				break;
+			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
 		}
 	} else {
 		err = __br_mdb_del(br, entry);
-- 
2.13.0

[PATCH net-next 4/6] net: bridge: add __br_mdb_do

[Vivien Didelot]

br_mdb_add and br_mdb_del call respectively __br_mdb_add and
__br_mdb_del and notify the message type on success.

Factorize this in a new __br_mdb_do function which add or delete a
br_mdb_entry depending on the message type, then notify it.

Note that the dev argument is p->br->dev, so no need to pass it twice.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 24eeeefb4179..a72d5e6f339f 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,6 +556,9 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	return ret;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+		       int msgtype);
+
 static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		      struct netlink_ext_ack *extack)
 {
@@ -592,15 +595,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_add(p, entry);
+			err = __br_mdb_do(p, entry, RTM_NEWMDB);
 			if (err)
 				break;
-			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 		}
 	} else {
-		err = __br_mdb_add(p, entry);
-		if (!err)
-			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
+		err = __br_mdb_do(p, entry, RTM_NEWMDB);
 	}
 
 	return err;
@@ -651,6 +651,22 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	return err;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+		       int msgtype)
+{
+	int err = -EINVAL;
+
+	if (msgtype == RTM_NEWMDB)
+		err = __br_mdb_add(p, entry);
+	else if (msgtype == RTM_DELMDB)
+		err = __br_mdb_del(p->br, entry);
+
+	if (!err)
+		__br_mdb_notify(p->br->dev, p, entry, msgtype);
+
+	return err;
+}
+
 static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		      struct netlink_ext_ack *extack)
 {
@@ -687,15 +703,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_del(br, entry);
+			err = __br_mdb_do(p, entry, RTM_DELMDB);
 			if (err)
 				break;
-			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
 		}
 	} else {
-		err = __br_mdb_del(br, entry);
-		if (!err)
-			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
+		err = __br_mdb_do(p, entry, RTM_DELMDB);
 	}
 
 	return err;
-- 
2.13.0

[Bridge] [PATCH net-next 4/6] net: bridge: add __br_mdb_do

[Vivien Didelot]

br_mdb_add and br_mdb_del call respectively __br_mdb_add and
__br_mdb_del and notify the message type on success.

Factorize this in a new __br_mdb_do function which add or delete a
br_mdb_entry depending on the message type, then notify it.

Note that the dev argument is p->br->dev, so no need to pass it twice.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 24eeeefb4179..a72d5e6f339f 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,6 +556,9 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	return ret;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+		       int msgtype);
+
 static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		      struct netlink_ext_ack *extack)
 {
@@ -592,15 +595,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_add(p, entry);
+			err = __br_mdb_do(p, entry, RTM_NEWMDB);
 			if (err)
 				break;
-			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
 		}
 	} else {
-		err = __br_mdb_add(p, entry);
-		if (!err)
-			__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
+		err = __br_mdb_do(p, entry, RTM_NEWMDB);
 	}
 
 	return err;
@@ -651,6 +651,22 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	return err;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+		       int msgtype)
+{
+	int err = -EINVAL;
+
+	if (msgtype == RTM_NEWMDB)
+		err = __br_mdb_add(p, entry);
+	else if (msgtype == RTM_DELMDB)
+		err = __br_mdb_del(p->br, entry);
+
+	if (!err)
+		__br_mdb_notify(p->br->dev, p, entry, msgtype);
+
+	return err;
+}
+
 static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		      struct netlink_ext_ack *extack)
 {
@@ -687,15 +703,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_del(br, entry);
+			err = __br_mdb_do(p, entry, RTM_DELMDB);
 			if (err)
 				break;
-			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
 		}
 	} else {
-		err = __br_mdb_del(br, entry);
-		if (!err)
-			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
+		err = __br_mdb_do(p, entry, RTM_DELMDB);
 	}
 
 	return err;
-- 
2.13.0

[PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Vivien Didelot]

Retrieve the message type from the nlmsghdr structure instead of
hardcoding it in both br_mdb_add and br_mdb_del.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a72d5e6f339f..d280b20587cb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_bridge_port *p;
 	struct net_bridge_vlan *v;
 	struct net_bridge *br;
+	int msgtype = nlh->nlmsg_type;
 	int err;
 
 	err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, RTM_NEWMDB);
+			err = __br_mdb_do(p, entry, msgtype);
 			if (err)
 				break;
 		}
 	} else {
-		err = __br_mdb_do(p, entry, RTM_NEWMDB);
+		err = __br_mdb_do(p, entry, msgtype);
 	}
 
 	return err;
@@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_bridge_port *p;
 	struct net_bridge_vlan *v;
 	struct net_bridge *br;
+	int msgtype = nlh->nlmsg_type;
 	int err;
 
 	err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, RTM_DELMDB);
+			err = __br_mdb_do(p, entry, msgtype);
 			if (err)
 				break;
 		}
 	} else {
-		err = __br_mdb_do(p, entry, RTM_DELMDB);
+		err = __br_mdb_do(p, entry, msgtype);
 	}
 
 	return err;
-- 
2.13.0

[Bridge] [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Vivien Didelot]

Retrieve the message type from the nlmsghdr structure instead of
hardcoding it in both br_mdb_add and br_mdb_del.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a72d5e6f339f..d280b20587cb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_bridge_port *p;
 	struct net_bridge_vlan *v;
 	struct net_bridge *br;
+	int msgtype = nlh->nlmsg_type;
 	int err;
 
 	err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, RTM_NEWMDB);
+			err = __br_mdb_do(p, entry, msgtype);
 			if (err)
 				break;
 		}
 	} else {
-		err = __br_mdb_do(p, entry, RTM_NEWMDB);
+		err = __br_mdb_do(p, entry, msgtype);
 	}
 
 	return err;
@@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_bridge_port *p;
 	struct net_bridge_vlan *v;
 	struct net_bridge *br;
+	int msgtype = nlh->nlmsg_type;
 	int err;
 
 	err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, RTM_DELMDB);
+			err = __br_mdb_do(p, entry, msgtype);
 			if (err)
 				break;
 		}
 	} else {
-		err = __br_mdb_do(p, entry, RTM_DELMDB);
+		err = __br_mdb_do(p, entry, msgtype);
 	}
 
 	return err;
-- 
2.13.0

[PATCH net-next 6/6] net: bridge: add br_mdb_do

[Vivien Didelot]

Now that the two functions br_mdb_add and br_mdb_del are identical, keep
a single function named br_mdb_do and register it for both RTM_NEWMDB
and RTM_DELMDB message types.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 61 +++++------------------------------------------------
 1 file changed, 5 insertions(+), 56 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d280b20587cb..1a1da25f3727 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,57 +556,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	return ret;
 }
 
-static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
-		       int msgtype);
-
-static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
-		      struct netlink_ext_ack *extack)
-{
-	struct net *net = sock_net(skb->sk);
-	struct net_bridge_vlan_group *vg;
-	struct net_device *dev, *pdev;
-	struct br_mdb_entry *entry;
-	struct net_bridge_port *p;
-	struct net_bridge_vlan *v;
-	struct net_bridge *br;
-	int msgtype = nlh->nlmsg_type;
-	int err;
-
-	err = br_mdb_parse(skb, nlh, &dev, &entry);
-	if (err < 0)
-		return err;
-
-	br = netdev_priv(dev);
-
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
-	/* If vlan filtering is enabled and VLAN is not specified
-	 * install mdb entry on all vlans configured on the port.
-	 */
-	pdev = __dev_get_by_index(net, entry->ifindex);
-	if (!pdev)
-		return -ENODEV;
-
-	p = br_port_get_rtnl(pdev);
-	if (!p || p->br != br || p->state == BR_STATE_DISABLED)
-		return -EINVAL;
-
-	vg = nbp_vlan_group(p);
-	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
-		list_for_each_entry(v, &vg->vlan_list, vlist) {
-			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, msgtype);
-			if (err)
-				break;
-		}
-	} else {
-		err = __br_mdb_do(p, entry, msgtype);
-	}
-
-	return err;
-}
-
 static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 {
 	struct net_bridge_mdb_htable *mdb;
@@ -668,8 +617,8 @@ static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
 	return err;
 }
 
-static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
-		      struct netlink_ext_ack *extack)
+static int br_mdb_do(struct sk_buff *skb, struct nlmsghdr *nlh,
+		     struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
 	struct net_bridge_vlan_group *vg;
@@ -691,7 +640,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 
 	/* If vlan filtering is enabled and VLAN is not specified
-	 * delete mdb entry on all vlans configured on the port.
+	 * add or delete mdb entry on all vlans configured on the port.
 	 */
 	pdev = __dev_get_by_index(net, entry->ifindex);
 	if (!pdev)
@@ -719,8 +668,8 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 void br_mdb_init(void)
 {
 	rtnl_register(PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, NULL);
-	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, NULL);
-	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_do, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_do, NULL, NULL);
 }
 
 void br_mdb_uninit(void)
-- 
2.13.0

[Bridge] [PATCH net-next 6/6] net: bridge: add br_mdb_do

[Vivien Didelot]

Now that the two functions br_mdb_add and br_mdb_del are identical, keep
a single function named br_mdb_do and register it for both RTM_NEWMDB
and RTM_DELMDB message types.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 61 +++++------------------------------------------------
 1 file changed, 5 insertions(+), 56 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d280b20587cb..1a1da25f3727 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,57 +556,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 	return ret;
 }
 
-static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
-		       int msgtype);
-
-static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
-		      struct netlink_ext_ack *extack)
-{
-	struct net *net = sock_net(skb->sk);
-	struct net_bridge_vlan_group *vg;
-	struct net_device *dev, *pdev;
-	struct br_mdb_entry *entry;
-	struct net_bridge_port *p;
-	struct net_bridge_vlan *v;
-	struct net_bridge *br;
-	int msgtype = nlh->nlmsg_type;
-	int err;
-
-	err = br_mdb_parse(skb, nlh, &dev, &entry);
-	if (err < 0)
-		return err;
-
-	br = netdev_priv(dev);
-
-	if (!netif_running(br->dev) || br->multicast_disabled)
-		return -EINVAL;
-
-	/* If vlan filtering is enabled and VLAN is not specified
-	 * install mdb entry on all vlans configured on the port.
-	 */
-	pdev = __dev_get_by_index(net, entry->ifindex);
-	if (!pdev)
-		return -ENODEV;
-
-	p = br_port_get_rtnl(pdev);
-	if (!p || p->br != br || p->state == BR_STATE_DISABLED)
-		return -EINVAL;
-
-	vg = nbp_vlan_group(p);
-	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
-		list_for_each_entry(v, &vg->vlan_list, vlist) {
-			entry->vid = v->vid;
-			err = __br_mdb_do(p, entry, msgtype);
-			if (err)
-				break;
-		}
-	} else {
-		err = __br_mdb_do(p, entry, msgtype);
-	}
-
-	return err;
-}
-
 static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 {
 	struct net_bridge_mdb_htable *mdb;
@@ -668,8 +617,8 @@ static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
 	return err;
 }
 
-static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
-		      struct netlink_ext_ack *extack)
+static int br_mdb_do(struct sk_buff *skb, struct nlmsghdr *nlh,
+		     struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
 	struct net_bridge_vlan_group *vg;
@@ -691,7 +640,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 
 	/* If vlan filtering is enabled and VLAN is not specified
-	 * delete mdb entry on all vlans configured on the port.
+	 * add or delete mdb entry on all vlans configured on the port.
 	 */
 	pdev = __dev_get_by_index(net, entry->ifindex);
 	if (!pdev)
@@ -719,8 +668,8 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 void br_mdb_init(void)
 {
 	rtnl_register(PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, NULL);
-	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, NULL);
-	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_do, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_do, NULL, NULL);
 }
 
 void br_mdb_uninit(void)
-- 
2.13.0

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index d20a01622b20..24eeeefb4179 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
>   			err = __br_mdb_del(br, entry);
> -			if (!err)
> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
> +			if (err)
> +				break;
> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>   		}
>   	} else {
>   		err = __br_mdb_del(br, entry);
> 

This can potentially break user-space scripts that rely on the best-effort
behaviour, this is the normal "delete without vid & enabled vlan filtering".
You can check the fdb delete code which does the same, this was intentional.

You can add an mdb entry without a vid to all vlans, add a vlan and then try
to remove it from all vlans where it is present - with this patch obviously
that will fail at the new vlan.

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index d20a01622b20..24eeeefb4179 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
>   			err = __br_mdb_del(br, entry);
> -			if (!err)
> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
> +			if (err)
> +				break;
> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>   		}
>   	} else {
>   		err = __br_mdb_del(br, entry);
> 

This can potentially break user-space scripts that rely on the best-effort
behaviour, this is the normal "delete without vid & enabled vlan filtering".
You can check the fdb delete code which does the same, this was intentional.

You can add an mdb entry without a vid to all vlans, add a vlan and then try
to remove it from all vlans where it is present - with this patch obviously
that will fail at the new vlan.

Re: [Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index d20a01622b20..24eeeefb4179 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
>   			err = __br_mdb_del(br, entry);
> -			if (!err)
> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
> +			if (err)
> +				break;
> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>   		}
>   	} else {
>   		err = __br_mdb_del(br, entry);
> 

This can potentially break user-space scripts that rely on the best-effort
behaviour, this is the normal "delete without vid & enabled vlan filtering".
You can check the fdb delete code which does the same, this was intentional.

You can add an mdb entry without a vid to all vlans, add a vlan and then try
to remove it from all vlans where it is present - with this patch obviously
that will fail at the new vlan.

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>>   			err = __br_mdb_del(br, entry);
>> -			if (!err)
>> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>> +			if (err)
>> +				break;
>> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>   		}
>>   	} else {
>>   		err = __br_mdb_del(br, entry);
>> 
>
> This can potentially break user-space scripts that rely on the best-effort
> behaviour, this is the normal "delete without vid & enabled vlan filtering".
> You can check the fdb delete code which does the same, this was intentional.
>
> You can add an mdb entry without a vid to all vlans, add a vlan and then try
> to remove it from all vlans where it is present - with this patch obviously
> that will fail at the new vlan.

OK good to know. That intention wasn't obvious. I can make __br_mdb_del
return void instead? What about the rest of the patchset if I do so?

Thanks,

        Vivien

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>>   			err = __br_mdb_del(br, entry);
>> -			if (!err)
>> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>> +			if (err)
>> +				break;
>> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>   		}
>>   	} else {
>>   		err = __br_mdb_del(br, entry);
>> 
>
> This can potentially break user-space scripts that rely on the best-effort
> behaviour, this is the normal "delete without vid & enabled vlan filtering".
> You can check the fdb delete code which does the same, this was intentional.
>
> You can add an mdb entry without a vid to all vlans, add a vlan and then try
> to remove it from all vlans where it is present - with this patch obviously
> that will fail at the new vlan.

OK good to know. That intention wasn't obvious. I can make __br_mdb_del
return void instead? What about the rest of the patchset if I do so?

Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>>   			err = __br_mdb_del(br, entry);
>> -			if (!err)
>> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>> +			if (err)
>> +				break;
>> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>   		}
>>   	} else {
>>   		err = __br_mdb_del(br, entry);
>> 
>
> This can potentially break user-space scripts that rely on the best-effort
> behaviour, this is the normal "delete without vid & enabled vlan filtering".
> You can check the fdb delete code which does the same, this was intentional.
>
> You can add an mdb entry without a vid to all vlans, add a vlan and then try
> to remove it from all vlans where it is present - with this patch obviously
> that will fail at the new vlan.

OK good to know. That intention wasn't obvious. I can make __br_mdb_del
return void instead? What about the rest of the patchset if I do so?

Thanks,

        Vivien

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Nikolay Aleksandrov]

On 5/18/17 6:08 PM, Vivien Didelot wrote:
> Hi Nikolay,
> 
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> 
>>>    			err = __br_mdb_del(br, entry);
>>> -			if (!err)
>>> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>> +			if (err)
>>> +				break;
>>> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>>    		}
>>>    	} else {
>>>    		err = __br_mdb_del(br, entry);
>>>
>>
>> This can potentially break user-space scripts that rely on the best-effort
>> behaviour, this is the normal "delete without vid & enabled vlan filtering".
>> You can check the fdb delete code which does the same, this was intentional.
>>
>> You can add an mdb entry without a vid to all vlans, add a vlan and then try
>> to remove it from all vlans where it is present - with this patch obviously
>> that will fail at the new vlan.
> 
> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
> return void instead? What about the rest of the patchset if I do so?
> 
> Thanks,
> 
>          Vivien
> 

If you make it return void we will not be able to return proper error value
when doing a single operation (the else case). About the rest I see only some
minor style issues, I'll comment on the respective patches. Another minor nit is 
using switch() instead of if/else for the message types but that is really up to 
you, I don't mind either way. :-)

Cheers,
  Nik

Re: [Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Nikolay Aleksandrov]

On 5/18/17 6:08 PM, Vivien Didelot wrote:
> Hi Nikolay,
> 
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> 
>>>    			err = __br_mdb_del(br, entry);
>>> -			if (!err)
>>> -				__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>> +			if (err)
>>> +				break;
>>> +			__br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>>    		}
>>>    	} else {
>>>    		err = __br_mdb_del(br, entry);
>>>
>>
>> This can potentially break user-space scripts that rely on the best-effort
>> behaviour, this is the normal "delete without vid & enabled vlan filtering".
>> You can check the fdb delete code which does the same, this was intentional.
>>
>> You can add an mdb entry without a vid to all vlans, add a vlan and then try
>> to remove it from all vlans where it is present - with this patch obviously
>> that will fail at the new vlan.
> 
> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
> return void instead? What about the rest of the patchset if I do so?
> 
> Thanks,
> 
>          Vivien
> 

If you make it return void we will not be able to return proper error value
when doing a single operation (the else case). About the rest I see only some
minor style issues, I'll comment on the respective patches. Another minor nit is 
using switch() instead of if/else for the message types but that is really up to 
you, I don't mind either way. :-)

Cheers,
  Nik

Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Retrieve the message type from the nlmsghdr structure instead of
> hardcoding it in both br_mdb_add and br_mdb_del.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index a72d5e6f339f..d280b20587cb 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

minor nits:
nlmsg_type is a u16, also please keep the order and arrange these from longest 
to shortest

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> @@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

same here

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_DELMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_DELMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> 

Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Retrieve the message type from the nlmsghdr structure instead of
> hardcoding it in both br_mdb_add and br_mdb_del.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index a72d5e6f339f..d280b20587cb 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

minor nits:
nlmsg_type is a u16, also please keep the order and arrange these from longest 
to shortest

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> @@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

same here

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_DELMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_DELMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> 

Re: [Bridge] [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Nikolay Aleksandrov]

On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Retrieve the message type from the nlmsghdr structure instead of
> hardcoding it in both br_mdb_add and br_mdb_del.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/bridge/br_mdb.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index a72d5e6f339f..d280b20587cb 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

minor nits:
nlmsg_type is a u16, also please keep the order and arrange these from longest 
to shortest

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_NEWMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> @@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	struct net_bridge_port *p;
>   	struct net_bridge_vlan *v;
>   	struct net_bridge *br;
> +	int msgtype = nlh->nlmsg_type;

same here

>   	int err;
>   
>   	err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	if (br_vlan_enabled(br) && vg && entry->vid == 0) {
>   		list_for_each_entry(v, &vg->vlan_list, vlist) {
>   			entry->vid = v->vid;
> -			err = __br_mdb_do(p, entry, RTM_DELMDB);
> +			err = __br_mdb_do(p, entry, msgtype);
>   			if (err)
>   				break;
>   		}
>   	} else {
> -		err = __br_mdb_do(p, entry, RTM_DELMDB);
> +		err = __br_mdb_do(p, entry, msgtype);
>   	}
>   
>   	return err;
> 

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
>> return void instead? What about the rest of the patchset if I do so?
>
> If you make it return void we will not be able to return proper error value
> when doing a single operation (the else case). About the rest I see only some
> minor style issues, I'll comment on the respective patches. Another minor nit is 
> using switch() instead of if/else for the message types but that is really up to 
> you, I don't mind either way. :-)

Ho OK I understand better the batch vs single delete operation now.
__br_mdb_do hardly makes sense now, because we don't know which case we
are handling... But factorizing br_mdb_do still makes sense. I'll come
up with something.

Thanks,

        Vivien

Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
>> return void instead? What about the rest of the patchset if I do so?
>
> If you make it return void we will not be able to return proper error value
> when doing a single operation (the else case). About the rest I see only some
> minor style issues, I'll comment on the respective patches. Another minor nit is 
> using switch() instead of if/else for the message types but that is really up to 
> you, I don't mind either way. :-)

Ho OK I understand better the batch vs single delete operation now.
__br_mdb_do hardly makes sense now, because we don't know which case we
are handling... But factorizing br_mdb_do still makes sense. I'll come
up with something.

Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
>> return void instead? What about the rest of the patchset if I do so?
>
> If you make it return void we will not be able to return proper error value
> when doing a single operation (the else case). About the rest I see only some
> minor style issues, I'll comment on the respective patches. Another minor nit is 
> using switch() instead of if/else for the message types but that is really up to 
> you, I don't mind either way. :-)

Ho OK I understand better the batch vs single delete operation now.
__br_mdb_do hardly makes sense now, because we don't know which case we
are handling... But factorizing br_mdb_do still makes sense. I'll come
up with something.

Thanks,

        Vivien

Re: [SPAM]Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>> +	int msgtype = nlh->nlmsg_type;
>
> minor nits:
> nlmsg_type is a u16, also please keep the order and arrange these from longest 
> to shortest

The reverse christmas tree \o/

Hum, __br_mdb_notify takes an int type, and struct nlmsghdr defines it
as a __u16. Does u16 still make sense here instead of int?


Thanks,

        Vivien

Re: [Bridge] [SPAM]Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Vivien Didelot]

Hi Nikolay,

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:

>> +	int msgtype = nlh->nlmsg_type;
>
> minor nits:
> nlmsg_type is a u16, also please keep the order and arrange these from longest 
> to shortest

The reverse christmas tree \o/

Hum, __br_mdb_notify takes an int type, and struct nlmsghdr defines it
as a __u16. Does u16 still make sense here instead of int?


Thanks,

        Vivien

Re: [SPAM]Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Nikolay Aleksandrov]

On 5/18/17 6:53 PM, Vivien Didelot wrote:
> Hi Nikolay,
> 
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> 
>>> +	int msgtype = nlh->nlmsg_type;
>>
>> minor nits:
>> nlmsg_type is a u16, also please keep the order and arrange these from longest
>> to shortest
> 
> The reverse christmas tree \o/
> 
> Hum, __br_mdb_notify takes an int type, and struct nlmsghdr defines it
> as a __u16. Does u16 still make sense here instead of int?
> 
> 
> Thanks,
> 
>          Vivien
> 

Either way is fine as long as the value is unchanged, and since the rest of
the code uses an int then lets be consistent and leave it or if you decide 
change all.

Cheers,
  Nik

Re: [Bridge] [SPAM]Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Nikolay Aleksandrov]

On 5/18/17 6:53 PM, Vivien Didelot wrote:
> Hi Nikolay,
> 
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> 
>>> +	int msgtype = nlh->nlmsg_type;
>>
>> minor nits:
>> nlmsg_type is a u16, also please keep the order and arrange these from longest
>> to shortest
> 
> The reverse christmas tree \o/
> 
> Hum, __br_mdb_notify takes an int type, and struct nlmsghdr defines it
> as a __u16. Does u16 still make sense here instead of int?
> 
> 
> Thanks,
> 
>          Vivien
> 

Either way is fine as long as the value is unchanged, and since the rest of
the code uses an int then lets be consistent and leave it or if you decide 
change all.

Cheers,
  Nik