[ovs-dev] [ovs-discuss] [ACLv2 12/19] flow: Allow matching on additional ARP payload fields.

Jesse Gross jesse at nicira.com
Fri Sep 4 23:52:37 UTC 2009



Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>
>   
>> @@ -53,7 +53,10 @@
>>      CLS_FIELD(OFPFW_NW_DST_MASK, nw_dst,      NW_DST)       \
>>      CLS_FIELD(OFPFW_NW_PROTO,    nw_proto,    NW_PROTO)     \
>>      CLS_FIELD(OFPFW_TP_SRC,      tp_src,      TP_SRC)       \
>> -    CLS_FIELD(OFPFW_TP_DST,      tp_dst,      TP_DST)
>> +    CLS_FIELD(OFPFW_TP_DST,      tp_dst,      TP_DST)       \
>> +    CLS_FIELD(NICFW_AR_SHA,      ar_sha,      AR_SHA)       \
>> +    CLS_FIELD(NICFW_AR_THA,      ar_tha,      AR_THA)
>> +
>>     
>
> I'm not a huge fan of adding two more fields to the classifier
> table.  I think that tp_src and tp_dst are mutually exclusive
> with ar_sha and ar_tha.  Is that correct?  If so, is there any
> way that we can just extend tp_src and tp_dst to 6 bytes each and
> keep the current number of classifier fields?
>   

Yes, these fields are mutually exclusive.  I combined them into two 6 
byte fields.  I made them unions.  For example:

    union {
        struct {
            uint32_t tp_src_pad;
            uint16_t tp_src;    /* TCP/UDP source port. */
        } PACKED;
        uint8_t ar_sha[ETH_ALEN]; /* ARP sender hardware address. */
    };

> I don't see any updates to tests/test-classifier.c, but I think
> that some would be required, perhaps just to get it to compile
> and otherwise to make sure that the new fields are tested.  (Does
> this pass "make check"?  How does the code coverage for
> classifier.c look if you build with --enable-coverage?  We get
> 82% coverage at the moment and I'd like to see that go up, not
> down.)
>
>   

I updated the tests.  Everything passes and the coverage is the same as 
before.

>> diff --git a/lib/flow.h b/lib/flow.h
>> index 0932746..d110213 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -30,6 +30,13 @@ struct ds;
>>  struct ofp_match;
>>  struct ofpbuf;
>>  
>> +enum nicira_flow_wildcards {
>> +    NICFW_AR_SHA  = 1 << 25,  /* ARP sender hardware address. */
>> +    NICFW_AR_THA  = 1 << 26   /* ARP target hardware address. */
>> +};
>>     
>
> Can we use ovs_flow_wildcards and OVSFW_*?  After all this is
> Open vSwitch code, not anything really Nicira-specific.
>
>   

OK, I renamed them.

>> +#define OFPFW_ALL (OFPFW_ALL | NICFW_AR_SHA | NICFW_AR_THA)
>>     
>
> I don't think that we should redefine OFPFW_ALL, especially not
> in terms of itself.  That's sure to confuse people and cause
> bizarre problems for anything that includes openflow.h but not
> flow.h.  How about OVSFW_ALL?
>
>   

I originally redefined OFPFW_ALL to ensure that all fields were properly 
wildcarded.  However, it doesn't matter now because the new fields 
overlap with existing ones so the wildcards are the same.




More information about the dev mailing list