[ovs-dev] [PATCH] tunnel: Support matching on the presence of Geneve options.

Jesse Gross jesse at nicira.com
Fri Aug 28 23:22:15 UTC 2015


On Fri, Aug 28, 2015 at 2:50 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Jesse,
>
> Some comments and a possible bug below, otherwise looks good.
>
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>
> I recall Ben has reviewed the earlier patches in this domain, so maybe it would be good to get his Ack as well.

Thanks for the review. I fixed up the issues below but I'll wait a bit
to see if Ben feels like reviewing it.

>> On Aug 28, 2015, at 9:39 AM, Jesse Gross <jesse at nicira.com> wrote:
>> diff --git a/lib/bitmap.h b/lib/bitmap.h
>> index cf5d3f0..35f033e 100644
>> --- a/lib/bitmap.h
>> +++ b/lib/bitmap.h
>> @@ -278,4 +278,6 @@ bitmap_is_all_zeros(const unsigned long *bitmap, size_t n)
>> #define ULLONG_SET0(MAP, OFFSET) ((MAP) &= ~(1ULL << (OFFSET)))
>> #define ULLONG_SET1(MAP, OFFSET) ((MAP) |= 1ULL << (OFFSET))
>>
>> +#define ULLONG_GET(MAP, OFFSET) ((MAP) & (1ULL << (OFFSET)))
>> +
>
> It seems a bit unclear if this is intended to return the bit as ULLONG or a boolean for the presence of the bit.

I guess it doesn't really matter much for these purposes but I forced
it to be a bool and added a comment pointing that out.

>> mf_is_zero(const struct mf_field *mf, const struct flow *flow)
>> {
>> -    union mf_value value;
>> +    if (!mf_is_tun_metadata(mf)) {
>> +        union mf_value value;
>>
>> -    mf_get_value(mf, flow, &value);
>> -    return is_all_zeros(&value, mf->n_bytes);
>> +        mf_get_value(mf, flow, &value);
>> +        return is_all_zeros(&value, mf->n_bytes);
>> +    } else {
>> +        return !ULLONG_GET(flow->tunnel.metadata.present.map,
>> +                           mf->id - MFF_TUN_METADATA0);
>
> What if the option is present, but the value is zero? For masks that is not allowed, but how about the flow itself?

It actually is possible to have an option that is present but with a
zero mask (that would mean a match on the option being present but the
value is not important). There are therefore similar issues.

There are only two callers of this function and both use it to see if
a field has been written to previously - such as when parsing an
OpenFlow match string to check for duplicates. From this point of
view, I think this implementation is the right one.

I think that renaming the function to reflect it's purpose (and now
implementation) is probably the best solution. I changed it to
mf_is_set().

>> diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
>> index acaacff..bef4641 100644
>> --- a/lib/tun-metadata.c
>> +++ b/lib/tun-metadata.c
>> @@ -280,7 +280,7 @@ tun_metadata_write(struct flow_tnl *tnl,
>>
>> static const struct tun_metadata_loc *
>> metadata_loc_from_match(struct tun_table *map, struct match *match,
>> -                        unsigned int idx, unsigned int field_len)
>> +                        unsigned int idx, unsigned int field_len, bool masked)
>> {
>>     ovs_assert(idx < TUN_METADATA_NUM_OPTS);
>>
>> @@ -293,18 +293,19 @@ metadata_loc_from_match(struct tun_table *map, struct match *match,
>>     }
>>
>>     if (match->tun_md.alloc_offset + field_len >= TUN_METADATA_TOT_OPT_SIZE ||
>> -        match->tun_md.loc[idx].len) {
>> +        match->wc.masks.tunnel.metadata.present.map & idx) {
>
> Is ‘idx’ an index or a mask?

Fixed, thanks that should be using the new ULLONG_GET macro.



More information about the dev mailing list