Thanks for the quick turnaround Jesse. See inline. On 11/3/14 7:46 PM, Jesse Gross wrote:
On Mon, Nov 3, 2014 at 6:31 AM, Lorand Jakab <loja...@cisco.com> wrote:diff --git a/datapath/flow.c b/datapath/flow.c index a3c5d2f..fea26ae 100644 --- a/datapath/flow.c +++ b/datapath/flow.c @@ -458,28 +458,31 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) + /* Link layer. */ + if (key->phy.noeth) { + key->eth.tci = 0; + key->eth.type = skb->protocol;Is there a reason to set the TCI to zero here? The MAC addresses are left uninitialized, so at a minimum it seems inconsistent unless it is used somewhere.
Right, will remove it.
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index 37b0bdd..c9625e6 100644 --- a/datapath/flow_netlink.c +++ b/datapath/flow_netlink.c @@ -685,6 +689,10 @@ static int metadata_from_nlattrs(struct sw_flow_match *match, u64 *attrs, return -EINVAL; *attrs &= ~(1ULL << OVS_KEY_ATTR_TUNNEL); } + if (*attrs & (1ULL << OVS_KEY_ATTR_ETHERNET)) + SW_FLOW_KEY_PUT(match, phy.noeth, false, is_mask); + else + SW_FLOW_KEY_PUT(match, phy.noeth, true, is_mask); return 0; }It's not entirely clear to me what the intended semantics are for the no Ethernet header case are with respect to wildcarding. Is it supposed to match only L3 packets? Or is it supposed to be a wildcard. The above looks like a wildcard to me but my guess is that's not the intention.
Will change the code to always exact match phy.noeth.
@@ -751,6 +759,18 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs, if (attrs & (1ULL << OVS_KEY_ATTR_IPV4)) { const struct ovs_key_ipv4 *ipv4_key; + /* Add eth.type value for layer 3 flows */ + if (!(attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE))) { + __be16 eth_type; + + if (is_mask) { + eth_type = htons(0xffff); + } else { + eth_type = htons(ETH_P_IP); + }"Official" kernel style is to not have curly braces around single line if statements.
Will fix.
@@ -1779,6 +1819,7 @@ static int __ovs_nla_copy_actions(const struct nlattr *attr, { const struct nlattr *a; int rem, err; + bool noeth = key->phy.noeth;Related to the above comment about wildcarding - I'm not sure that that this is safe currently. If noeth is wildcarded it will show up as false, which means that we will act as if we do have an Ethernet header but we might not in some cases.
Should be fixed with the exact matching of phy.noeth.
As a side note, it's somewhat more difficult to read names with negatives in them, particularly in cases like this where they are set to false so you have to parse a double negative to understand the meaning.
Will rename to is_layer3.
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c index f3d450f..a51d2da 100644 --- a/datapath/vport-lisp.c +++ b/datapath/vport-lisp.c @@ -452,8 +440,9 @@ static int lisp_send(struct vport *vport, struct sk_buff *skb) tun_key = &OVS_CB(skb)->egress_tun_info->tunnel; - if (skb->protocol != htons(ETH_P_IP) && - skb->protocol != htons(ETH_P_IPV6)) { + if ((skb->protocol != htons(ETH_P_IP) && + skb->protocol != htons(ETH_P_IPV6)) || + skb->vlan_tci & htons(VLAN_TAG_PRESENT)) {Sparse caught a byte order problem here: CHECK /home/jesse/openvswitch/datapath/linux/vport-lisp.c /home/jesse/openvswitch/datapath/linux/vport-lisp.c:445:29: warning: restricted __be16 degrades to integer But there's a help function called vlan_tx_tag_present() that would be better to use in any case.
Done. I will send out a new revision in a minute. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
