Re: [ovs-dev] [PATCH v2 1/3] ofp-actions: Make formatting and parsing functions take a struct argument.

[Yifeng Sun]

Looks good to me, thanks.

Reviewed-by: Yifeng Sun 


On Fri, Jan 12, 2018 at 12:57 PM, Ben Pfaff  wrote:

> An upcoming commit will add another parameter for parsing and formatting
> actions.  It is much easier to add these parameters if they are
> encapsulated in a struct, so this commit first makes that change.
>
> Signed-off-by: Ben Pfaff 
> ---
>  include/openvswitch/ofp-actions.h |   32 +-
>  lib/ofp-actions.c | 1137 +++---
> ---
>  lib/ofp-parse.c   |   24 +-
>  lib/ofp-print.c   |   40 +-
>  ofproto/ofproto-dpif-trace.c  |8 +-
>  ofproto/ofproto-dpif-xlate.c  |   12 +-
>  ofproto/ofproto.c |3 +-
>  ovn/controller/ofctrl.c   |3 +-
>  ovn/utilities/ovn-sbctl.c |3 +-
>  tests/test-ovn.c  |3 +-
>  utilities/ovs-ofctl.c |   19 +-
>  11 files changed, 565 insertions(+), 719 deletions(-)
>
> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-
> actions.h
> index 4e957358f90a..454c705ccf73 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1064,18 +1064,32 @@ bool ofpacts_equal_stringwise(const struct ofpact
> a[], size_t a_len,
>  const struct mf_field *ofpact_get_mf_dst(const struct ofpact *ofpact);
>  uint32_t ofpacts_get_meter(const struct ofpact[], size_t ofpacts_len);
>
> -/* Formatting and parsing ofpacts. */
> +/* Formatting ofpacts. */
> +struct ofpact_format_params {
> +/* Input. */
> +const struct ofputil_port_map *port_map;
> +
> +/* Output. */
> +struct ds *s;
> +};
>  void ofpacts_format(const struct ofpact[], size_t ofpacts_len,
> -const struct ofputil_port_map *, struct ds *);
> -char *ofpacts_parse_actions(const char *, const struct ofputil_port_map *,
> -struct ofpbuf *ofpacts,
> -enum ofputil_protocol *usable_protocols)
> +const struct ofpact_format_params *);
> +const char *ofpact_name(enum ofpact_type);
> +
> +/* Parsing ofpacts. */
> +struct ofpact_parse_params {
> +/* Input. */
> +const struct ofputil_port_map *port_map;
> +
> +/* Output. */
> +struct ofpbuf *ofpacts;
> +enum ofputil_protocol *usable_protocols;
> +};
> +char *ofpacts_parse_actions(const char *, const struct
> ofpact_parse_params *)
>  OVS_WARN_UNUSED_RESULT;
> -char *ofpacts_parse_instructions(const char *, const struct
> ofputil_port_map *,
> - struct ofpbuf *ofpacts,
> - enum ofputil_protocol *usable_protocols)
> +char *ofpacts_parse_instructions(const char *,
> + const struct ofpact_parse_params *)
>  OVS_WARN_UNUSED_RESULT;
> -const char *ofpact_name(enum ofpact_type);
>
>  /* Internal use by the helpers below. */
>  void ofpact_init(struct ofpact *, enum ofpact_type, size_t len);
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index db933634bf8b..93792ddfca4b 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -417,8 +417,7 @@ static void *ofpact_put_raw(struct ofpbuf *, enum
> ofp_version,
>  enum ofp_raw_action_type, uint64_t arg);
>
>  static char *OVS_WARN_UNUSED_RESULT ofpacts_parse(
> -char *str, const struct ofputil_port_map *,
> -struct ofpbuf *ofpacts, enum ofputil_protocol *usable_protocols,
> +char *str, const struct ofpact_parse_params *pp,
>  bool allow_instructions, enum ofpact_type outer_action);
>  static enum ofperr ofpacts_pull_openflow_actions__(
>  struct ofpbuf *openflow, unsigned int actions_len,
> @@ -426,8 +425,7 @@ static enum ofperr ofpacts_pull_openflow_actions__(
>  struct ofpbuf *ofpacts, enum ofpact_type outer_action,
>  const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap);
>  static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
> -const char *s_, const struct ofputil_port_map *, struct ofpbuf
> *ofpacts,
> -enum ofputil_protocol *usable_protocols,
> +const char *s_, const struct ofpact_parse_params *pp,
>  bool allow_instructions, enum ofpact_type outer_action);
>
>  /* Returns the ofpact following 'ofpact', except that if 'ofpact' contains
> @@ -607,16 +605,16 @@ encode_OUTPUT(const struct ofpact_output *output,
>  }
>
>  static char * OVS_WARN_UNUSED_RESULT
> -parse_truncate_subfield(struct ofpact_output_trunc *output_trunc,
> -const char *arg_,
> -const struct ofputil_port_map *port_map)
> +parse_truncate_subfield(const char *arg_,
> +const struct ofpact_parse_params *pp,
> +struct ofpact_output_trunc *output_trunc)
>  {
>  char *key, *value;
>  char *arg = CONST_CAST(char *, arg_);
>
>  while (ofputil_parse_key_value(, , )) {
>  if (!strcmp(key, 

[ovs-dev] [PATCH v2 1/3] ofp-actions: Make formatting and parsing functions take a struct argument.

[Ben Pfaff]

An upcoming commit will add another parameter for parsing and formatting
actions.  It is much easier to add these parameters if they are
encapsulated in a struct, so this commit first makes that change.

Signed-off-by: Ben Pfaff 
---
 include/openvswitch/ofp-actions.h |   32 +-
 lib/ofp-actions.c | 1137 +++--
 lib/ofp-parse.c   |   24 +-
 lib/ofp-print.c   |   40 +-
 ofproto/ofproto-dpif-trace.c  |8 +-
 ofproto/ofproto-dpif-xlate.c  |   12 +-
 ofproto/ofproto.c |3 +-
 ovn/controller/ofctrl.c   |3 +-
 ovn/utilities/ovn-sbctl.c |3 +-
 tests/test-ovn.c  |3 +-
 utilities/ovs-ofctl.c |   19 +-
 11 files changed, 565 insertions(+), 719 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 4e957358f90a..454c705ccf73 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1064,18 +1064,32 @@ bool ofpacts_equal_stringwise(const struct ofpact a[], 
size_t a_len,
 const struct mf_field *ofpact_get_mf_dst(const struct ofpact *ofpact);
 uint32_t ofpacts_get_meter(const struct ofpact[], size_t ofpacts_len);
 
-/* Formatting and parsing ofpacts. */
+/* Formatting ofpacts. */
+struct ofpact_format_params {
+/* Input. */
+const struct ofputil_port_map *port_map;
+
+/* Output. */
+struct ds *s;
+};
 void ofpacts_format(const struct ofpact[], size_t ofpacts_len,
-const struct ofputil_port_map *, struct ds *);
-char *ofpacts_parse_actions(const char *, const struct ofputil_port_map *,
-struct ofpbuf *ofpacts,
-enum ofputil_protocol *usable_protocols)
+const struct ofpact_format_params *);
+const char *ofpact_name(enum ofpact_type);
+
+/* Parsing ofpacts. */
+struct ofpact_parse_params {
+/* Input. */
+const struct ofputil_port_map *port_map;
+
+/* Output. */
+struct ofpbuf *ofpacts;
+enum ofputil_protocol *usable_protocols;
+};
+char *ofpacts_parse_actions(const char *, const struct ofpact_parse_params *)
 OVS_WARN_UNUSED_RESULT;
