Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman <sfeldma@gmail.com>]

On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 06/02/15 03:10, Scott Feldman wrote:
>
>
>> Actually, we're now consistent with bridge man page which says master
>> is the default.
>>
>> Want we want, I believe, is to adjust what the man page says (and the
>> bridge vlan command itself), by making the default master and self.
>> The kernel and driver are fine, it's the default in the bridge command
>> that needs adjusting.  Once we do this, we'll be back to transparent
>> with software-only bridge.
>>
>
> Question to ask when looking at something of this nature:
> Will it work with no suprises if you used today's unmodified app?
> The default behavior shouldnt change and unfortunately it does here.

The default behavior does change, yes, but there shouldn't be any
surprises even if using today's unmodified app.  The reason why is no
in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
three drivers that have ndo_bridge_setlink use if to set hwmode to
VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
master to master|self, the bridge's
ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
driver's using ndo_vlan_rx_add_vid, and if they implement
ndo_bridge_setlink, they'll get called a second time but will noop
because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.

So it comes down to two choices:

1) break ABI, which is inconsequential for in-kernel drivers and
preserve (iproute2) command transparency, or

2) embrace existing behavior which is consistent with man pages but
breaks command transparency for any driver implementing
ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
can see the DSA going down this path also based on another concurrent
thread.

We're at option 2) right now.

> It is not just iproute2 - since this is breaking ABI expectations.
> Looking at some app i wrote a while back based on analyzing kernel
> expectations at the time, I see the following logic:
>
> user can set master or self on command line.
> ...
> ....
> if (user DID NOT set master_on || user set self on)
>    then set self to on
>
> iow, current behavior:
>  01: master is only set if user explicitly asked.
>  11: master|self when user explicitly sets both
>  10: self is on by default when the user doesnt specify anything
>  00: and the last option is to have none set which is not
>      possible since we have defaults.
>
> cheers,
> jamal
>
>
> So this is very similar to iproute2 - if nothing is set
> it defaults to self.

Ha, you're giving the behavior for "bridge fdb" command, where self is
the default.

For "bridge link" and "bridge vlan", the default is master.  The user
must explicitly specify "self" to act on the device side of the port.

It's unfortunate the iproute2 defaults aren't consistent between
commands.  Maybe someone knows the history here and can explain.

-scott