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

[Jay Vosburgh <jay.vosburgh@canonical.com>]

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.

	-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