From: Ding Tianhong <dingtianhong@huawei•com>
To: "Jörn Engel" <joern@purestorage•com>,
"Jay Vosburgh" <jay.vosburgh@canonical•com>
Cc: David Miller <davem@davemloft•net>, <zyjzyj2000@gmail•com>,
<andy@greyhouse•net>, <netdev@vger•kernel.org>
Subject: Re: [PATCH] bonding: Allow tun-interfaces as slaves
Date: Tue, 30 Aug 2016 09:44:36 +0800 [thread overview]
Message-ID: <57C4E504.1010801@huawei.com> (raw)
In-Reply-To: <20160811182418.GC25877@cork>
On 2016/8/12 2:24, Jörn Engel wrote:
> 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.
>
I think there is no such flag to distinguish the tun device, if you insistent on to support new flag
for Tun device, you could send a patch and we could review it, otherwise BOND_FOM_KEEPMAC is enough to
fix this problem.
Thanks
Ding
> 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
>
> .
>
next prev parent reply other threads:[~2016-08-30 1:44 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20160808211530.GH22974@cork>
2016-08-08 21:21 ` [Regression] Bonding no longer support tun-interfaces Jörn Engel
2016-08-08 21:48 ` [PATCH] bonding: Allow tun-interfaces as slaves Jörn Engel
2016-08-09 2:18 ` Ding Tianhong
2016-08-09 3:09 ` Jörn Engel
2016-08-09 5:29 ` zhuyj
2016-08-09 13:28 ` Ding Tianhong
2016-08-09 18:08 ` Jörn Engel
2016-08-09 19:06 ` David Miller
2016-08-09 21:10 ` Jörn Engel
2016-08-09 23:51 ` Jay Vosburgh
2016-08-10 1:06 ` Ding Tianhong
2016-08-10 9:27 ` Ding Tianhong
2016-08-10 17:41 ` Jay Vosburgh
2016-08-11 1:20 ` Ding Tianhong
2016-08-10 21:26 ` Jörn Engel
2016-08-10 22:00 ` Jörn Engel
2016-08-11 0:58 ` Jay Vosburgh
2016-08-11 1:37 ` Ding Tianhong
2016-08-11 18:24 ` Jörn Engel
2016-08-29 22:49 ` Jörn Engel
2016-08-30 1:44 ` Ding Tianhong [this message]
2016-08-09 5:52 ` Ding Tianhong
2016-08-09 18:21 ` Jay Vosburgh
2016-08-09 18:40 ` Jörn Engel
2016-08-09 19:10 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57C4E504.1010801@huawei.com \
--to=dingtianhong@huawei$(echo .)com \
--cc=andy@greyhouse$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=jay.vosburgh@canonical$(echo .)com \
--cc=joern@purestorage$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=zyjzyj2000@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox