Flavio,
Thanks for your review. I am preparing a version 6 of the patch in which
I will be fixing the issues you pointed out but I am interested in
further guidance in your responses below.
Thanks, Tom
On 12/30/14, 3:30 PM, Flavio Leitner wrote:
On Tuesday, December 30, 2014 10:12:59 AM you wrote:
This is the linux kernel datapath portion of the 802.1AD patch.
Signed-off-by: Thomas F Herbert <thomasfherb...@entpnt.com>
---
datapath/actions.c | 32 ++++++---
datapath/flow.c | 80 +++++++++++++++++++----
datapath/flow.h | 1 +
datapath/flow_netlink.c | 62 ++++++++++++++++--
datapath/linux/compat/include/linux/openvswitch.h | 16 ++---
5 files changed, 153 insertions(+), 38 deletions(-)
...
diff --git a/datapath/flow.c b/datapath/flow.c
index 69b13b3..d549fae 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
...
+
+ if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+ sizeof(__be16)))
+ return 0;
This check is included in the pskb_may_pull() below.
This puzzles me too. The double headroom check looks redundant but it is
leveraged from the existing code that does it for 802.1q single vlans.
Please advise, should we change this everywhere?
+
+ if (unlikely(!pskb_may_pull(skb, sizeof(struct
qtag_prefix) +
+ sizeof(__be16)))) {
+ return -ENOMEM;
+ }
+
This is also leveraged from existing vlan code that checks for headroom
for the vlan header plus the encapsulated ethertype. I didn't change
this except extending it for a full 802.1ad vlan header. Please advise,
should we change this everywhere?
I'd suggest to use offsetof(struct qtag_prefix, ctci), but I am not really
sure if it's portable. Or simply define the ctci_offset above as
sizeof(struct qtag_prefix) + sizeof(__be16) for better readability.
+ if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
+ key->eth.ctci = qp->tci |
htons(VLAN_TAG_PRESENT);
+ __skb_pull(skb, sizeof(struct qtag_prefix));
+ }
+ }
+ return 0;
+ }
....
Thanks
fbl
--
Thomas F. Herbert
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
