On 2016/8/11 8:58, 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. > >>> - 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.vosbu...@canonical.com > > . >