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

[Ben Pfaff]

On Fri, Nov 29, 2013 at 05:34:04PM +0800, Alexander Wu wrote:
> On 26/11/2013 05:27, Ben Pfaff wrote:
> >>   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.
> >
> 
> I'm trying to add OFPAT13_ACTION() to ofp-util.def, include all actions
> of OpenFlow 1.3.2[p58], do you think it's the right way?
> 
> it looks like:
>  #ifndef OFPAT13_ACTION
>  #define OFPAT13_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME)
>  #endif
>  OFPAT13_ACTION(OFPAT13_OUTPUT,       ofp11_action_output,    0, "output")
>  OFPAT13_ACTION(OFPAT13_SET_MPLS_TTL, ofp11_action_mpls_ttl,  0, "set_mpls_ttl")
>  OFPAT13_ACTION(OFPAT13_DEC_MPLS_TTL, ofp_action_header,      0, "dec_mpls_ttl")
>  OFPAT13_ACTION(OFPAT13_PUSH_VLAN,    ofp11_action_push,      0, "push_vlan")
>  OFPAT13_ACTION(OFPAT13_POP_VLAN,     ofp_action_header,      0, "pop_vlan")
>  OFPAT13_ACTION(OFPAT13_PUSH_MPLS,    ofp11_action_push,      0, "push_mpls")
>  OFPAT13_ACTION(OFPAT13_POP_MPLS,     ofp11_action_pop_mpls,  0, "pop_mpls")
>  OFPAT13_ACTION(OFPAT13_SET_QUEUE,    ofp11_action_set_queue, 0, "set_queue")
>  OFPAT13_ACTION(OFPAT13_GROUP,        ofp11_action_group,     0, "group")
>  OFPAT13_ACTION(OFPAT13_SET_NW_TTL,   ofp11_action_nw_ttl,    0, "set_nw_ttl")
>  OFPAT13_ACTION(OFPAT13_DEC_NW_TTL,   ofp_action_header,      0, "dec_nw_ttl")
>  OFPAT13_ACTION(OFPAT13_SET_FIELD,    ofp12_action_set_field, 1, "set_field")

Yes, that sort of format is fine, as long as you can adjust other code
to work with it.

Thanks,

Ben.