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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 11 Feb 2011 18:51:40 +0100
From:	Jiri Pirko <jpirko@...hat.com>
To:	Jay Vosburgh <fubar@...ibm.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	shemminger@...ux-foundation.org, kaber@...sh.net
Subject: Re: [patch net-next-2.6 3/4] bond: implement slave management
 operations

Fri, Feb 11, 2011 at 06:19:50PM CET, fubar@...ibm.com wrote:
>Jiri Pirko <jpirko@...hat.com> wrote:
>
>>Signed-off-by: Jiri Pirko <jpirko@...hat.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@...ibm.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html