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.
>>> > @@ -884,13 +843,14 @@ static int ovs_key_from_nlattrs(struct
>>> > sw_flow_match *match, u64 attrs,
>>> > nd_key->nd_sll, ETH_ALEN, is_mask);
>>> > SW_FLOW_KEY_MEMCPY(match, ipv6.nd.tll,
>>> > nd_key->nd_tll, ETH_ALEN, is_mask);
>>> > - attrs &= ~(1ULL << OVS_KEY_ATTR_ND);
>>> > + a[OVS_KEY_ATTR_ND] = NULL;
>>> > }
>>> >
>>> > - if (attrs != 0) {
>>> > - OVS_NLERR("Unknown key attributes (%llx).\n",
>>> > - (unsigned long long)attrs);
>>> > - return -EINVAL;
>>> > + for (i = 0; i < OVS_KEY_ATTR_MAX; i++) {
>>> > + if (a[i]) {
>>> > + OVS_NLERR("Unknown key attribute (%d).\n", i);
>>> > + return -EINVAL;
>>> > + }
>>> > }
>>> >
>>> Do we still need it with NLA_PARSE_F_UNKNOWN flag?
>>
>>
>> Probably not. I'll fix this up.
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
