[patch net-next-2.6 1/4] net: extend netlink interface to handle generic slave management

[patch net-next-2.6 1/4] net: extend netlink interface to handle generic slave management

[Jiri Pirko]

Drivers like bridge and bonding uses their own way to manipulate with
underlink devices. This is an attempt to introduce common interface using
netlink.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 include/linux/if_link.h   |   13 +++++++
 include/linux/netdevice.h |   22 ++++++++++++
 net/core/rtnetlink.c      |   81 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index f4a2e6b..48a5f95 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -136,6 +136,9 @@ enum {
 	IFLA_PORT_SELF,
 	IFLA_AF_SPEC,
 	IFLA_GROUP,		/* Group the device belongs to */
+	IFLA_SLAVE_LIST,
+	IFLA_SLAVE_ADD,
+	IFLA_SLAVE_DEL,
 	__IFLA_MAX
 };
 
@@ -379,4 +382,14 @@ struct ifla_port_vsi {
 	__u8 pad[3];
 };
 
+/* Slave devices management section */
+
+enum {
+	IFLA_SLAVE_UNSPEC,
+	IFLA_SLAVE_DEV,
+	__IFLA_SLAVE_MAX,
+};
+
+#define IFLA_SLAVE_MAX (__IFLA_SLAVE_MAX - 1)
+
 #endif /* _LINUX_IF_LINK_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c7d7074..844cb85 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -783,6 +783,21 @@ struct netdev_tc_txq {
  *	Set hardware filter for RFS.  rxq_index is the target queue index;
  *	flow_id is a flow ID to be passed to rps_may_expire_flow() later.
  *	Return the filter ID on success, or a negative error code.
+ *
+ *	Slave management functions (for bridge, bonding, etc). User should
+ *	make sure that slave list doesn't change within rtnl_lock.
+ * int (*ndo_add_slave)(struct net_device *dev, struct net_device *slave_dev);
+ *	Called to make another netdev an underlink.
+ *
+ * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
+ *	Called to release previously enslaved netdev.
+ *
+ * int (*ndo_get_slave_count)(const struct net_device *dev);
+ *	Called to get number of enslaved devices.
+ *
+ * struct net_device * (*ndo_get_slave)(const struct net_device *dev,
+ *					int slave_index);
+ *	Called to get slave device by it's index.
  */
 #define HAVE_NET_DEVICE_OPS
 struct net_device_ops {
@@ -862,6 +877,13 @@ struct net_device_ops {
 						     u16 rxq_index,
 						     u32 flow_id);
 #endif
+	int			(*ndo_add_slave)(struct net_device *dev,
+						 struct net_device *slave_dev);
+	int			(*ndo_del_slave)(struct net_device *dev,
+						 struct net_device *slave_dev);
+	int			(*ndo_get_slave_count)(const struct net_device *dev);
+	struct net_device *	(*ndo_get_slave)(const struct net_device *dev,
+						 int slave_index);
 };
 
 /*
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index da0fe45..1402a3f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -739,6 +739,20 @@ static size_t rtnl_port_size(const struct net_device *dev)
 		return port_self_size;
 }
 
+static size_t rtnl_slave_list_size(const struct net_device *dev)
+{
+	size_t slave_size = nla_total_size(sizeof(struct nlattr)) +
+			    nla_total_size(4);
+	size_t slave_list_size = nla_total_size(sizeof(struct nlattr));
+	int slave_count;
+
+	if (!dev->netdev_ops->ndo_get_slave_count ||
+	    !dev->netdev_ops->ndo_get_slave)
+		return 0;
+	slave_count = dev->netdev_ops->ndo_get_slave_count(dev);
+	return slave_list_size + slave_count * slave_size;
+}
+
 static noinline size_t if_nlmsg_size(const struct net_device *dev)
 {
 	return NLMSG_ALIGN(sizeof(struct ifinfomsg))
@@ -760,6 +774,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev)
 	       + nla_total_size(4) /* IFLA_NUM_VF */
 	       + rtnl_vfinfo_size(dev) /* IFLA_VFINFO_LIST */
 	       + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
+	       + rtnl_slave_list_size(dev) /* IFLA_SLAVE_LIST */
 	       + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
 	       + rtnl_link_get_af_size(dev); /* IFLA_AF_SPEC */
 }
@@ -839,6 +854,39 @@ static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static int rtnl_slave_list_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	int slave_count;
+	int i;
+	struct nlattr *slave_list;
+
+	if (!dev->netdev_ops->ndo_get_slave_count ||
+	    !dev->netdev_ops->ndo_get_slave)
+		return 0;
+	slave_count = dev->netdev_ops->ndo_get_slave_count(dev);
+	slave_list = nla_nest_start(skb, IFLA_SLAVE_LIST);
+	if (!slave_list)
+		return -EMSGSIZE;
+	for (i = 0; i < slave_count; i++) {
+		struct net_device *slave_dev;
+
+		slave_dev = dev->netdev_ops->ndo_get_slave(dev, i);
+		if (!slave_dev) {
+			nla_nest_cancel(skb, slave_list);
+			goto nla_put_failure;
+		}
+		NLA_PUT_U32(skb, IFLA_SLAVE_DEV, slave_dev->ifindex);
+	}
+	nla_nest_end(skb, slave_list);
+
+	return 0;
+
+nla_put_failure:
+	nla_nest_cancel(skb, slave_list);
+	return -EMSGSIZE;
+
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags)
@@ -953,6 +1001,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	if (rtnl_port_fill(skb, dev))
 		goto nla_put_failure;
 
+	if (rtnl_slave_list_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (dev->rtnl_link_ops) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;
@@ -1047,6 +1098,8 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_VF_PORTS]		= { .type = NLA_NESTED },
 	[IFLA_PORT_SELF]	= { .type = NLA_NESTED },
 	[IFLA_AF_SPEC]		= { .type = NLA_NESTED },
+	[IFLA_SLAVE_ADD]	= { .type = NLA_U32 },
+	[IFLA_SLAVE_DEL]	= { .type = NLA_U32 },
 };
 EXPORT_SYMBOL(ifla_policy);
 
@@ -1392,6 +1445,34 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 			modified = 1;
 		}
 	}
+
+	if (tb[IFLA_SLAVE_ADD]) {
+		int ifindex = nla_get_u32(tb[IFLA_SLAVE_ADD]);
+		struct net_device *slave_dev;
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_add_slave) {
+			slave_dev = __dev_get_by_index(dev_net(dev), ifindex);
+			err = ops->ndo_add_slave(dev, slave_dev);
+		}
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
+
+	if (tb[IFLA_SLAVE_DEL]) {
+		int ifindex = nla_get_u32(tb[IFLA_SLAVE_DEL]);
+		struct net_device *slave_dev;
+
+		err = -EOPNOTSUPP;
+		if (ops->ndo_del_slave) {
+			slave_dev = __dev_get_by_index(dev_net(dev), ifindex);
+			err = ops->ndo_del_slave(dev, slave_dev);
+		}
+		if (err < 0)
+			goto errout;
+		modified = 1;
+	}
 	err = 0;
 
 errout:
-- 
1.7.3.4

[patch iproute2 2/4] implement slave management operations

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 ip/ipaddress.c |   22 ++++++++++++++++++++++
 ip/iplink.c    |   16 ++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index a775ecd..7446b81 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -216,6 +216,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
 		fprintf(fp, ", tx rate %d (Mbps)", vf_tx_rate->rate);
 }
 
