On Wed, Sep 17, 2014 at 9:11 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Wed, Sep 17, 2014 at 9:02 PM, Joe Stringer <joestrin...@nicira.com> wrote:
>>
>>
>> On 18 September 2014 13:02, Joe Stringer <joestrin...@nicira.com> wrote:
>>>>
>>>>
>>>> > @@ -229,17 +244,19 @@ static bool match_validate(const struct
>>>> > sw_flow_match *match,
>>>> > }
>>>> > }
>>>> >
>>>> > - if ((key_attrs & key_expected) != key_expected) {
>>>> > + attrs = attrs_to_bitmask(key_attrs, OVS_KEY_ATTR_MAX);
>>>> > + if ((attrs & key_expected) != attrs) {
>>>> > /* Key attributes check failed. */
>>>> > OVS_NLERR("Missing expected key attributes
>>>> > (key_attrs=%llx, expected=%llx).\n",
>>>> > - (unsigned long long)key_attrs,
>>>> > (unsigned long long)key_expected);
>>>> > + (unsigned long long)attrs, (unsigned
>>>> > long long)key_expected);
>>>> > return false;
>>>> > }
>>
>>
>> I also accidentally changed the key_attrs comparison line, which I'll roll
>> in a fix for.
>>
>>>>
>>>> <snip>
>>>>
>>>> > @@ -611,30 +569,31 @@ int ovs_nla_put_egress_tunnel_key(struct sk_buff
>>>> > *skb,
>>>> > egress_tun_info->options_len);
>>>> > }
>>>> >
>>>> > -static int metadata_from_nlattrs(struct sw_flow_match *match, u64
>>>> > *attrs,
>>>> > +static int metadata_from_nlattrs(struct sw_flow_match *match,
>>>> > const struct nlattr **a, bool is_mask)
>>>> > {
>>>> > - if (*attrs & (1ULL << OVS_KEY_ATTR_DP_HASH)) {
>>>> > + if (a[OVS_KEY_ATTR_DP_HASH]) {
>>>> > u32 hash_val = nla_get_u32(a[OVS_KEY_ATTR_DP_HASH]);
>>>> >
>>>> > SW_FLOW_KEY_PUT(match, ovs_flow_hash, hash_val,
>>>> > is_mask);
>>>> > - *attrs &= ~(1ULL << OVS_KEY_ATTR_DP_HASH);
>>>> > + a[OVS_KEY_ATTR_DP_HASH] = NULL;
>>>> > }
>>>> If you change a[], then match_validate() does not work.
>>>
>>>
>>> I thought that I was misunderstanding something here, now I see that
>>> ovs_key_from_nlattrs() doesn't modify the caller's version of the attrs
>>> bitmask on master.
>>>
>>> Would you prefer that the ovs_key_from_nlattrs() function made a local
>>> copy of 'a' and use that here or just convert it to a bitmask and keep
>>> metadata_from_nlattrs() unchanged from master?
>>>
>>
>> Actually, as far as I can tell, the logic below is the only reason we are
>> changing 'a' in metadata_from_nlattrs(). We can drop all of the a[foo] =
>> NULL and it all becomes simpler.
>>
>>
> Sounds good to me. But I would like to take opinion from Andy.
>
I had offline chat with Andy. He is ok with it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
