[PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Scott Feldman]

If link is IFF_SLAVE, extend link dev netlink attributes to include
slave attributes with new IFLA_SLAVE nest.  Add netlink notification
(RTM_NEWLINK) when slave status changes from backup to active, or
visa-versa.

Adds new ndo_get_slave op to net_device_ops to fill skb with IFLA_SLAVE
attributes.  Currently only used by bonding driver, but could be
used by other aggregating devices with slaves.

Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
---
 drivers/net/bonding/bond_main.c    |    1 +
 drivers/net/bonding/bond_netlink.c |   36 ++++++++++++++++++++++++
 drivers/net/bonding/bonding.h      |   11 ++++++-
 include/linux/netdevice.h          |    5 +++
 include/uapi/linux/if_link.h       |   13 +++++++++
 net/core/rtnetlink.c               |   54 ++++++++++++++++++++++++++++++++++++
 6 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index df85cec..3220b48 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3883,6 +3883,7 @@ static const struct net_device_ops bond_netdev_ops = {
 #endif
 	.ndo_add_slave		= bond_enslave,
 	.ndo_del_slave		= bond_release,
+	.ndo_get_slave		= bond_get_slave,
 	.ndo_fix_features	= bond_fix_features,
 };
 
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 555c783..21c6488 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -22,6 +22,42 @@
 #include <linux/reciprocal_div.h>
 #include "bonding.h"
 
+int bond_get_slave(struct net_device *slave_dev, struct sk_buff *skb)
+{
+	struct slave *slave = bond_slave_get_rtnl(slave_dev);
+	const struct aggregator *agg;
+
+	if (nla_put_u8(skb, IFLA_SLAVE_STATE, bond_slave_state(slave)))
+		goto nla_put_failure;
+
+	if (nla_put_u8(skb, IFLA_SLAVE_MII_STATUS, slave->link))
+		goto nla_put_failure;
+
+	if (nla_put_u32(skb, IFLA_SLAVE_LINK_FAILURE_COUNT,
+			slave->link_failure_count))
+		goto nla_put_failure;
+
+	if (nla_put(skb, IFLA_SLAVE_PERM_HWADDR,
+		    slave_dev->addr_len, slave->perm_hwaddr))
+		goto nla_put_failure;
+
+	if (nla_put_u16(skb, IFLA_SLAVE_QUEUE_ID, slave->queue_id))
+		goto nla_put_failure;
+
+	if (slave->bond->params.mode == BOND_MODE_8023AD) {
+		agg = SLAVE_AD_INFO(slave).port.aggregator;
+		if (agg)
+			if (nla_put_u16(skb, IFLA_SLAVE_AD_AGGREGATOR_ID,
+					agg->aggregator_identifier))
+				goto nla_put_failure;
+	}
+
+	return 0;
+
+nla_put_failure:
+	return -EMSGSIZE;
+}
+
 static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
 	[IFLA_BOND_MODE]		= { .type = NLA_U8 },
 	[IFLA_BOND_ACTIVE_SLAVE]	= { .type = NLA_U32 },
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 309757d..8a935f8 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -285,12 +285,18 @@ static inline bool bond_is_lb(const struct bonding *bond)
 
 static inline void bond_set_active_slave(struct slave *slave)
 {
-	slave->backup = 0;
+	if (slave->backup) {
+		slave->backup = 0;
+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
+	}
 }
 
 static inline void bond_set_backup_slave(struct slave *slave)
 {
-	slave->backup = 1;
+	if (!slave->backup) {
+		slave->backup = 1;
+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
+	}
 }
 
 static inline int bond_slave_state(struct slave *slave)
