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

[Ben Pfaff]

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.