On 5/6/15 2:36 PM, Pravin Shelar wrote:
On Tue, May 5, 2015 at 8:51 AM, Thomas F Herbert
<thomasfherb...@gmail.com> wrote:
Add support for 802.1ad including the ability to push and pop double
tagged vlans.

Signed-off-by: Thomas F Herbert <thomasfherb...@gmail.com>
---
>....
checkpatch.pl complains about blank lines.
Pravin, Thanks for your review.
Fixed in V9


....
I think we need to invalidate the flow key even when protocol is
8021Q. This is because packet is changed from double vlan
encapsulation to single. In this case vlan 8021Q TCI is mapped to
different key struct member.
Yes, good catch and thanks. this extra check in the pop and push actions wasn't necessary. The original push and pop code in net-next was already qinq-ready.

....
Same goes here, we need to invalidate flow key here.
See above.

         else
                 key->eth.tci = vlan->vlan_tci;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 2dacc7b..0430466 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -298,21 +298,79 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
  static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
...
+                       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+                                       sizeof(__be16)))
+                               return 0;
+
In case of error we need to clear key->eth.tci.
Done. Fixed in V9


Move local variable declaration to block where it is actually used.
Done. Fixed in V9


Can you move this vlan key parsing to separate function.
Good idea. In V9, Vlan to key parsing is now consolidated in a separate function.



I do not see key->eth.ctci mask set according to userspace mask attribute.
You could use ovs_nested_vlan_from_nlattrs().
Fixed in V9.



--
Thomas F. Herbert
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to