Re: [PATCH] bonding: Allow tun-interfaces as slaves

[Ding Tianhong <dingtianhong@huawei.com>]

On 2016/8/11 8:58, Jay Vosburgh wrote:
> Jörn Engel <joern@purestorage.com> wrote:
> 
>> On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
>>>
>>> Having to set one more parameter is a bit annoying.  It would have to be
>>> documented in a prominent place and people would still often miss it.
>>> So I wonder if we can make the interface a little nicer.
>>>
>>> Options:
>>> - If there are no slaves yet and the first slave added is tun, we trust
>>>   the users to know what they are doing.  Automatically set
>>>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
>>>   Maybe do a printk to inform the user in case of a mistake.
> 
> 	I don't think this is feasible, as I don't see a reliable way to
> test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
> cannot check the ops as they are not statically built into the kernel).
> I'm also not sure that heuristics are the proper way to enable this
> functionality in general.
> 
>>> - If we get an error and the slave device is tun, do a printk giving the
>>>   user enough information to find this parameter.
> 
> 	This could probably be done as a change the existing logic, e.g.,
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..019c1a689aae 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  				res = -EOPNOTSUPP;
>  				goto err_undo_flags;
>  			}
> +		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
> +				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
>  		}
>  	}
>  
> 	I haven't tested this, and I'm not sure it will get all corner
> cases correct, but this should basically cover it.
> 

Looks fine to cover the case, but if we still let it pass, I am not sure it is suitable.

> 	-J
> 
>>> I'm leaning towards the former, but you probably know a reason why I am
>>> wrong again.
>>
>> Patch below is an implementation of the former.  Not sure if something
>> like this is worth considering.
>>
>> Jörn
>>
>> --
>> To announce that there must be no criticism of the President, or that we
>> are to stand by the President, right or wrong, is not only unpatriotic
>> and servile, but is morally treasonable to the American public.
>> -- Theodore Roosevelt, Kansas City Star, 1918
>>
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1f276fa30ba6..306909a44fab 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1482,8 +1482,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> 	 */
>> 	ether_addr_copy(new_slave->perm_hwaddr, slave_dev->dev_addr);
>>
>> -	if (!bond->params.fail_over_mac ||
>> -	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> +	if (bond_dev->type != ARPHRD_NONE &&
>> +	    (!bond->params.fail_over_mac ||
>> +	     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
>> 		/* Set slave to master's mac address.  The application already
>> 		 * set the master's mac address to that of the first slave
>> 		 */
>> -- 
>> 2.1.4
>>
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
> 
> .
>