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...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
