mbox series

[ovs-dev,0/2] Add include mode to priority tags port option

Message ID 20190508073959.2364-1-elibr@mellanox.com
Headers show
Series Add include mode to priority tags port option | expand

Message

Eli Britstein May 8, 2019, 7:39 a.m. UTC 
Setting priority-tags to "true" Open vSwitch still omits the
802.1Q header on output if both the VLAN ID and priority would be zero.
Add an option to keep the 8021Q header for such frames as well.

Patch #1: change boolean to enum as a pre-step to adding addition option
Patch #2: add "include" mode for priority-tags configuration

Eli Britstein (2):
  ofproto-dpif-xlate: Change priority tags from boolean to enum
  ofproto-dpif-xlate: Add include mode to priority tags

 ofproto/ofproto-dpif-xlate.c | 20 ++++++++++++--------
 ofproto/ofproto-dpif-xlate.h |  2 +-
 ofproto/ofproto-dpif.c       |  3 ++-
 ofproto/ofproto.h            | 11 ++++++++++-
 tests/ofproto-dpif.at        |  6 +++---
 vswitchd/bridge.c            | 10 ++++++++--
 vswitchd/vswitch.xml         |  9 +++++----
 7 files changed, 41 insertions(+), 20 deletions(-)

Comments

Ben Pfaff May 10, 2019, 4:29 a.m. UTC | #1 
On Wed, May 08, 2019 at 07:39:57AM +0000, Eli Britstein wrote:
> Setting priority-tags to "true" Open vSwitch still omits the
> 802.1Q header on output if both the VLAN ID and priority would be zero.
> Add an option to keep the 8021Q header for such frames as well.
> 
> Patch #1: change boolean to enum as a pre-step to adding addition option
> Patch #2: add "include" mode for priority-tags configuration

This series seems very reasonable to me.

I could quibble with the enumerated naming choices.  For example,
"always" and "never" plus "if-needed" or "if-nonzero" seem like good
names too.  But there is not really anything wrong with the current
choices either.

Patch 2 should add an item to NEWS.

This bit of documentation could be improved: I guess it really means
"retain the 802.1Q header" in such frames, not "keep":

          For <code>include-non-zero</code> Open vSwitch omits the
          802.1Q header on output if both the VLAN ID and priority would
          be zero. Set to <code>include</code> to keep such frames as
          well.

I'm happy with it otherwise.  Thank you!  I'll look forward to v2.

Thanks,

Ben.