@@ -426,6 +432,7 @@ int bond_sysfs_slave_add(struct slave *slave);
 void bond_sysfs_slave_del(struct slave *slave);
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
+int bond_get_slave(struct net_device *slave_dev, struct sk_buff *skb);
 int bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, int count);
 int bond_parse_parm(const char *mode_arg, const struct bond_parm_tbl *tbl);
 int bond_parm_tbl_lookup(int mode, const struct bond_parm_tbl *tbl);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d7668b88..22c8698 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -908,6 +908,9 @@ struct netdev_phys_port_id {
  * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
  *	Called to release previously enslaved netdev.
  *
+ * int (*ndo_get_slave)(struct net_device *slave_dev, struct sk_buff *skb);
+ *	Called to fill netlink skb with slave info.
+ *
  *      Feature/offload setting functions.
  * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
  *		netdev_features_t features);
@@ -1080,6 +1083,8 @@ struct net_device_ops {
 						 struct net_device *slave_dev);
 	int			(*ndo_del_slave)(struct net_device *dev,
 						 struct net_device *slave_dev);
+	int			(*ndo_get_slave)(struct net_device *slave_dev,
+						 struct sk_buff *skb);
 	netdev_features_t	(*ndo_fix_features)(struct net_device *dev,
 						    netdev_features_t features);
 	int			(*ndo_set_features)(struct net_device *dev,
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 3e6bd3c..ba2f3bf 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -144,6 +144,7 @@ enum {
 	IFLA_NUM_RX_QUEUES,
 	IFLA_CARRIER,
 	IFLA_PHYS_PORT_ID,
+	IFLA_SLAVE,
 	__IFLA_MAX
 };
 
@@ -368,6 +369,18 @@ enum {
 
 #define IFLA_BOND_AD_INFO_MAX	(__IFLA_BOND_AD_INFO_MAX - 1)
 
+enum {
+	IFLA_SLAVE_STATE,
+	IFLA_SLAVE_MII_STATUS,
+	IFLA_SLAVE_LINK_FAILURE_COUNT,
+	IFLA_SLAVE_PERM_HWADDR,
+	IFLA_SLAVE_QUEUE_ID,
+	IFLA_SLAVE_AD_AGGREGATOR_ID,
+	__IFLA_SLAVE_MAX,
+};
+
+#define IFLA_SLAVE_MAX	(__IFLA_SLAVE_MAX - 1)
+
 /* SR-IOV virtual function management section */
 
 enum {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e6e7d58..4f85de7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -721,6 +721,28 @@ static size_t rtnl_port_size(const struct net_device *dev)
 		return port_self_size;
 }
 
+static size_t rtnl_bond_slave_size(const struct net_device *dev)
+{
+	struct net_device *bond;
+	size_t slave_size =
+		nla_total_size(sizeof(struct nlattr)) +	/* IFLA_SLAVE */
+		nla_total_size(1) +	/* IFLA_SLAVE_STATE */
+		nla_total_size(1) +	/* IFLA_SLAVE_MII_STATUS */
+		nla_total_size(4) +	/* IFLA_SLAVE_LINK_FAILURE_COUNT */
+		nla_total_size(MAX_ADDR_LEN) +	/* IFLA_SLAVE_PERM_HWADDR */
+		nla_total_size(2) +	/* IFLA_SLAVE_QUEUE_ID */
+		nla_total_size(2) +	/* IFLA_SLAVE_AD_AGGREGATOR_ID */
+		0;
+
+	if (netif_is_bond_slave((struct net_device *)dev)) {
+		bond = netdev_master_upper_dev_get((struct net_device *)dev);
+		if (bond && bond->netdev_ops->ndo_get_slave)
+			return slave_size;
+	}
+
+	return 0;
+}
+
 static noinline size_t if_nlmsg_size(const struct net_device *dev,
 				     u32 ext_filter_mask)
 {
@@ -750,6 +772,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
 	       + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
 	       + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */
+	       + rtnl_bond_slave_size(dev) /* IFLA_SLAVE */
 	       + nla_total_size(MAX_PHYS_PORT_ID_LEN); /* IFLA_PHYS_PORT_ID */
 }
 
@@ -847,6 +870,34 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static size_t rtnl_bond_slave_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	struct net_device *bond;
+	struct nlattr *nest;
+	int err;
+
+	if (!netif_is_bond_slave(dev))
+		return 0;
+
+	bond = netdev_master_upper_dev_get(dev);
+	if (!bond || !bond->netdev_ops->ndo_get_slave)
+		return 0;
+
+	nest = nla_nest_start(skb, IFLA_SLAVE);
+	if (!nest)
+		return -EMSGSIZE;
+
+	err = bond->netdev_ops->ndo_get_slave(dev, skb);
+	if (err) {
+		nla_nest_cancel(skb, nest);
+		return (err == -EMSGSIZE) ? err : 0;
+	}
+
+	nla_nest_end(skb, nest);
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask)
@@ -1001,6 +1052,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_bond_slave_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (dev->rtnl_link_ops) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;

Re: [PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Jiri Pirko]

Fri, Jan 17, 2014 at 07:57:56AM CET, sfeldma@cumulusnetworks.com wrote:
>If link is IFF_SLAVE, extend link dev netlink attributes to include
>slave attributes with new IFLA_SLAVE nest.  Add netlink notification
>(RTM_NEWLINK) when slave status changes from backup to active, or
>visa-versa.
>
>Adds new ndo_get_slave op to net_device_ops to fill skb with IFLA_SLAVE
>attributes.  Currently only used by bonding driver, but could be
>used by other aggregating devices with slaves.
>
>Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
>---
> drivers/net/bonding/bond_main.c    |    1 +
> drivers/net/bonding/bond_netlink.c |   36 ++++++++++++++++++++++++
> drivers/net/bonding/bonding.h      |   11 ++++++-
> include/linux/netdevice.h          |    5 +++
> include/uapi/linux/if_link.h       |   13 +++++++++
> net/core/rtnetlink.c               |   54 ++++++++++++++++++++++++++++++++++++
> 6 files changed, 118 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index df85cec..3220b48 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3883,6 +3883,7 @@ static const struct net_device_ops bond_netdev_ops = {
> #endif
> 	.ndo_add_slave		= bond_enslave,
> 	.ndo_del_slave		= bond_release,
>+	.ndo_get_slave		= bond_get_slave,
> 	.ndo_fix_features	= bond_fix_features,
> };
> 
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 555c783..21c6488 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -22,6 +22,42 @@
> #include <linux/reciprocal_div.h>
> #include "bonding.h"
> 
>+int bond_get_slave(struct net_device *slave_dev, struct sk_buff *skb)
>+{
>+	struct slave *slave = bond_slave_get_rtnl(slave_dev);
>+	const struct aggregator *agg;
>+
>+	if (nla_put_u8(skb, IFLA_SLAVE_STATE, bond_slave_state(slave)))
>+		goto nla_put_failure;
>+
>+	if (nla_put_u8(skb, IFLA_SLAVE_MII_STATUS, slave->link))
>+		goto nla_put_failure;
>+
>+	if (nla_put_u32(skb, IFLA_SLAVE_LINK_FAILURE_COUNT,
>+			slave->link_failure_count))
>+		goto nla_put_failure;
>+
>+	if (nla_put(skb, IFLA_SLAVE_PERM_HWADDR,
>+		    slave_dev->addr_len, slave->perm_hwaddr))
>+		goto nla_put_failure;
>+
>+	if (nla_put_u16(skb, IFLA_SLAVE_QUEUE_ID, slave->queue_id))
>+		goto nla_put_failure;
>+
>+	if (slave->bond->params.mode == BOND_MODE_8023AD) {
>+		agg = SLAVE_AD_INFO(slave).port.aggregator;
>+		if (agg)
>+			if (nla_put_u16(skb, IFLA_SLAVE_AD_AGGREGATOR_ID,
>+					agg->aggregator_identifier))
>+				goto nla_put_failure;
>+	}
>+
>+	return 0;
>+
>+nla_put_failure:
>+	return -EMSGSIZE;
>+}
>+
> static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> 	[IFLA_BOND_MODE]		= { .type = NLA_U8 },
> 	[IFLA_BOND_ACTIVE_SLAVE]	= { .type = NLA_U32 },
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 309757d..8a935f8 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -285,12 +285,18 @@ static inline bool bond_is_lb(const struct bonding *bond)
> 
> static inline void bond_set_active_slave(struct slave *slave)
> {
>-	slave->backup = 0;
>+	if (slave->backup) {
>+		slave->backup = 0;
>+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
>+	}
> }
> 
> static inline void bond_set_backup_slave(struct slave *slave)
> {
>-	slave->backup = 1;
>+	if (!slave->backup) {
>+		slave->backup = 1;
>+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
>+	}
> }
> 
> static inline int bond_slave_state(struct slave *slave)
>@@ -426,6 +432,7 @@ int bond_sysfs_slave_add(struct slave *slave);
> void bond_sysfs_slave_del(struct slave *slave);
> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
> int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
>+int bond_get_slave(struct net_device *slave_dev, struct sk_buff *skb);
> int bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, int count);
> int bond_parse_parm(const char *mode_arg, const struct bond_parm_tbl *tbl);
> int bond_parm_tbl_lookup(int mode, const struct bond_parm_tbl *tbl);
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index d7668b88..22c8698 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -908,6 +908,9 @@ struct netdev_phys_port_id {
>  * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
>  *	Called to release previously enslaved netdev.
>  *
>+ * int (*ndo_get_slave)(struct net_device *slave_dev, struct sk_buff *skb);
>+ *	Called to fill netlink skb with slave info.
>+ *
>  *      Feature/offload setting functions.
>  * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
>  *		netdev_features_t features);
>@@ -1080,6 +1083,8 @@ struct net_device_ops {
> 						 struct net_device *slave_dev);
> 	int			(*ndo_del_slave)(struct net_device *dev,
> 						 struct net_device *slave_dev);
>+	int			(*ndo_get_slave)(struct net_device *slave_dev,
>+						 struct sk_buff *skb);
> 	netdev_features_t	(*ndo_fix_features)(struct net_device *dev,
> 						    netdev_features_t features);
> 	int			(*ndo_set_features)(struct net_device *dev,
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index 3e6bd3c..ba2f3bf 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -144,6 +144,7 @@ enum {
> 	IFLA_NUM_RX_QUEUES,
> 	IFLA_CARRIER,
> 	IFLA_PHYS_PORT_ID,
>+	IFLA_SLAVE,
> 	__IFLA_MAX
> };
> 
>@@ -368,6 +369,18 @@ enum {
> 
> #define IFLA_BOND_AD_INFO_MAX	(__IFLA_BOND_AD_INFO_MAX - 1)
> 
>+enum {
>+	IFLA_SLAVE_STATE,
>+	IFLA_SLAVE_MII_STATUS,
>+	IFLA_SLAVE_LINK_FAILURE_COUNT,
>+	IFLA_SLAVE_PERM_HWADDR,
>+	IFLA_SLAVE_QUEUE_ID,
>+	IFLA_SLAVE_AD_AGGREGATOR_ID,
>+	__IFLA_SLAVE_MAX,
>+};


The names are not right in my opinion. "BOND" should be mentioned there.

>+
>+#define IFLA_SLAVE_MAX	(__IFLA_SLAVE_MAX - 1)
>+
> /* SR-IOV virtual function management section */
> 
> enum {
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index e6e7d58..4f85de7 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -721,6 +721,28 @@ static size_t rtnl_port_size(const struct net_device *dev)
> 		return port_self_size;
> }
> 
>+static size_t rtnl_bond_slave_size(const struct net_device *dev)
>+{
>+	struct net_device *bond;
>+	size_t slave_size =
>+		nla_total_size(sizeof(struct nlattr)) +	/* IFLA_SLAVE */
>+		nla_total_size(1) +	/* IFLA_SLAVE_STATE */
>+		nla_total_size(1) +	/* IFLA_SLAVE_MII_STATUS */
>+		nla_total_size(4) +	/* IFLA_SLAVE_LINK_FAILURE_COUNT */
>+		nla_total_size(MAX_ADDR_LEN) +	/* IFLA_SLAVE_PERM_HWADDR */
>+		nla_total_size(2) +	/* IFLA_SLAVE_QUEUE_ID */
>+		nla_total_size(2) +	/* IFLA_SLAVE_AD_AGGREGATOR_ID */
>+		0;
>+
>+	if (netif_is_bond_slave((struct net_device *)dev)) {
>+		bond = netdev_master_upper_dev_get((struct net_device *)dev);
>+		if (bond && bond->netdev_ops->ndo_get_slave)
>+			return slave_size;
>+	}
>+
>+	return 0;
>+}
>+
> static noinline size_t if_nlmsg_size(const struct net_device *dev,
> 				     u32 ext_filter_mask)
> {
>@@ -750,6 +772,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
> 	       + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
> 	       + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
> 	       + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */
>+	       + rtnl_bond_slave_size(dev) /* IFLA_SLAVE */
> 	       + nla_total_size(MAX_PHYS_PORT_ID_LEN); /* IFLA_PHYS_PORT_ID */
> }
> 
>@@ -847,6 +870,34 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
> 	return 0;
> }
> 
>+static size_t rtnl_bond_slave_fill(struct sk_buff *skb, struct net_device *dev)
>+{
>+	struct net_device *bond;
>+	struct nlattr *nest;
>+	int err;
>+
>+	if (!netif_is_bond_slave(dev))
>+		return 0;
>+
>+	bond = netdev_master_upper_dev_get(dev);
>+	if (!bond || !bond->netdev_ops->ndo_get_slave)
>+		return 0;
>+
>+	nest = nla_nest_start(skb, IFLA_SLAVE);
>+	if (!nest)
>+		return -EMSGSIZE;
>+
>+	err = bond->netdev_ops->ndo_get_slave(dev, skb);
>+	if (err) {
>+		nla_nest_cancel(skb, nest);
>+		return (err == -EMSGSIZE) ? err : 0;
>+	}
>+
>+	nla_nest_end(skb, nest);
>+
>+	return 0;
>+}
>+
> static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> 			    int type, u32 pid, u32 seq, u32 change,
> 			    unsigned int flags, u32 ext_filter_mask)
>@@ -1001,6 +1052,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_bond_slave_fill(skb, dev))
>+		goto nla_put_failure;
>+

