Re: [PATCH net-next 6/8] bonding: add arp_ip_target netlink support

[Jiri Pirko <jiri@resnulli.us>]

Thu, Nov 07, 2013 at 11:56:35AM CET, sfeldma@cumulusnetworks.com wrote:
>
>On Nov 7, 2013, at 12:16 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Thu, Nov 07, 2013 at 10:43:30AM CET, sfeldma@cumulusnetworks.com wrote:
>>> Add IFLA_BOND_ARP_IP_TARGET to allow get/set of bonding parameter
>>> arp_ip_target via netlink.
>>> 
>>> Signed-off-by: Scott Feldman <sfeldma@cumulusnetworks.com>
>>> ---
>>> drivers/net/bonding/bond_netlink.c |   43 ++++++++++++++-
>>> drivers/net/bonding/bond_options.c |  102 ++++++++++++++++++++++++++++++++++++
>>> drivers/net/bonding/bond_sysfs.c   |   77 +++------------------------
>>> drivers/net/bonding/bonding.h      |    3 +
>>> include/uapi/linux/if_link.h       |    1 
>>> 5 files changed, 155 insertions(+), 71 deletions(-)
>>> 
>>> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>>> index 9c50165..e4673ba 100644
>>> --- a/drivers/net/bonding/bond_netlink.c
>>> +++ b/drivers/net/bonding/bond_netlink.c
>>> @@ -29,6 +29,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
>>> 	[IFLA_BOND_DOWNDELAY]		= { .type = NLA_U32 },
>>> 	[IFLA_BOND_USE_CARRIER]		= { .type = NLA_U8 },
>>> 	[IFLA_BOND_ARP_INTERVAL]	= { .type = NLA_U32 },
>>> +	[IFLA_BOND_ARP_IP_TARGET]	= { .type = NLA_NESTED },
>>> };
>>> 
>>> static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
>>> @@ -109,6 +110,21 @@ static int bond_changelink(struct net_device *bond_dev,
>>> 		if (err)
>>> 			return err;
>>> 	}
>>> +	if (data[IFLA_BOND_ARP_IP_TARGET]) {
>>> +		struct nlattr *attr;
>>> +		int rem;
>>> +
>>> +		err = bond_option_arp_ip_target_clear_all(bond);
>>> +		if (err)
>>> +			return err;
>>> +
>> 
>> Hmm. I think this is not correct. I think you should not clear all. They
>> might be needed to be used but for a brief moment, they disappear.
>> 
>> Just set or unset them one by one.
>
>That’s a good catch.  I should compare existing set with new set and retain entries entries common to both sets (and add/remove uncommon ones).
>
>Just to be clear, for netlink, the idea is to push a set of ip targets addrs as a nested attribute and process as a set.  This is consistent with arp_ip_target documentation in Documentation/networking/bonding.txt where arp_ip_target is passed as a comma-separated list of addrs.  However, it’s not consistent with the sysfs interface where addrs are added/removed one at a time using “+” or “-“ prefix.

I believe that the behaviour you described is ok.

>
>e.g.:
>
>ip link add bond7 type bond mode 1
>ip link set dev bond7 type bond arp_ip_target 10.0.0.1,10.0.0.5
>ip link set dev bond7 type bond arp_ip_target 10.0.0.1
>
>
>>> @@ -141,6 +159,8 @@ static int bond_fill_info(struct sk_buff *skb,
>>> {
>>> 	struct bonding *bond = netdev_priv(bond_dev);
>>> 	struct net_device *slave_dev = bond_option_active_slave_get(bond);
>>> +	struct nlattr *targets;
>>> +	int i, targets_added;
>>> 
>>> 	if (nla_put_u8(skb, IFLA_BOND_MODE, bond->params.mode))
>>> 		goto nla_put_failure;
>>> @@ -152,10 +172,12 @@ static int bond_fill_info(struct sk_buff *skb,
>>> 	if (nla_put_u32(skb, IFLA_BOND_MIIMON, bond->params.miimon))
>>> 		goto nla_put_failure;
>>> 
>>> -	if (nla_put_u32(skb, IFLA_BOND_UPDELAY, bond->params.updelay))
>>> +	if (nla_put_u32(skb, IFLA_BOND_UPDELAY,
>>> +		bond->params.updelay * bond->params.miimon))
>>> 		goto nla_put_failure;
>> 
>> 	This bit should probably no be here :)
>
>Drats, that change goes with earlier patch.  I’ll fix on patch set resend for issue above.
>
>-scott