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.
> 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.
> @@ -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.
> @@ -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.
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.
> 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
