Paths

Table of Contentst

ifconfig: Improve bridge vlan filtering syntax
ClosedPublic
Actions

Authored by ivy on Sat, Aug 2, 5:43 PM.

Details

Reviewers

None

Group Reviewers

network
bridge

Commits

rGc41469a89f74: ifconfig: Improve bridge vlan filtering syntax

Summary

The current syntax to add an interface to a filtering bridge requires
repeating the interface name up to three times:

ifconfig bridge0 addm ix0 untagged ix0 10 tagged ix0 100-199

Since at least one of these options nearly always needs to be set,
this results in excessively verbose configuration.

Extend "addm" to support optional arguments, and add two arguments,
"untagged" and "tagged", which infer the interface name from the
addm command. Now the interface only has to be given once:

ifconfig bridge0 addm ix0 untagged 10 tagged 100-199

To avoid confusion with the existing untagged and tagged commands,
rename those to ifuntagged and iftagged.

In future, this syntax will make it possible to add an interface and
set its vlan configuration atomically (once the API supports that),
but switching to the new syntax now means we don't need to change it
after 15.0.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65934
Build 62817: arc lint + arc unit

Event Timeline

ivy created this revision.Sat, Aug 2, 5:43 PM

Herald added subscribers: ziaee, glebius, melifaro and 2 others. · View Herald TranscriptSat, Aug 2, 5:43 PM

ivy requested review of this revision.Sat, Aug 2, 5:43 PM

Harbormaster completed remote builds in B65931: Diff 159646.Sat, Aug 2, 5:44 PM

I like the syntax ifconfig bridge0 addm ix0 untagged 10 tagged 100-199, it is much clear what is intended.

test vlreq.bv_op instead of using inefficient __BIT_COUNT.

Harbormaster completed remote builds in B65934: Diff 159666.Sun, Aug 3, 2:09 AM

style

Harbormaster completed remote builds in B65935: Diff 159667.Sun, Aug 3, 2:33 AM

In D51707#1181110, @zlei wrote:

I like the syntax ifconfig bridge0 addm ix0 untagged 10 tagged 100-199, it is much clear what is intended.

i agree this syntax is better, but if you like it, would you consider accepting the review so i may land it? :-)

ivy added a reviewer: bridge.Sun, Aug 3, 10:24 PM

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Aug 5, 7:26 PM

Closed by commit rGc41469a89f74: ifconfig: Improve bridge vlan filtering syntax (authored by ivy). · Explain Why

This revision was automatically updated to reflect the committed changes.

ivy added a commit: rGc41469a89f74: ifconfig: Improve bridge vlan filtering syntax.

In D51707#1181289, @ivy wrote:

In D51707#1181110, @zlei wrote:

I like the syntax ifconfig bridge0 addm ix0 untagged 10 tagged 100-199, it is much clear what is intended.

i agree this syntax is better, but if you like it, would you consider accepting the review so i may land it? :-)

I have about one week AFK and no access to my dev machine ( I have to use the dev's machine, but not easy to test ), so I lost lots of your changes about the feature vlanfilter and can not speak too much on it.

Well, I think there's still room to improve the syntax. For example, I'd like to split the operation on bridge member into a sub command vlanfilter, rather than directly via iftagged. The main goal is to make the command clear and straight forward. So that the operation on the bridge itself goes like

ifconfig bridge0 addm em0
ifconfig bridge0 vlanfilter

and for the operation on bridge members I'd prefer

ifconfig bridge0 vlanfilter em0 untagged 10 [+-]?tagged 100-199

rather than

ifconfig bridge0 [+-]?iftagged em0 100-149

I'd suggest to discuss that on mailing list, thoughts ?

In D51707#1182237, @zlei wrote:

ifconfig bridge0 vlanfilter em0 untagged 10 [+-]?tagged 100-199

actually, i intend to make untagged option independent of vlan filtering, i.e. you will be able to set (if)untagged without enabling vlanfilter. this is useful for the side-effect of untagged which places incoming packets in a non-default VLAN, that has lots of use-cases that don't require vlan filtering itself (i.e., access control).

in that case i don't think it makes sense to make this a subcommand of vlanfilter, and doing that with tagged but not untagged would be somewhat inconsistent. i will probably do the same with ifvlanproto (again, the feature doesn't require vlan filtering).

that leaves (def)qinq and iftagged as the remaining vlanfilter-specific options. since these are both about limiting access and have no other effects, i think they should remain gated behind vlanfilter.

I'd suggest to discuss that on mailing list, thoughts ?

i was planning to send a mail to net@ asking for testing and feedback on syntax, etc. but i was putting it off until we'd landed the entire feature, which only happened earlier this week (in particular i've landed this diff because i wanted to get it in before code slush). we do still have some time to fiddle with this during code slush, so i might do that now.

btw, remember any syntax needs to support adding an interface and setting its vlan configuration at the same time, which is partly why i made this part of addm. for example consider this:

ifconfig bridge0 vlanfilter defuntagged 10
ifconfig bridge0 addm ix0
ifconfig bridge0 untagged ix0 20

the user intends ix0 to be on vlan 20, but briefly after being added, it will be on vlan 10 instead, which could leak packets. currently this is unavoidable due to the ioctl API, but the Netlink API will make it possible to configure both at once.