+static void print_slave_info(FILE *fp, struct rtattr *sl)
+{
+	SPRINT_BUF(b1);
+
+	if (sl->rta_type != IFLA_SLAVE_DEV) {
+		fprintf(stderr, "BUG: rta type is %d\n", sl->rta_type);
+		return;
+	}
+
+	fprintf(fp, " %s", ll_idx_n2a(*(int*)RTA_DATA(sl), b1));
+}
+
 int print_linkinfo(const struct sockaddr_nl *who,
 		   struct nlmsghdr *n, void *arg)
 {
@@ -420,6 +432,16 @@ int print_linkinfo(const struct sockaddr_nl *who,
 			print_vfinfo(fp, i);
 	}
 
+	if (tb[IFLA_SLAVE_LIST]) {
+		struct rtattr *i, *slave_list = tb[IFLA_SLAVE_LIST];
+		int rem = RTA_PAYLOAD(slave_list);
+		fprintf(fp, "\n    slaves:");
+		for (i = RTA_DATA(slave_list);
+		     RTA_OK(i, rem);
+		     i = RTA_NEXT(i, rem))
+			print_slave_info(fp, i);
+	}
+
 	fprintf(fp, "\n");
 	fflush(fp);
 	return 0;
diff --git a/ip/iplink.c b/ip/iplink.c
index cb2c4f5..02c48a2 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -71,6 +71,7 @@ void iplink_usage(void)
 	fprintf(stderr, "	                  [ vf NUM [ mac LLADDR ]\n");
 	fprintf(stderr, "				   [ vlan VLANID [ qos VLAN-QOS ] ]\n");
 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
+	fprintf(stderr, "			  [ slave [ { add | del } ] DEVICE ]\n");
 	fprintf(stderr, "       ip link show [ DEVICE ]\n");
 
 	if (iplink_have_newlink()) {
@@ -361,6 +362,21 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			if (len < 0)
 				return -1;
 			addattr_nest_end(&req->n, vflist);
+		} else if (matches(*argv, "slave") == 0) {
+			int type;
+			int ifindex;
+			NEXT_ARG();
+			if (strcmp(*argv, "add") == 0)
+				type = IFLA_SLAVE_ADD;
+			else if (strcmp(*argv, "del") == 0)
+				type = IFLA_SLAVE_DEL;
+			else
+				invarg("Invalid slave action\n", *argv);
+			NEXT_ARG();
+			ifindex = ll_name_to_index(*argv);
+			if (!ifindex)
+				invarg("Invalid slave device\n", *argv);
+			addattr_l(&req->n, sizeof(*req), type, &ifindex, 4);
 #ifdef IFF_DYNAMIC
 		} else if (matches(*argv, "dynamic") == 0) {
 			NEXT_ARG();
-- 
1.7.3.4

[patch net-next-2.6 3/4] bond: implement slave management operations

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/bonding/bond_main.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1df9f0e..f8e59f9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4285,6 +4285,40 @@ unwind:
 	return res;
 }
 
+static int bond_add_slave(struct net_device *bond_dev,
+			  struct net_device *slave_dev)
+{
+	return bond_enslave(bond_dev, slave_dev);
+}
+
+static int bond_del_slave(struct net_device *bond_dev,
+			  struct net_device *slave_dev)
+{
+	return bond_release(bond_dev, slave_dev);
+}
+
+static int bond_get_slave_count(const struct net_device *bond_dev)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+
+	return bond->slave_cnt;
+}
+
+static struct net_device *bond_get_slave(const struct net_device *bond_dev,
+					 int slave_index)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+	struct slave *slave;
+	int i;
+
+	/* no need to hold bond->lock here, protected against writers by rtnl */
+	bond_for_each_slave(bond, slave, i) {
+		if (slave_index == i)
+			return slave->dev;
+	}
+	return NULL;
+}
+
 static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
@@ -4657,6 +4691,10 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_netpoll_cleanup	= bond_netpoll_cleanup,
 	.ndo_poll_controller	= bond_poll_controller,
 #endif
+	.ndo_add_slave		= bond_add_slave,
+	.ndo_del_slave		= bond_del_slave,
+	.ndo_get_slave_count	= bond_get_slave_count,
+	.ndo_get_slave		= bond_get_slave,
 };
 
 static void bond_destructor(struct net_device *bond_dev)
-- 
1.7.3.4

