On Wed, Aug 10, 2016 at 05:58:38PM -0700, Jay Vosburgh wrote:
> Jörn Engel <jo...@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
--
It is easier to lead men to combat, stirring up their passion, than to
restrain them and direct them toward the patient labours of peace.
-- Andre Gide