-char *ofpacts_parse_instructions(const char *, const struct ofputil_port_map *,
- struct ofpbuf *ofpacts,
- enum ofputil_protocol *usable_protocols)
+char *ofpacts_parse_instructions(const char *,
+ const struct ofpact_parse_params *)
 OVS_WARN_UNUSED_RESULT;
-const char *ofpact_name(enum ofpact_type);
 
 /* Internal use by the helpers below. */
 void ofpact_init(struct ofpact *, enum ofpact_type, size_t len);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index db933634bf8b..93792ddfca4b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -417,8 +417,7 @@ static void *ofpact_put_raw(struct ofpbuf *, enum 
ofp_version,
 enum ofp_raw_action_type, uint64_t arg);
 
 static char *OVS_WARN_UNUSED_RESULT ofpacts_parse(
-char *str, const struct ofputil_port_map *,
-struct ofpbuf *ofpacts, enum ofputil_protocol *usable_protocols,
+char *str, const struct ofpact_parse_params *pp,
 bool allow_instructions, enum ofpact_type outer_action);
 static enum ofperr ofpacts_pull_openflow_actions__(
 struct ofpbuf *openflow, unsigned int actions_len,
@@ -426,8 +425,7 @@ static enum ofperr ofpacts_pull_openflow_actions__(
 struct ofpbuf *ofpacts, enum ofpact_type outer_action,
 const struct vl_mff_map *vl_mff_map, uint64_t *ofpacts_tlv_bitmap);
 static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
-const char *s_, const struct ofputil_port_map *, struct ofpbuf *ofpacts,
-enum ofputil_protocol *usable_protocols,
+const char *s_, const struct ofpact_parse_params *pp,
 bool allow_instructions, enum ofpact_type outer_action);
 
 /* Returns the ofpact following 'ofpact', except that if 'ofpact' contains
@@ -607,16 +605,16 @@ encode_OUTPUT(const struct ofpact_output *output,
 }
 
 static char * OVS_WARN_UNUSED_RESULT
-parse_truncate_subfield(struct ofpact_output_trunc *output_trunc,
-const char *arg_,
-const struct ofputil_port_map *port_map)
+parse_truncate_subfield(const char *arg_,
+const struct ofpact_parse_params *pp,
+struct ofpact_output_trunc *output_trunc)
 {
 char *key, *value;
 char *arg = CONST_CAST(char *, arg_);
 
 while (ofputil_parse_key_value(, , )) {
 if (!strcmp(key, "port")) {
-if (!ofputil_port_from_string(value, port_map,
+if (!ofputil_port_from_string(value, pp->port_map,
   _trunc->port)) {
 return xasprintf("output to unknown truncate port: %s",
   value);
@@ -643,21 +641,18 @@