[patch net-next-2.6 4/4] bridge: implement slave management operations

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 net/bridge/br_device.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 5564435..5648002 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -297,6 +297,46 @@ void br_netpoll_disable(struct net_bridge_port *p)
 
 #endif
 
+static int br_add_slave(struct net_device *dev, struct net_device *slave_dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	return br_add_if(br, slave_dev);
+}
+
+static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	return br_del_if(br, slave_dev);
+}
+
+static int br_get_slave_count(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_port *p;
+	int i = 0;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		i++;
+	}
+	return i;
+}
+
+static struct net_device *br_get_slave(const struct net_device *dev,
+				       int slave_index)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_port *p;
+	int i = 0;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (slave_index == i++)
+			return p->dev;
+	}
+	return NULL;
+}
+
 static const struct ethtool_ops br_ethtool_ops = {
 	.get_drvinfo    = br_getinfo,
 	.get_link	= ethtool_op_get_link,
@@ -326,6 +366,10 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_netpoll_cleanup	 = br_netpoll_cleanup,
 	.ndo_poll_controller	 = br_poll_controller,
 #endif
+	.ndo_add_slave		 = br_add_slave,
+	.ndo_del_slave		 = br_del_slave,
+	.ndo_get_slave_count	 = br_get_slave_count,
+	.ndo_get_slave		 = br_get_slave,
 };
 
 static void br_dev_free(struct net_device *dev)
-- 
1.7.3.4

Re: [patch iproute2 2/4] implement slave management operations

[Jiri Pirko]

Stephen,

I did not include include/linux/if_link.h changes here as I saw in
history it's been done in the same way. If they are needed I can repost.

Thanks, Jirka

