On 4/11/14, 1:47 AM, Jesse Gross wrote:
On Thu, Mar 20, 2014 at 4:37 AM, Lori Jakab <loja...@cisco.com> wrote:
On 1/6/14, 7:55 PM, Jesse Gross wrote:
On Tue, Dec 24, 2013 at 9:02 AM, Lorand Jakab<loja...@cisco.com> wrote:
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 7d193d8..8f0d5ab 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1289,6 +1289,14 @@ static int validate_tp_port(const struct sw_flow_key
*flow_key)
return -EINVAL;
}
+static int validate_eth_present(const struct sw_flow_key *flow_key)
+{
+ if (flow_key->phy.noeth)
+ return -EINVAL;
I don't know if I would bother making this a separate function - it's
actually more code in each location and arguably less clear.
OK, I will remove the function.
More importantly though, what happens if we have a pop followed by a
set action? I'm not sure that this will catch that.
Right. Fixed with the following:
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1283,7 +1283,8 @@ static int validate_and_copy_set_tun(const struct
nlattr *attr,
static int validate_set(const struct nlattr *a,
const struct sw_flow_key *flow_key,
struct sw_flow_actions **sfa,
- bool *set_tun)
+ bool *set_tun,
+ bool pop_eth_present)
{
const struct nlattr *ovs_key = nla_data(a);
int key_type = nla_type(ovs_key);
@@ -1416,6 +1421,7 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
{
const struct nlattr *a;
int rem, err;
+ bool pop_eth_present = false;
if (depth >= SAMPLE_ACTION_DEPTH)
return -EOVERFLOW;
@@ -1459,6 +1465,9 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
case OVS_ACTION_ATTR_POP_ETH:
+ if (key->phy.noeth)
+ return -EINVAL;
+ pop_eth_present = true;
break;
case OVS_ACTION_ATTR_PUSH_ETH:
@@ -1476,7 +1487,7 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
break;
case OVS_ACTION_ATTR_SET:
- err = validate_set(a, key, sfa, &skip_copy);
+ err = validate_set(a, key, sfa, &skip_copy,
pop_eth_present);
if (err)
return err;
break;
+noethernet:
if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))
goto nla_put_failure;
Does it still make sense to send the EtherType? It's not present in
the packet and I believe that the information is contained in the
attributes that we do send (i.e. IPv4 or v6 attributes).
We had a discussion about this in August last year:
One such decision is how to handle the flow key. I set all fields in
key->eth to 0, except the type, because we still need to know what
kind
of L3 packet do we have. Since a lot of code is accessing
key->eth.type, this is easier than having this information in a
different place, although it would be more elegant to set this field
to
0 as well.
Can we use skb->protocol for the cases where we currently access the
EtherType? Are there cases that you are particularly concerned about?
The only case I'm concerned about is in the action validation code,
particularly the validate_set() and validate_tp_port() functions, since
they
don't have access to the skb, and need to know the layer 3 protocol of
the
flow. In validate_set() we could relax the check for eth.type for
OVS_KEY_ATTR_IPV4 and OVS_KEY_ATTR_IPV6, but I'm not sure what to do
about
validate_tp_port().
In general, I think it would be a good idea for the flow key to have a
field
specifying the layer 3 protocol, when an ethernet header is not
present.
That makes sense to me.
You mean that we keep eth.type as the L3 protocol field, or define a new
one, separate from the eth union?
I think it's fine to keep using eth.type as the protocol field. I
think we can probably some holes to move the is_layer3 flag into the
non-tunnel portion of the flow though.
Should we revisit this discussion?
I was just referring to the Netlink encoding here. Can we populate the
flow key in the kernel when we translate the flow?
Not sure I understand the question.
Going through the code, I see that omitting OVS_KEY_ATTR_ETHERTYPE
currently means an 802.2 packet, if the mask is set to 0xffff. Are you
suggesting to omit OVS_KEY_ATTR_ETHERTYPE for layer 3 packets both in
the flow key and the mask?
-Lori
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
