On Dec 19, 2012, at 9:48 , ext Jarno Rajahalme wrote:
> On Dec 18, 2012, at 19:37 , ext Jesse Gross wrote:
>> On Tue, Dec 18, 2012 at 5:57 AM, Jarno Rajahalme
>> <[email protected]> wrote:
>>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>>> index f1075e3..1ba241d 100644
>>> --- a/lib/odp-util.c
>>> +++ b/lib/odp-util.c
>>> static void
>>> -commit_set_tun_id_action(const struct flow *flow, struct flow *base,
>>> +commit_set_tunnel_action(const struct flow *flow, struct flow *base,
>>> struct ofpbuf *odp_actions)
>>> {
>>> - if (base->tunnel.tun_id == flow->tunnel.tun_id) {
>>> + if (!memcmp(&base->tunnel, &flow->tunnel, sizeof base->tunnel)) {
>>> return;
>>> }
>>> - base->tunnel.tun_id = flow->tunnel.tun_id;
>>> + memcpy(&base->tunnel, &flow->tunnel, sizeof base->tunnel);
>>>
>>> - commit_set_action(odp_actions, OVS_KEY_ATTR_TUN_ID,
>>> - &base->tunnel.tun_id, sizeof(base->tunnel.tun_id));
>>> + /* A valid IPV4_TUNNEL must have non-zero ip_dst. */
>>> + if (flow->tunnel.ip_dst) {
>>> + struct ovs_key_ipv4_tunnel ipv4_tun_key;
>>> +
>>> + ipv4_tun_key.tun_id = base->tunnel.tun_id;
>>> + ipv4_tun_key.tun_flags = flow_to_odp_flags(base->tunnel.flags);
>>> + ipv4_tun_key.ipv4_src = base->tunnel.ip_src;
>>> + ipv4_tun_key.ipv4_dst = base->tunnel.ip_dst;
>>> + ipv4_tun_key.ipv4_tos = base->tunnel.ip_tos;
>>> + ipv4_tun_key.ipv4_ttl = base->tunnel.ip_ttl;
>>> + memset(&ipv4_tun_key.pad, 0, sizeof ipv4_tun_key.pad);
>>> +
>>> + commit_set_action(odp_actions, OVS_KEY_ATTR_IPV4_TUNNEL,
>>> + &ipv4_tun_key, sizeof ipv4_tun_key);
>>> + } else if (base->tunnel.tun_id != htonll(0)) {
>>> + commit_set_action(odp_actions, OVS_KEY_ATTR_TUN_ID,
>>> + &base->tunnel.tun_id, sizeof
>>> base->tunnel.tun_id);
>>
>> I think this check for tun_id introduces a bug. This sequence is possible:
>> set(tun_id(1)),output,set(tun_id(0)),output
>>
>> However, here we would drop the second set action. We already know
>> that some thing has changed at this point, so we can just make it an
>> unconditional else.
>
> In the previous version there was an explicit check to the old tun_id value.
> That became a bit more difficult after moving the compare and copy to the top.
>
> Anyway, a corresponding change should probably be done here as well:
>
> @@ -1361,7 +1382,20 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const
> struct flow *flow,
> nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, flow->skb_priority);
> }
>
> - if (flow->tunnel.tun_id != htonll(0)) {
> + if (flow->tunnel.ip_dst) {
> + struct ovs_key_ipv4_tunnel *ipv4_tun_key;
> +
> + ipv4_tun_key = nl_msg_put_unspec_uninit(buf,
> OVS_KEY_ATTR_IPV4_TUNNEL,
> + sizeof *ipv4_tun_key);
> + /* layouts differ, flags has different size */
> + ipv4_tun_key->tun_id = flow->tunnel.tun_id;
> + ipv4_tun_key->tun_flags = flow_to_odp_flags(flow->tunnel.flags);
> + ipv4_tun_key->ipv4_src = flow->tunnel.ip_src;
> + ipv4_tun_key->ipv4_dst = flow->tunnel.ip_dst;
> + ipv4_tun_key->ipv4_tos = flow->tunnel.ip_tos;
> + ipv4_tun_key->ipv4_ttl = flow->tunnel.ip_ttl;
> + memset(ipv4_tun_key->pad, 0, sizeof ipv4_tun_key->pad);
> + } else if (flow->tunnel.tun_id != htonll(0)) {
> nl_msg_put_be64(buf, OVS_KEY_ATTR_TUN_ID, flow->tunnel.tun_id);
> }
>
>
On a second thought, shouldn't the FLOW_TNL_F_KEY tell if the tun_id is valid?
However, based on the following code in lib/match.c, it appears that the
semantics of tun_id has changed. Maybe the FLOW_TNL_F_KEY should be set also
when setting the tun_id only, so that also the zero tun_id could be used as a
valid key value?
if (flow->tunnel.ip_dst) {
if (flow->tunnel.flags & FLOW_TNL_F_KEY) {
memset(&wc->masks.tunnel.tun_id, 0xff, sizeof
wc->masks.tunnel.tun_id);
}
memset(&wc->masks.tunnel.ip_src, 0xff, sizeof wc->masks.tunnel.ip_src);
memset(&wc->masks.tunnel.ip_dst, 0xff, sizeof wc->masks.tunnel.ip_dst);
memset(&wc->masks.tunnel.flags, 0xff, sizeof wc->masks.tunnel.flags);
memset(&wc->masks.tunnel.ip_tos, 0xff, sizeof wc->masks.tunnel.ip_tos);
memset(&wc->masks.tunnel.ip_ttl, 0xff, sizeof wc->masks.tunnel.ip_ttl);
} else if (flow->tunnel.tun_id) {
memset(&wc->masks.tunnel.tun_id, 0xff, sizeof wc->masks.tunnel.tun_id);
}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