I must say I do not like this at all. This should be done in a generic
way. By a callback registered by bonding and possibly other master-slave
device types.

I have something in mind, will try to prepare patch soon.

Jiri


> 	if (dev->rtnl_link_ops) {
> 		if (rtnl_link_fill(skb, dev) < 0)
> 			goto nla_put_failure;
>
>--
>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

Re: [PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Scott Feldman]

On Jan 21, 2014, at 5:34 AM, Jiri Pirko <jiri@resnulli.us> wrote:

>> +	if (rtnl_bond_slave_fill(skb, dev))
>> +		goto nla_put_failure;
>> +
> 
> I must say I do not like this at all. This should be done in a generic
> way. By a callback registered by bonding and possibly other master-slave
> device types.

The bond was registered with the ndo_get_slave op.  ndo_get_slave could be used for other master-slave device types.  I’ll agree that rtnl_bond_slave_fill() could have been written more generically.  Is that the objection?

> I have something in mind, will try to prepare patch soon.

Re: [PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Jiri Pirko]

Tue, Jan 21, 2014 at 10:36:58PM CET, sfeldma@cumulusnetworks.com wrote:
>
>On Jan 21, 2014, at 5:34 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
>>> +	if (rtnl_bond_slave_fill(skb, dev))
>>> +		goto nla_put_failure;
>>> +
>> 
>> I must say I do not like this at all. This should be done in a generic
>> way. By a callback registered by bonding and possibly other master-slave
>> device types.
>
>The bond was registered with the ndo_get_slave op.  ndo_get_slave could be used for other master-slave device types.  I’ll agree that rtnl_bond_slave_fill() could have been written more generically.  Is that the objection?

I think is should be done rather in rtnl_link_ops. It's the natural point
for this ops. I have patchset prepared. Will send it very soon.

>
>> I have something in mind, will try to prepare patch soon.
>
>

Re: [PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Scott Feldman]

On Jan 21, 2014, at 2:00 PM, Jiri Pirko <jiri@resnulli.us> wrote:

> Tue, Jan 21, 2014 at 10:36:58PM CET, sfeldma@cumulusnetworks.com wrote:
>> 
>> On Jan 21, 2014, at 5:34 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> 
>>>> +	if (rtnl_bond_slave_fill(skb, dev))
>>>> +		goto nla_put_failure;
>>>> +
>>> 
>>> I must say I do not like this at all. This should be done in a generic
>>> way. By a callback registered by bonding and possibly other master-slave
>>> device types.
>> 
>> The bond was registered with the ndo_get_slave op.  ndo_get_slave could be used for other master-slave device types.  I’ll agree that rtnl_bond_slave_fill() could have been written more generically.  Is that the objection?
> 
> I think is should be done rather in rtnl_link_ops. It's the natural point
> for this ops. I have patchset prepared. Will send it very soon.

Ok, cool.

Also, right now I have IFLA_SLAVE as a nest for IFLA_SLAVE_xxx attrs.  Do you think we should have a two-layer nest so we can capture other master-slave devices rather than just bond slaves?  I.e.:

	IFLA_SLAVE
		IFLA_BOND_SLAVE
			IFLA_BOND_SLAVE_xxx
			IFLA_BOND_SLAVE_yyy
			IFLA_BOND_SLAVE_zzz
		IFLA_FOO_SLAVE			// FOO is some other non-bond master
			IFLA_FOO_SLAVE_xxx
			IFLA_FOO_SLAVE_yyy
			IFLA_FOO_SLAVE_zzz

(Of course, slave wouldn’t be bond and foo slave at same time).

-scott

Re: [PATCH net-next v2 2/2] bonding: add netlink attributes to slave link dev

[Jiri Pirko]

Tue, Jan 21, 2014 at 11:42:58PM CET, sfeldma@cumulusnetworks.com wrote:
>
>On Jan 21, 2014, at 2:00 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Tue, Jan 21, 2014 at 10:36:58PM CET, sfeldma@cumulusnetworks.com wrote:
>>> 
>>> On Jan 21, 2014, at 5:34 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> 
>>>>> +	if (rtnl_bond_slave_fill(skb, dev))
>>>>> +		goto nla_put_failure;
>>>>> +
>>>> 
>>>> I must say I do not like this at all. This should be done in a generic
>>>> way. By a callback registered by bonding and possibly other master-slave
>>>> device types.
>>> 
>>> The bond was registered with the ndo_get_slave op.  ndo_get_slave could be used for other master-slave device types.  I’ll agree that rtnl_bond_slave_fill() could have been written more generically.  Is that the objection?
>> 
>> I think is should be done rather in rtnl_link_ops. It's the natural point
>> for this ops. I have patchset prepared. Will send it very soon.
>
>Ok, cool.
>
>Also, right now I have IFLA_SLAVE as a nest for IFLA_SLAVE_xxx attrs.  Do you think we should have a two-layer nest so we can capture other master-slave devices rather than just bond slaves?  I.e.:
>
>	IFLA_SLAVE
>		IFLA_BOND_SLAVE
>			IFLA_BOND_SLAVE_xxx
>			IFLA_BOND_SLAVE_yyy
>			IFLA_BOND_SLAVE_zzz
>		IFLA_FOO_SLAVE			// FOO is some other non-bond master
>			IFLA_FOO_SLAVE_xxx
>			IFLA_FOO_SLAVE_yyy
>			IFLA_FOO_SLAVE_zzz
>
>(Of course, slave wouldn’t be bond and foo slave at same time).

I would rather do this in LINKINFO nest the same way IFLA_BOND_* are
done. Please see following patch:

http://patchwork.ozlabs.org/patch/313156/

>
>-scott