Fri, Feb 11, 2011 at 04:22:15PM CET, jpirko@redhat.com wrote:
>
>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>---
> ip/ipaddress.c |   22 ++++++++++++++++++++++
> ip/iplink.c    |   16 ++++++++++++++++
> 2 files changed, 38 insertions(+), 0 deletions(-)
>
>diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>index a775ecd..7446b81 100644
>--- a/ip/ipaddress.c
>+++ b/ip/ipaddress.c
>@@ -216,6 +216,18 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> 		fprintf(fp, ", tx rate %d (Mbps)", vf_tx_rate->rate);
> }
> 
>+static void print_slave_info(FILE *fp, struct rtattr *sl)
>+{
>+	SPRINT_BUF(b1);
>+
>+	if (sl->rta_type != IFLA_SLAVE_DEV) {
>+		fprintf(stderr, "BUG: rta type is %d\n", sl->rta_type);
>+		return;
>+	}
>+
>+	fprintf(fp, " %s", ll_idx_n2a(*(int*)RTA_DATA(sl), b1));
>+}
>+
> int print_linkinfo(const struct sockaddr_nl *who,
> 		   struct nlmsghdr *n, void *arg)
> {
>@@ -420,6 +432,16 @@ int print_linkinfo(const struct sockaddr_nl *who,
> 			print_vfinfo(fp, i);
> 	}
> 
>+	if (tb[IFLA_SLAVE_LIST]) {
>+		struct rtattr *i, *slave_list = tb[IFLA_SLAVE_LIST];
>+		int rem = RTA_PAYLOAD(slave_list);
>+		fprintf(fp, "\n    slaves:");
>+		for (i = RTA_DATA(slave_list);
>+		     RTA_OK(i, rem);
>+		     i = RTA_NEXT(i, rem))
>+			print_slave_info(fp, i);
>+	}
>+
> 	fprintf(fp, "\n");
> 	fflush(fp);
> 	return 0;
>diff --git a/ip/iplink.c b/ip/iplink.c
>index cb2c4f5..02c48a2 100644
>--- a/ip/iplink.c
>+++ b/ip/iplink.c
>@@ -71,6 +71,7 @@ void iplink_usage(void)
> 	fprintf(stderr, "	                  [ vf NUM [ mac LLADDR ]\n");
> 	fprintf(stderr, "				   [ vlan VLANID [ qos VLAN-QOS ] ]\n");
> 	fprintf(stderr, "				   [ rate TXRATE ] ] \n");
>+	fprintf(stderr, "			  [ slave [ { add | del } ] DEVICE ]\n");
> 	fprintf(stderr, "       ip link show [ DEVICE ]\n");
> 
> 	if (iplink_have_newlink()) {
>@@ -361,6 +362,21 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
> 			if (len < 0)
> 				return -1;
> 			addattr_nest_end(&req->n, vflist);
>+		} else if (matches(*argv, "slave") == 0) {
>+			int type;
>+			int ifindex;
>+			NEXT_ARG();
>+			if (strcmp(*argv, "add") == 0)
>+				type = IFLA_SLAVE_ADD;
>+			else if (strcmp(*argv, "del") == 0)
>+				type = IFLA_SLAVE_DEL;
>+			else
>+				invarg("Invalid slave action\n", *argv);
>+			NEXT_ARG();
>+			ifindex = ll_name_to_index(*argv);
>+			if (!ifindex)
>+				invarg("Invalid slave device\n", *argv);
>+			addattr_l(&req->n, sizeof(*req), type, &ifindex, 4);
> #ifdef IFF_DYNAMIC
> 		} else if (matches(*argv, "dynamic") == 0) {
> 			NEXT_ARG();
>-- 
>1.7.3.4
>

Re: [patch net-next-2.6 1/4] net: extend netlink interface to handle generic slave management

[Patrick McHardy]

On 11.02.2011 16:21, Jiri Pirko wrote:
> Drivers like bridge and bonding uses their own way to manipulate with
> underlink devices. This is an attempt to introduce common interface using
> netlink.

Thanks for working on this, this has been on my TODO list for a
long time.

> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -136,6 +136,9 @@ enum {
>  	IFLA_PORT_SELF,
>  	IFLA_AF_SPEC,
>  	IFLA_GROUP,		/* Group the device belongs to */
> +	IFLA_SLAVE_LIST,
> +	IFLA_SLAVE_ADD,
> +	IFLA_SLAVE_DEL,

I don't like this very much though, the attributes usually contain
data, not commands. We already have NEWLINK, DELLINK etc. on the
top level, the combinations of NEWLINK/NLM_F_CREAT and SLAVE_DEL
or DELLINK and SLAVE_ADD and so on simply don't make sense.

We usually also try to keep the interface symetrical in both
directions (a NEWLINK message from the kernel is identical to a
NEWLINK message from userspace, a DELLINK message as well besides
containing additional information), so using different attributes
for dumping slaves than for adding them seems wrong. If we can
dump all slaves in one message, it should also be possible to
enslave multiple devices using the same message.

What I originally had planned to support enslaving devices is to
make use of the IFLA_MASTER attribute. The IFLA_MASTER attribute
would contain the bond or bridge ifindex and the IFLA_IFNAME
attribute or ifindex would specify the slave device. All operations
would be performed on the slave device as usual, if the IFLA_MASTER
attribute is present we'd additionally call a master specific
callback for enslaving or releasing slave devices. Besides allowing
to keep messages symetrical, an additional benefit is that it would
be possible to create and enslave a device in a single step.

Re: [patch net-next-2.6 3/4] bond: implement slave management operations

[Jay Vosburgh]

Jiri Pirko <jpirko@redhat.com> wrote:

>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>---
> drivers/net/bonding/bond_main.c |   38 ++++++++++++++++++++++++++++++++++++++
> 1 files changed, 38 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1df9f0e..f8e59f9 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c

	I think this would be better served by a new bond_netlink.c
file instead of cramming this into (the already huge) bond_main.c.  In
the long run, there will be a lot more netlink related code in bonding,
so I think it makes sense to give it a file of its own from the
beginning.

>@@ -4285,6 +4285,40 @@ unwind:
> 	return res;
> }
>
>+static int bond_add_slave(struct net_device *bond_dev,
>+			  struct net_device *slave_dev)
>+{
>+	return bond_enslave(bond_dev, slave_dev);
>+}
>+
>+static int bond_del_slave(struct net_device *bond_dev,
>+			  struct net_device *slave_dev)
>+{
>+	return bond_release(bond_dev, slave_dev);
>+}
>+
>+static int bond_get_slave_count(const struct net_device *bond_dev)
>+{
>+	struct bonding *bond = netdev_priv(bond_dev);
>+
>+	return bond->slave_cnt;
>+}
>+
>+static struct net_device *bond_get_slave(const struct net_device *bond_dev,
>+					 int slave_index)
>+{
>+	struct bonding *bond = netdev_priv(bond_dev);
>+	struct slave *slave;
>+	int i;
>+
>+	/* no need to hold bond->lock here, protected against writers by rtnl */
>+	bond_for_each_slave(bond, slave, i) {
>+		if (slave_index == i)
>+			return slave->dev;
>+	}
>+	return NULL;

	I think using the name "slave_index" for this variable is
confusing, since it isn't the ifindex of the slave.  This "index" is
used to iterate through the list of slaves, so perhaps "slave_num" or
"slave_position" is clearer.  The same comment applies to the equivalent
code for bridge.

	-J

>+}
>+
> static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
>@@ -4657,6 +4691,10 @@ static const struct net_device_ops bond_netdev_ops = {
> 	.ndo_netpoll_cleanup	= bond_netpoll_cleanup,
> 	.ndo_poll_controller	= bond_poll_controller,
> #endif
>+	.ndo_add_slave		= bond_add_slave,
>+	.ndo_del_slave		= bond_del_slave,
>+	.ndo_get_slave_count	= bond_get_slave_count,
>+	.ndo_get_slave		= bond_get_slave,
> };
>
> static void bond_destructor(struct net_device *bond_dev)
>-- 
>1.7.3.4
>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [patch net-next-2.6 1/4] net: extend netlink interface to handle generic slave management

[Jiri Pirko]

Fri, Feb 11, 2011 at 04:48:19PM CET, kaber@trash.net wrote:
>On 11.02.2011 16:21, Jiri Pirko wrote:
>> Drivers like bridge and bonding uses their own way to manipulate with
>> underlink devices. This is an attempt to introduce common interface using
>> netlink.
>
>Thanks for working on this, this has been on my TODO list for a
>long time.
>
>> --- a/include/linux/if_link.h
>> +++ b/include/linux/if_link.h
>> @@ -136,6 +136,9 @@ enum {
>>  	IFLA_PORT_SELF,
>>  	IFLA_AF_SPEC,
>>  	IFLA_GROUP,		/* Group the device belongs to */
>> +	IFLA_SLAVE_LIST,
>> +	IFLA_SLAVE_ADD,
>> +	IFLA_SLAVE_DEL,
>
>I don't like this very much though, the attributes usually contain
>data, not commands. We already have NEWLINK, DELLINK etc. on the
>top level, the combinations of NEWLINK/NLM_F_CREAT and SLAVE_DEL
>or DELLINK and SLAVE_ADD and so on simply don't make sense.
>
>We usually also try to keep the interface symetrical in both
>directions (a NEWLINK message from the kernel is identical to a
>NEWLINK message from userspace, a DELLINK message as well besides
>containing additional information), so using different attributes
>for dumping slaves than for adding them seems wrong. If we can
>dump all slaves in one message, it should also be possible to
>enslave multiple devices using the same message.
>
>What I originally had planned to support enslaving devices is to
>make use of the IFLA_MASTER attribute. The IFLA_MASTER attribute
>would contain the bond or bridge ifindex and the IFLA_IFNAME
>attribute or ifindex would specify the slave device. All operations
>would be performed on the slave device as usual, if the IFLA_MASTER
>attribute is present we'd additionally call a master specific
>callback for enslaving or releasing slave devices. Besides allowing
>to keep messages symetrical, an additional benefit is that it would
>be possible to create and enslave a device in a single step.
>

Yes, that makes sense. I'm going to respin the patchset soon.

Jirka
>

Re: [patch net-next-2.6 3/4] bond: implement slave management operations

[Jiri Pirko]

Fri, Feb 11, 2011 at 06:19:50PM CET, fubar@us.ibm.com wrote:
>Jiri Pirko <jpirko@redhat.com> wrote:
>
>>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>---
>> drivers/net/bonding/bond_main.c |   38 ++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 38 insertions(+), 0 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 1df9f0e..f8e59f9 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>
>	I think this would be better served by a new bond_netlink.c
>file instead of cramming this into (the already huge) bond_main.c.  In
>the long run, there will be a lot more netlink related code in bonding,
>so I think it makes sense to give it a file of its own from the
>beginning.

Well technically is not netlink code so givin it into *netlink* file
would imho make no sense.

>
>>@@ -4285,6 +4285,40 @@ unwind:
>> 	return res;
>> }
>>
>>+static int bond_add_slave(struct net_device *bond_dev,
>>+			  struct net_device *slave_dev)
>>+{
>>+	return bond_enslave(bond_dev, slave_dev);
>>+}
>>+
>>+static int bond_del_slave(struct net_device *bond_dev,
>>+			  struct net_device *slave_dev)
>>+{
>>+	return bond_release(bond_dev, slave_dev);
>>+}
>>+
>>+static int bond_get_slave_count(const struct net_device *bond_dev)
>>+{
>>+	struct bonding *bond = netdev_priv(bond_dev);
>>+
>>+	return bond->slave_cnt;
>>+}
>>+
>>+static struct net_device *bond_get_slave(const struct net_device *bond_dev,
>>+					 int slave_index)
>>+{
>>+	struct bonding *bond = netdev_priv(bond_dev);
>>+	struct slave *slave;
>>+	int i;
>>+
>>+	/* no need to hold bond->lock here, protected against writers by rtnl */
>>+	bond_for_each_slave(bond, slave, i) {
>>+		if (slave_index == i)
>>+			return slave->dev;
>>+	}
>>+	return NULL;
>
>	I think using the name "slave_index" for this variable is
>confusing, since it isn't the ifindex of the slave.  This "index" is
>used to iterate through the list of slaves, so perhaps "slave_num" or
>"slave_position" is clearer.  The same comment applies to the equivalent
>code for bridge.
>
>	-J
>
>>+}
>>+
>> static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
>> {
>> 	struct bonding *bond = netdev_priv(bond_dev);
>>@@ -4657,6 +4691,10 @@ static const struct net_device_ops bond_netdev_ops = {
>> 	.ndo_netpoll_cleanup	= bond_netpoll_cleanup,
>> 	.ndo_poll_controller	= bond_poll_controller,
>> #endif
>>+	.ndo_add_slave		= bond_add_slave,
>>+	.ndo_del_slave		= bond_del_slave,
>>+	.ndo_get_slave_count	= bond_get_slave_count,
>>+	.ndo_get_slave		= bond_get_slave,
>> };
>>
>> static void bond_destructor(struct net_device *bond_dev)
>>-- 
>>1.7.3.4
>>
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [patch net-next-2.6 3/4] bond: implement slave management operations

[Nicolas de Pesloüan]

Le 11/02/2011 16:22, Jiri Pirko a écrit :
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
> ---
>   drivers/net/bonding/bond_main.c |   38 ++++++++++++++++++++++++++++++++++++++
>   1 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1df9f0e..f8e59f9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4285,6 +4285,40 @@ unwind:
>   	return res;
>   }
>
> +static int bond_add_slave(struct net_device *bond_dev,
> +			  struct net_device *slave_dev)
> +{
> +	return bond_enslave(bond_dev, slave_dev);
> +}
> +
> +static int bond_del_slave(struct net_device *bond_dev,
> +			  struct net_device *slave_dev)
> +{
> +	return bond_release(bond_dev, slave_dev);
> +}
> +

Hi Jiri,

Why did you add another level of function nesting (bond_add_slave() and bond_del_slave()) instead of 
using bond_enslave() and bond_release() directly in the structure below ?

The function prototypes are identical.

Or may be, rename bond_enslave() to bond_add_slave() and bond_release() to bond_del_slave(), for 
consistency.

> +	.ndo_add_slave		= bond_add_slave,
> +	.ndo_del_slave		= bond_del_slave,
> +	.ndo_get_slave_count	= bond_get_slave_count,
> +	.ndo_get_slave		= bond_get_slave,

	Nicolas.

Re: [patch net-next-2.6 3/4] bond: implement slave management operations

[Jiri Pirko]

Sat, Feb 12, 2011 at 02:16:18PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 11/02/2011 16:22, Jiri Pirko a écrit :
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>---
>>  drivers/net/bonding/bond_main.c |   38 ++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 38 insertions(+), 0 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 1df9f0e..f8e59f9 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -4285,6 +4285,40 @@ unwind:
>>  	return res;
>>  }
>>
>>+static int bond_add_slave(struct net_device *bond_dev,
>>+			  struct net_device *slave_dev)
>>+{
>>+	return bond_enslave(bond_dev, slave_dev);
>>+}
>>+
>>+static int bond_del_slave(struct net_device *bond_dev,
>>+			  struct net_device *slave_dev)
>>+{
>>+	return bond_release(bond_dev, slave_dev);
>>+}
>>+
>
>Hi Jiri,
>
>Why did you add another level of function nesting (bond_add_slave()
>and bond_del_slave()) instead of using bond_enslave() and
>bond_release() directly in the structure below ?
>
>The function prototypes are identical.
>
>Or may be, rename bond_enslave() to bond_add_slave() and
>bond_release() to bond_del_slave(), for consistency.

Yes you are right - I did it in bonding by copy & paste code I did in
bridge and I didn't see that here it isn't necessary... Will remake this
in respin patchset.

Thanks.

Jirka
>
>>+	.ndo_add_slave		= bond_add_slave,
>>+	.ndo_del_slave		= bond_del_slave,
>>+	.ndo_get_slave_count	= bond_get_slave_count,
>>+	.ndo_get_slave		= bond_get_slave,
>
>	Nicolas.

Re: [patch net-next-2.6 4/4] bridge: implement slave management operations

[Stephen Hemminger]

On Fri, 11 Feb 2011 16:23:47 +0100
Jiri Pirko <jpirko@redhat.com> wrote:

> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

NAK
This does not belong in ethtool. It should be done by rt_netlink_ops.
I am working on doing that right now.


-- 

Re: [patch net-next-2.6 4/4] bridge: implement slave management operations

[Stephen Hemminger]

On Mon, 28 Feb 2011 14:17:29 -0800
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Fri, 11 Feb 2011 16:23:47 +0100
> Jiri Pirko <jpirko@redhat.com> wrote:
> 
> > 
> > Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> 

Never mind, this is good. I was confusing slave with bridge initialization
and netdevice ops with ethtool ops.

Ethtool ops seems to be growing like weeds lately...



-- 

Re: [patch net-next-2.6 4/4] bridge: implement slave management operations

[Jiri Pirko]

Mon, Feb 28, 2011 at 11:45:16PM CET, shemminger@vyatta.com wrote:
>On Mon, 28 Feb 2011 14:17:29 -0800
>Stephen Hemminger <shemminger@linux-foundation.org> wrote:
>
>> On Fri, 11 Feb 2011 16:23:47 +0100
>> Jiri Pirko <jpirko@redhat.com> wrote:
>> 
>> > 
>> > Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>> 
>
>Never mind, this is good. I was confusing slave with bridge initialization
>and netdevice ops with ethtool ops.
>
>Ethtool ops seems to be growing like weeds lately...


This was replaced by:
bridge: implement [add/del]_slave ops


never intended to be in ethtool
>
>
>
>--