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);
}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
