On Wed, Dec 19, 2012 at 2:48 AM, Jarno Rajahalme
<[email protected]> 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.
I think the comparison at the top of the function alone is sufficient.
In theory it's possible that ip_dst is zero and something other than
tun_id has changed but that would indicate a bug somewhere else and
the resulting output would still be legal, so we probably don't need
to worry about it.
> 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);
> }
Since this is encoding a match instead of an action list the semantics
are slightly different. In both cases, the default value is zero but
in a match there is only ever one instance of tun_id whereas you can
have several in an action list. Therefore, here's it's OK to omit the
key in the zero case.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
