| Message ID | 1489707159-16214-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Stringer |
| Headers | show |
On 16 March 2017 at 16:32, Andy Zhou <azhou@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 <azhou@ovn.org> This seems a little intimidating, but I think it does address the problem of keeping dpif_show_support() up to date. At the moment we have this not-entirely-clear distinction between ODP support and DPIF support. Seems to me like the ODP side is about the ability to match various fields, while DPIF support is about the ability to execute certain actions. Maybe that's worth describing above the macros (or renaming the macros to suggest this)? > --- > lib/odp-util.h | 46 ++++++++++++++++++++++++-------------------- > ofproto/ofproto-dpif.c | 28 +++++++++++---------------- > ofproto/ofproto-dpif.h | 52 ++++++++++++++++++++++++++++---------------------- > 3 files changed, 65 insertions(+), 61 deletions(-) > > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 50fa1d1..48449e7 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -167,30 +167,34 @@ int odp_flow_from_string(const char *s, > const struct simap *port_names, > struct ofpbuf *, struct ofpbuf *); > > +#define ODP_SUPPORT_FIELDS \ > + /* Maximum number of 802.1q VLAN headers to serialize in a mask. */ \ > + ODP_SUPPORT_FIELD(size_t, max_vlan_headers, "Max Vlan headers") \ VLAN > + /* Maximum number of MPLS label stack entries to serialise in a mask. */ \ > + ODP_SUPPORT_FIELD(size_t, max_mpls_depth, "Max MPLS depth") \ > + /* If this is true, then recirculation fields will always be \ > + * serialised. */ \ > + ODP_SUPPORT_FIELD(bool, recirc, "Recirc") \ > + /* If true, serialise the corresponding OVS_KEY_ATTR_CONN_* field. */ \ > + ODP_SUPPORT_FIELD(bool, ct_state, "CT state") \ > + ODP_SUPPORT_FIELD(bool, ct_zone, "CT zone") \ > + ODP_SUPPORT_FIELD(bool, ct_mark, "CT mark") \ > + ODP_SUPPORT_FIELD(bool, ct_label, "CT label") \ > + \ > + /* If true, it means that the datapath supports the NAT bits in \ > + * 'ct_state'. The above 'ct_state' member must be true for this \ > + * to make sense */ \ > + ODP_SUPPORT_FIELD(bool, ct_state_nat, "CT state NAT") \ > + \ > + /* Conntrack original direction tuple matching * supported. */ \ > + ODP_SUPPORT_FIELD(bool, ct_orig_tuple, "CT orig tuple") > + > /* Indicates support for various fields. This defines how flows will be > * serialised. */ > struct odp_support { > - /* Maximum number of 802.1q VLAN headers to serialize in a mask. */ > - size_t max_vlan_headers; > - /* Maximum number of MPLS label stack entries to serialise in a mask. */ > - size_t max_mpls_depth; > - > - /* If this is true, then recirculation fields will always be serialised. */ > - bool recirc; > - > - /* If true, serialise the corresponding OVS_KEY_ATTR_CONN_* field. */ > - bool ct_state; > - bool ct_zone; > - bool ct_mark; > - bool ct_label; > - > - /* If true, it means that the datapath supports the NAT bits in > - * 'ct_state'. The above 'ct_state' member must be true for this > - * to make sense */ > - bool ct_state_nat; > - > - bool ct_orig_tuple; /* Conntrack original direction tuple matching > - * supported. */ > +#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) TYPE NAME; > + ODP_SUPPORT_FIELDS > +#undef ODP_SUPPORT_FIELD > }; > > struct odp_flow_key_parms { > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 1c4c804..b32428c 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -4928,13 +4928,13 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, > } > > static void > -show_dp_feature_b(struct ds *ds, const char *feature, bool b) > +show_dp_feature_bool(struct ds *ds, const char *feature, bool b) > { > ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No"); > } > > static void > -show_dp_feature_s(struct ds *ds, const char *feature, size_t s) > +show_dp_feature_size_t(struct ds *ds, const char *feature, size_t s) > { > ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s); > } > @@ -4942,21 +4942,15 @@ show_dp_feature_s(struct ds *ds, const char *feature, size_t s) > static void > dpif_show_support(const struct dpif_backer_support *support, struct ds *ds) > { > - show_dp_feature_b(ds, "Variable length userdata", > - support->variable_length_userdata); > - show_dp_feature_b(ds, "Masked set action", support->masked_set_action); > - show_dp_feature_b(ds, "Tunnel push pop", support->tnl_push_pop); > - show_dp_feature_b(ds, "Ufid", support->ufid); > - show_dp_feature_b(ds, "Trunc action", support->trunc); > - show_dp_feature_b(ds, "Clone action", support->clone); > - show_dp_feature_s(ds, "Max MPLS depth", support->odp.max_mpls_depth); > - show_dp_feature_b(ds, "Recirc", support->odp.recirc); > - show_dp_feature_b(ds, "CT state", support->odp.ct_state); > - show_dp_feature_b(ds, "CT zone", support->odp.ct_zone); > - show_dp_feature_b(ds, "CT mark", support->odp.ct_mark); > - show_dp_feature_b(ds, "CT label", support->odp.ct_label); > - show_dp_feature_b(ds, "CT State NAT", support->odp.ct_state_nat); > - show_dp_feature_s(ds, "Max sample nesting", support->sample_nesting); > +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ > + show_dp_feature_##TYPE (ds, TITLE, support->NAME); > + DPIF_SUPPORT_FIELDS > +#undef DPIF_SUPPORT_FIELD > + > +#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ > + show_dp_feature_##TYPE (ds, TITLE, support->odp.NAME ); > + ODP_SUPPORT_FIELDS > +#undef ODP_SUPPORT_FIELD > } > > static void > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index cc514d2..0562d02 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -143,34 +143,40 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, > * ofproto-dpif) resides. A backer can host several bridges, but a bridge is > * backed by only a single dpif. */ > > +#define DPIF_SUPPORT_FIELDS \ > + /* True if the datapath supports variable-length \ > + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. \ > + * False if the datapath supports only 8-byte (or shorter) userdata. */ \ > + DPIF_SUPPORT_FIELD(bool, variable_length_userdata, \ > + "Variable length userdata") \ > + \ > + /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET \ > + * actions. */ \ > + DPIF_SUPPORT_FIELD(bool, masked_set_action, "Masked set action") \ > + \ > + /* True if the datapath supports tnl_push and pop actions. */ \ > + DPIF_SUPPORT_FIELD(bool, tnl_push_pop, "Tunnel push pop") \ > + \ > + /* True if the datapath supports OVS_FLOW_ATTR_UFID. */ \ > + DPIF_SUPPORT_FIELD(bool, ufid, "Ufid") \ > + \ > + /* True if the datapath supports OVS_ACTION_ATTR_TRUNC action. */ \ > + DPIF_SUPPORT_FIELD(bool, trunc, "Trunk action") \ The description should be truncate, not trunk. > + \ > + /* True if the datapath supports OVS_ACTION_ATTR_CLONE action. */ \ > + DPIF_SUPPORT_FIELD(bool, clone, "Clone action") \ > + \ > + /* Maximum level of nesting allowed by OVS_ACTION_ATTR_SAMPLE action. */\ > + DPIF_SUPPORT_FIELD(size_t, sample_nesting, "Sample neesting") nesting > + > /* Stores the various features which the corresponding backer supports. */ > struct dpif_backer_support { > - /* True if the datapath supports variable-length > - * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. > - * False if the datapath supports only 8-byte (or shorter) userdata. */ > - bool variable_length_userdata; > - > - /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET > - * actions. */ > - bool masked_set_action; > - > - /* True if the datapath supports tnl_push and pop actions. */ > - bool tnl_push_pop; > - > - /* True if the datapath supports OVS_FLOW_ATTR_UFID. */ > - bool ufid; > - > - /* True if the datapath supports OVS_ACTION_ATTR_TRUNC action. */ > - bool trunc; > +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) TYPE NAME; > + DPIF_SUPPORT_FIELDS > +#undef DPIF_SUPPORT_FIELD > > /* Each member represents support for related OVS_KEY_ATTR_* fields. */ > struct odp_support odp; > - > - /* True if the datapath supports OVS_ACTION_ATTR_CLONE action. */ > - bool clone; > - > - /* Maximum level of nesting allowed by OVS_ACTION_ATTR_SAMPLE action. */ > - size_t sample_nesting; > }; > > /* Reasons that we might need to revalidate every datapath flow, and > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/odp-util.h b/lib/odp-util.h index 50fa1d1..48449e7 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -167,30 +167,34 @@ int odp_flow_from_string(const char *s, const struct simap *port_names, struct ofpbuf *, struct ofpbuf *); +#define ODP_SUPPORT_FIELDS \ + /* Maximum number of 802.1q VLAN headers to serialize in a mask. */ \ + ODP_SUPPORT_FIELD(size_t, max_vlan_headers, "Max Vlan headers") \ + /* Maximum number of MPLS label stack entries to serialise in a mask. */ \ + ODP_SUPPORT_FIELD(size_t, max_mpls_depth, "Max MPLS depth") \ + /* If this is true, then recirculation fields will always be \ + * serialised. */ \ + ODP_SUPPORT_FIELD(bool, recirc, "Recirc") \ + /* If true, serialise the corresponding OVS_KEY_ATTR_CONN_* field. */ \ + ODP_SUPPORT_FIELD(bool, ct_state, "CT state") \ + ODP_SUPPORT_FIELD(bool, ct_zone, "CT zone") \ + ODP_SUPPORT_FIELD(bool, ct_mark, "CT mark") \ + ODP_SUPPORT_FIELD(bool, ct_label, "CT label") \ + \ + /* If true, it means that the datapath supports the NAT bits in \ + * 'ct_state'. The above 'ct_state' member must be true for this \ + * to make sense */ \ + ODP_SUPPORT_FIELD(bool, ct_state_nat, "CT state NAT") \ + \ + /* Conntrack original direction tuple matching * supported. */ \ + ODP_SUPPORT_FIELD(bool, ct_orig_tuple, "CT orig tuple") + /* Indicates support for various fields. This defines how flows will be * serialised. */ struct odp_support { - /* Maximum number of 802.1q VLAN headers to serialize in a mask. */ - size_t max_vlan_headers; - /* Maximum number of MPLS label stack entries to serialise in a mask. */ - size_t max_mpls_depth; - - /* If this is true, then recirculation fields will always be serialised. */ - bool recirc; - - /* If true, serialise the corresponding OVS_KEY_ATTR_CONN_* field. */ - bool ct_state; - bool ct_zone; - bool ct_mark; - bool ct_label; - - /* If true, it means that the datapath supports the NAT bits in - * 'ct_state'. The above 'ct_state' member must be true for this - * to make sense */ - bool ct_state_nat; - - bool ct_orig_tuple; /* Conntrack original direction tuple matching - * supported. */ +#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) TYPE NAME; + ODP_SUPPORT_FIELDS +#undef ODP_SUPPORT_FIELD }; struct odp_flow_key_parms { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 1c4c804..b32428c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4928,13 +4928,13 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, } static void -show_dp_feature_b(struct ds *ds, const char *feature, bool b) +show_dp_feature_bool(struct ds *ds, const char *feature, bool b) { ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No"); } static void -show_dp_feature_s(struct ds *ds, const char *feature, size_t s) +show_dp_feature_size_t(struct ds *ds, const char *feature, size_t s) { ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s); } @@ -4942,21 +4942,15 @@ show_dp_feature_s(struct ds *ds, const char *feature, size_t s) static void dpif_show_support(const struct dpif_backer_support *support, struct ds *ds) { - show_dp_feature_b(ds, "Variable length userdata", - support->variable_length_userdata); - show_dp_feature_b(ds, "Masked set action", support->masked_set_action); - show_dp_feature_b(ds, "Tunnel push pop", support->tnl_push_pop); - show_dp_feature_b(ds, "Ufid", support->ufid); - show_dp_feature_b(ds, "Trunc action", support->trunc); - show_dp_feature_b(ds, "Clone action", support->clone); - show_dp_feature_s(ds, "Max MPLS depth", support->odp.max_mpls_depth); - show_dp_feature_b(ds, "Recirc", support->odp.recirc); - show_dp_feature_b(ds, "CT state", support->odp.ct_state); - show_dp_feature_b(ds, "CT zone", support->odp.ct_zone); - show_dp_feature_b(ds, "CT mark", support->odp.ct_mark); - show_dp_feature_b(ds, "CT label", support->odp.ct_label); - show_dp_feature_b(ds, "CT State NAT", support->odp.ct_state_nat); - show_dp_feature_s(ds, "Max sample nesting", support->sample_nesting); +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ + show_dp_feature_##TYPE (ds, TITLE, support->NAME); + DPIF_SUPPORT_FIELDS +#undef DPIF_SUPPORT_FIELD + +#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ + show_dp_feature_##TYPE (ds, TITLE, support->odp.NAME ); + ODP_SUPPORT_FIELDS +#undef ODP_SUPPORT_FIELD } static void diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index cc514d2..0562d02 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -143,34 +143,40 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, * ofproto-dpif) resides. A backer can host several bridges, but a bridge is * backed by only a single dpif. */ +#define DPIF_SUPPORT_FIELDS \ + /* True if the datapath supports variable-length \ + * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. \ + * False if the datapath supports only 8-byte (or shorter) userdata. */ \ + DPIF_SUPPORT_FIELD(bool, variable_length_userdata, \ + "Variable length userdata") \ + \ + /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET \ + * actions. */ \ + DPIF_SUPPORT_FIELD(bool, masked_set_action, "Masked set action") \ + \ + /* True if the datapath supports tnl_push and pop actions. */ \ + DPIF_SUPPORT_FIELD(bool, tnl_push_pop, "Tunnel push pop") \ + \ + /* True if the datapath supports OVS_FLOW_ATTR_UFID. */ \ + DPIF_SUPPORT_FIELD(bool, ufid, "Ufid") \ + \ + /* True if the datapath supports OVS_ACTION_ATTR_TRUNC action. */ \ + DPIF_SUPPORT_FIELD(bool, trunc, "Trunk action") \ + \ + /* True if the datapath supports OVS_ACTION_ATTR_CLONE action. */ \ + DPIF_SUPPORT_FIELD(bool, clone, "Clone action") \ + \ + /* Maximum level of nesting allowed by OVS_ACTION_ATTR_SAMPLE action. */\ + DPIF_SUPPORT_FIELD(size_t, sample_nesting, "Sample neesting") + /* Stores the various features which the corresponding backer supports. */ struct dpif_backer_support { - /* True if the datapath supports variable-length - * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. - * False if the datapath supports only 8-byte (or shorter) userdata. */ - bool variable_length_userdata; - - /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET - * actions. */ - bool masked_set_action; - - /* True if the datapath supports tnl_push and pop actions. */ - bool tnl_push_pop; - - /* True if the datapath supports OVS_FLOW_ATTR_UFID. */ - bool ufid; - - /* True if the datapath supports OVS_ACTION_ATTR_TRUNC action. */ - bool trunc; +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) TYPE NAME; + DPIF_SUPPORT_FIELDS +#undef DPIF_SUPPORT_FIELD /* Each member represents support for related OVS_KEY_ATTR_* fields. */ struct odp_support odp; - - /* True if the datapath supports OVS_ACTION_ATTR_CLONE action. */ - bool clone; - - /* Maximum level of nesting allowed by OVS_ACTION_ATTR_SAMPLE action. */ - size_t sample_nesting; }; /* Reasons that we might need to revalidate every datapath flow, and
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 <azhou@ovn.org> --- lib/odp-util.h | 46 ++++++++++++++++++++++++-------------------- ofproto/ofproto-dpif.c | 28 +++++++++++---------------- ofproto/ofproto-dpif.h | 52 ++++++++++++++++++++++++++++---------------------- 3 files changed, 65 insertions(+), 61 deletions(-)