Re: [ovs-dev] [v2] ofproto: Use macros to define DPIF support fields

[Andy Zhou]

On Mon, Apr 3, 2017 at 5:30 PM, Joe Stringer  wrote:
> On 3 April 2017 at 13:30, Andy Zhou  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 

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 
>
>> 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.
>
> 
>
>> +/* 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

Re: [ovs-dev] [v2] ofproto: Use macros to define DPIF support fields

[Joe Stringer]

On 3 April 2017 at 13:30, Andy Zhou  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 
>
> ---

Looks like there's a few typos:

's/FILED/FIELD/g'

A couple more comments below, but otherwise LGTM. Thanks!

Acked-by: Joe Stringer 

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



> +/* 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?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [v2] ofproto: Use macros to define DPIF support fields

[Andy Zhou]

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 

---
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.   */
+#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 523adad6fa52..9556a0e0cd58 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4955,13 +4955,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);
 }
@@ -4969,21 +4969,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",
-