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

[Ben Pfaff]

On Mon, Nov 25, 2013 at 02:15:49PM +0800, Alexander Wu wrote:
> 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.

OK.

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

Right, I'm not talking about next tables.

>   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.

It's fine to add some more details to ofp-util.def.  It sounds like,
perhaps, you need to add a way to indicate the versions that support
the action in question.  That would be better than duplicating code.

>   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.

meta-flow.[ch] offers strings.

>     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.

Please modify meta-flow.[ch].  We will catch mistakes in review.