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

["Jörn Engel" <joern@purestorage.com>]

On Wed, Aug 10, 2016 at 05:58:38PM -0700, 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.

I was looking for a slightly more generic thing than "is this device
tun?".  Something along the lines of "is this device L3 only?".  We can
always introduce a new flag and have tun set the flag.  Naïve me thought
ARPHRD_NONE might already match what I was looking for.

But if such an approach causes problems for others, it is a non-starter.

> >> - 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.

Nit: Indentation is wrong (two tabs instead of one).

It should provide enough information for anyone that reads kernel messages.
Works for me.

[588380.721349] bond1: Adding slave tun0
[588380.721402] bond1: The slave device specified does not support setting the MAC address
[588380.721404] bond1: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac

Jörn