On 11/1/14 1:03 AM, Jesse Gross wrote:
On Thu, Oct 9, 2014 at 3:34 AM, Lori Jakab <loja...@cisco.com> wrote:
On 9/12/14 12:26 AM, Jesse Gross wrote:
On Mon, Sep 8, 2014 at 5:43 AM, Lori Jakab <loja...@cisco.com> wrote:
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1831,6 +1831,8 @@ static int __ovs_nla_copy_actions(const struct
nlattr
*attr,
case OVS_ACTION_ATTR_POP_ETH:
if (noeth)
return -EINVAL;
+ if (vlan_tci != htons(0))
+ return -EINVAL;
I wonder if this check is necessary/correct? It seems like there could
be other equivalent L2 tags that we don't know about, like QinQ.
Yes, but those tags should be removed before a pop_eth() action, not as the
part of the pop_eth() action. At least that was my understanding to the
above discussion. So I added that conditional to "enforce" that we have a
"clean" Ethernet header.
Yes, I agree that everything should already have been removed by this
point. The question was just whether it is good to have a partial
check that is not necessarily comprehensive but I guess there is no
harm in checking a common case (maybe add a comment)?
One small thing: It's maybe a little cleaner to check against
VLAN_TAG_PRESENT rather than 0 like we do in other places. I know it's
equivalent but it's perhaps a little more self-documenting.
Right, I changed to VLAN_TAG_PRESENT.
I think is_layer3 in the control block should not be used on output
anymore,
since we cannot guarantee it is correct. So another change I suggest is
removing the check for a packet being layer 2 or layer 3 in the various
${vport}-send() functions, and keep its use limited to receiving.
That generally makes sense to me. For things like LISP would you then
check to make sure that this is an IPv4/v6 packet before
encapsulation?
Well, the lisp_send() function in vport-lisp.c only receives the vport and
sk_buff as parameters, and OVS_CB doesn't have the flow key anymore. A
patch I proposed initially just removed the the 'flow' member in
ad50cb605b24495956b6a7664d379abd3912ed50, but I see the 'pkt_key' member was
removed later by Pravin:
commit e74d48171e590b50cdcb2798a3e7c5c32313887b
Author: Pravin B Shelar <pshe...@nicira.com>
Date: Wed Sep 17 18:58:44 2014 -0700
datapath: Remove pkt_key from OVS_CB.
OVS keeps pointer to packet key in skb->cb, but the packet key is
store on stack. This could make code bit tricky. So it is better to
get rid of the pointer.
Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
Acked-by: Andy Zhou <az...@nicira.com>
So I don't have the necessary information there to do the check.
I think skb->protocol should still be accurate, so probably the
existing check should actually be OK already (maybe with just an
additional check to look at skb->vlan_tci).
Done.
If so, could we apply the same mechanism on the receive
side to completely avoid the need for the is_layer3 flag (since I
think it would then only be read in a single place)?
I need the is_layer3 flag to be able to parse the packet correctly in
key_extract() in flow.c. Not sure if there is another way to do it,
especially without a flow key in SKB_CB.
I was just thinking about passing it in a parameter but I'm not sure
if that's actually cleaner or not. It's probably a little messier in
that code path but on the other hand it's less of a global object that
way. In any case, we're no longer so pressured for space in
OVS_SKB_CB.
In the end I fully removed 'is_layer3', I like it better that way too.
I posted a new revision here:
http://openvswitch.org/pipermail/dev/2014-November/048130.html
-Lori
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
