[ovs-dev] [PATCH 2/9] ofp-util: Implement OFPMP_TABLE_FEATURES en/decode

[Alexander Wu]

On 22/11/2013 06:19, Ben Pfaff wrote:
> On Thu, Nov 21, 2013 at 05:04:29PM +0800, Alexander Wu wrote:
>> V3:
>>    1. Update names of functions/macros to make them meaningful.
>>    2. Fix codingstyle.
>>    3. Remove useless logic/struct/function.
>>    4. Make printable messages more friendly.
>>    5. Add OVS_ACTIONS macro to display all action features.
>>
>> V2:
>>    Restructure implement of OFPMP_TABLE_FEATURES
>>    Change decode_*_raw to normalized pull functions
>>
>>    1. add defines and funcs to decode table features
>>    2. restructure OFPMP_TABLE_FEATURES decode function
>>       restructure the function, now them acts like others.
>>    3. Change big array to defines.
>>       Change big array to defines.(oxm, table_feature_prop)
>>       Fix some names, now they're more meaningful.
>>    4. use macros to restructure implement
>>    5. Restructure get_* to more effective ones. (table_features, oxm)
>>    6. remove useless array and prototype
>>
>> Simon Horman:
>>    Fix function paras alignment.
>>    Fix hard coding to marco.
>>    Fix VLOG calls with rl.
>>    Fix CodingStyle:
>>      max chars per line - 79
>>    Delete useless blank line.
>>    Restructure implement by macro.
>>
>> V1:
>>    ofp-util: Implement the encode/decode Table Features functions
>>
>>    Implement the encode/decode table features msgs function, and
>>    NOTE that we implement the decode functions *_raw, maybe we
>>    should change it the ofpbuf_pull?
>>
>> Signed-off-by: Alexander Wu <alexander.wu at huawei.com>
>
> This patch adds a lot of duplication of existing code.  The
> OVS_ACTIONS macro duplicates ofp-util.def, and the OXMs array
> duplicates include/openflow/openflow-1.2.h and meta-flow.[ch].  It
> would be much better to use the existing code than to duplicate it.
>
> The property format introduce for table features in OF1.3 is used in
> OF1.4 in many messages.  It would probably be a good idea to implement
> it in a general way, and with general-purpose names, so that the code
> can be reused.
>
> The abstracted versions of the table properties, in ofp-util.h, are
> not very abstract.  They are not much easier for code to deal with
> than the raw property-based format.  I think that it would be more
> useful to define the table features in terms of concepts that OVS
> already has elsewhere.  For example, one could add a member to struct
> ofputil_table_features that represents the supported instructions as a
> bitmask whose bits correspond to elements of OFPACT_*.  That would be
> a lot easier for code to deal with than finding the right property and
> then iterating through the instruction ids.
>
>

Thanks for your replies!

1. Duplication of existing code.
   I've read ofp-util.def and openflow-1.2.h/meta-flow.[ch].
   And I think the "duplication" may be needed. Here I implement 4 props:
     instructions, next_tables, actions, oxms.
   The actions and oxms use separate macro/struct. The others not.

   1.1 Instructions: I add a function in ofp-actions to use the existing
     code of instructions directly.

   1.2 Next tables: it doesn't need to use any existing code.

   1.3 Actions:
     Actions use separate struct, the reason as below:
     OFPAT11_ACTION(in ofp-util.def) has 24 actions, but OpenFlow 1.3.2
     [p58] has 17 actions, most of OFPAT11_SET_*, OFPAT11_COPY_TTL_* and
     *_PBB(not implement) are different here. So I think to use a separate
     struct here would be better. Perhaps I should rename OVS_ACTIONS to
     OFP13_FEATURE_ACTIONS, it's much more meaningful.

   1.4 OXMs:
     OXMs use separate struct, the reason as below:
     There are macros in openflow-1.2.h, which define OXM_OF_IN_PORT to
     OXM_OF_IPV6_EXTHDR_W, but there aren't strings regarding to them.
     Meanwhile, I found:
       oxm12_to_ofp11_flow_match_fields, ofputil_put_ofp10_table_stats
     Before OpenFlow 1.3, match field is defined as bitmap, and the functions
     above use macros in openflow-1.2.h to represent the bitmap. My works are
     not different from them, I just define OVS_OXMS to represent OpenFlow1.3
     supported oxm features. Perhaps I should rename OVS_OXMS to
     OFP13_FEATURE_OXMS, it's much more meaningful.

     metaflow.[ch] seem to be hermetical, I'm not sure I could modify it on
     the premise of not damaging the purpose of the files.

2. General way to implement property format
   Understand after reading OpenFlow 1.4. I'll fix it in next version.

3. Make it easier to code inside OVS
   Understand. I'll deal with this case.
   Fix it in next version.