On Mon, Jul 29, 2013 at 12:44 PM, Andy Zhou <az...@nicira.com> wrote:
> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross <je...@nicira.com> wrote:
>>
>> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou <az...@nicira.com> wrote:
>> > diff --git a/datapath/flow.c b/datapath/flow.c
>> > index ba775f4..5ec1b69 100644
>> > --- a/datapath/flow.c
>> > +++ b/datapath/flow.c
>> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
>> > sw_flow_match *match,
>> > return false;
>> > }
>> >
>> > + if (match->mask &&
>> > + !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
>> > + OVS_NLERR("VLAN present bit can not be
>> > wildcarded.\n");
>> > + /* Simply log error until user the space program
>> > is
>> > + * fixed. Then we can switch to return false
>> > from
>> > + * here.
>> > + */
>> > + }
>>
>> Can we just fix this? We already force the bit on for the value in
>> userspace, it seems like we could do it for the mask at the same time.
>
>
> We could, but I was hopping to return an error from here eventually.
I'm not sure that I understand. I was just referring to fixing the
issue that you mentioned in the comment now rather than later.
>>
>>
>> I think we could also simplify it by just removing the !is_mask test
>> in the check in ovs_key_from_nlattrs().
>
> That would cause the mask value to be set twice, which is fine. But the
> logic would seem inconsistent with IN_PORT and look odd to me. But I don't
> mind changing it if you insist.
Why would this cause the mask to get set twice? I think that check is
just protecting the sanity check for the CFI bit.
All of the other checks that are in the validate function look at the
combination of values and mask whereas the ones that can be evaluated
independently are in ovs_key_from_nlattrs(). Since the logic is the
same and needs to be applied to both values and masks I think it fits
pretty much exactly in from_nlattrs.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
