On 18 September 2014 13:02, Joe Stringer <[email protected]> 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.
> @@ -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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
