On Mon, Apr 3, 2017 at 5:30 PM, Joe Stringer <j...@ovn.org> wrote: > On 3 April 2017 at 13:30, Andy Zhou <az...@ovn.org> wrote: >> When adding a new field in the 'struct dpif_backer_support', the >> corresponding appctl show command should be updated to display >> the new field. Currently, there is nothing to remind the developer >> that to update the show command. This can lead to code maintenance >> issues. >> >> Switch to use macros to define those fields. This makes the show >> command update automatic. >> >> Signed-off-by: Andy Zhou <az...@ovn.org>
Thanks for the review. Pushed with fixes suggested. >> >> --- > > Looks like there's a few typos: > > 's/FILED/FIELD/g' Fixed > > A couple more comments below, but otherwise LGTM. Thanks! > > Acked-by: Joe Stringer <j...@ovn.org> > >> v1->v2: >> Add more comments. Fix typos >> --- >> lib/odp-util.h | 52 ++++++++++++++++++++++++++------------------ >> ofproto/ofproto-dpif.c | 28 ++++++++++-------------- >> ofproto/ofproto-dpif.h | 59 >> ++++++++++++++++++++++++++++++-------------------- >> 3 files changed, 78 insertions(+), 61 deletions(-) >> >> diff --git a/lib/odp-util.h b/lib/odp-util.h >> index 50fa1d133e1f..d9668a75e811 100644 >> --- a/lib/odp-util.h >> +++ b/lib/odp-util.h >> @@ -167,30 +167,40 @@ int odp_flow_from_string(const char *s, >> const struct simap *port_names, >> struct ofpbuf *, struct ofpbuf *); >> >> +/* ODP_SUPPORT_FIELD(TYPE, FIELD_NAME, FILED_DESCRIPTION) >> + * >> + * Each 'ODP_SUPPORT_FIELD' defines a member in 'struct odp_support', >> + * and represents support for related OVS_KEY_ATTR_* fields. >> + * They are defined as macros so that 'dpif_show_support()' is less >> + * likely to go out of sync when new fields are added. */ > > This is a bit simpler language, similarly for the dpif_support below. > > "They are defined as macros to keep 'dpif_show_support()' in sync as > new fields are added." Thanks, Will change. > > <snip> > >> +/* DPIF_SUPPORT_FILED(TYPE, FIELD_NAME, FIELD_DESCRIPTION) >> + * >> + * Each 'DPIF_SUPPORT_FILED' defines a member in 'struct >> dpif_backer_support' >> + * and represents support for a datapath action. >> + * They are defined as macros so that 'dpif_show_support()' is less >> + * likely to go out of sync when new fields are added. */ >> +#define DPIF_SUPPORT_FIELDS >> \ > > If DPIF_SUPPORT_FIELDS is about actions, perhaps a name like > DPIF_SUPPORT_ACTION() is a better name? They don't have to be only about actions down the road. The name is about defining a field for 'struct dpif_backer_support'. I kept it as is for now. We can always change them later. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
