[ovs-dev] [PATCH 2/2] odp-util: Use port names in output in more places.

Ben Pfaff blp at ovn.org
Fri Jun 23 09:18:25 UTC 2017


Thanks for looking at these and testing them.  I applied them to master.

(There is still some room for improvement in these areas, but I think
that this is a useful step forward.)

On Fri, Jun 23, 2017 at 08:11:50AM +0000, Jan Scheurich wrote:
> Acked-by: Jan Scheurich <jan.scheurich at ericsson.com>
> Tested-by: Jan Scheurich <jan.scheurich at ericsson.com>
> 
> > -----Original Message-----
> > From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Ben Pfaff
> > Sent: Tuesday, 20 June, 2017 04:26
> > To: dev at openvswitch.org
> > Cc: Ben Pfaff <blp at ovn.org>
> > Subject: [ovs-dev] [PATCH 2/2] odp-util: Use port names in output in more places.
> > 
> > Until now, ODP output only showed port names for in_port matches.  This
> > commit shows them in other places port numbers appear.
> > 
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> >  lib/dpctl.c                  |  8 ++--
> >  lib/dpif-netdev.c            |  2 +-
> >  lib/dpif.c                   |  4 +-
> >  lib/odp-util.c               | 98 +++++++++++++++++++++++++++-----------------
> >  lib/odp-util.h               |  5 ++-
> >  ofproto/ofproto-dpif-trace.c |  2 +-
> >  ofproto/ofproto-dpif.c       |  2 +-
> >  tests/test-odp.c             |  2 +-
> >  8 files changed, 74 insertions(+), 49 deletions(-)
> > 
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 1e3bf0a517db..1543c1cf3240 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -756,7 +756,7 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
> >          ds_put_cstr(ds, ", offloaded:yes");
> >      }
> >      ds_put_cstr(ds, ", actions:");
> > -    format_odp_actions(ds, f->actions, f->actions_len);
> > +    format_odp_actions(ds, f->actions, f->actions_len, ports);
> >  }
> > 
> >  static char *supported_dump_types[] = {
> > @@ -1348,7 +1348,7 @@ dpctl_parse_actions(int argc, const char *argv[], struct dpctl_params* dpctl_p)
> >          }
> > 
> >          ds_init(&s);
> > -        format_odp_actions(&s, actions.data, actions.size);
> > +        format_odp_actions(&s, actions.data, actions.size, NULL);
> >          dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
> >          ds_destroy(&s);
> > 
> > @@ -1513,7 +1513,7 @@ dpctl_normalize_actions(int argc, const char *argv[],
> > 
> >      if (dpctl_p->verbosity) {
> >          ds_clear(&s);
> > -        format_odp_actions(&s, odp_actions.data, odp_actions.size);
> > +        format_odp_actions(&s, odp_actions.data, odp_actions.size, NULL);
> >          dpctl_print(dpctl_p, "input actions: %s\n", ds_cstr(&s));
> >      }
> > 
> > @@ -1583,7 +1583,7 @@ dpctl_normalize_actions(int argc, const char *argv[],
> >          }
> > 
> >          ds_clear(&s);
> > -        format_odp_actions(&s, af->actions.data, af->actions.size);
> > +        format_odp_actions(&s, af->actions.data, af->actions.size, NULL);
> >          dpctl_puts(dpctl_p, false, ds_cstr(&s));
> > 
> >          ofpbuf_uninit(&af->actions);
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index f97e97ab2931..97fab8319b9c 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2417,7 +2417,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> >                          mask_buf.data, mask_buf.size,
> >                          NULL, &ds, false);
> >          ds_put_cstr(&ds, ", actions:");
> > -        format_odp_actions(&ds, actions, actions_len);
> > +        format_odp_actions(&ds, actions, actions_len, NULL);
> > 
> >          VLOG_DBG("%s", ds_cstr(&ds));
> > 
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 10bdd7055d5f..2ed0ba02f2ce 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1714,7 +1714,7 @@ log_flow_message(const struct dpif *dpif, int error,
> >      }
> >      if (actions || actions_len) {
> >          ds_put_cstr(&ds, ", actions:");
> > -        format_odp_actions(&ds, actions, actions_len);
> > +        format_odp_actions(&ds, actions, actions_len, NULL);
> >      }
> >      vlog(module, flow_message_log_level(error), "%s", ds_cstr(&ds));
> >      ds_destroy(&ds);
> > @@ -1802,7 +1802,7 @@ log_execute_message(const struct dpif *dpif,
> >                        (subexecute ? "sub-"
> >                         : dpif_execute_needs_help(execute) ? "super-"
> >                         : ""));
> > -        format_odp_actions(&ds, execute->actions, execute->actions_len);
> > +        format_odp_actions(&ds, execute->actions, execute->actions_len, NULL);
> >          if (error) {
> >              ds_put_format(&ds, " failed (%s)", ovs_strerror(error));
> >          }
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index d15095558243..6c2ab6cc5799 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -203,7 +203,8 @@ format_generic_odp_action(struct ds *ds, const struct nlattr *a)
> >  }
> > 
> >  static void
> > -format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
> > +format_odp_sample_action(struct ds *ds, const struct nlattr *attr,
> > +                         const struct hmap *portno_names)
> >  {
> >      static const struct nl_policy ovs_sample_policy[] = {
> >          [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32 },
> > @@ -229,19 +230,20 @@ format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
> >      ds_put_cstr(ds, "actions(");
> >      nla_acts = nl_attr_get(a[OVS_SAMPLE_ATTR_ACTIONS]);
> >      len = nl_attr_get_size(a[OVS_SAMPLE_ATTR_ACTIONS]);
> > -    format_odp_actions(ds, nla_acts, len);
> > +    format_odp_actions(ds, nla_acts, len, portno_names);
> >      ds_put_format(ds, "))");
> >  }
> > 
> >  static void
> > -format_odp_clone_action(struct ds *ds, const struct nlattr *attr)
> > +format_odp_clone_action(struct ds *ds, const struct nlattr *attr,
> > +                        const struct hmap *portno_names)
> >  {
> >      const struct nlattr *nla_acts = nl_attr_get(attr);
> >      int len = nl_attr_get_size(attr);
> > 
> >      ds_put_cstr(ds, "clone");
> >      ds_put_format(ds, "(");
> > -    format_odp_actions(ds, nla_acts, len);
> > +    format_odp_actions(ds, nla_acts, len, portno_names);
> >      ds_put_format(ds, ")");
> >  }
> > 
> > @@ -278,7 +280,8 @@ parse_odp_flags(const char *s, const char *(*bit_to_string)(uint32_t),
> >  }
> > 
> >  static void
> > -format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
> > +format_odp_userspace_action(struct ds *ds, const struct nlattr *attr,
> > +                            const struct hmap *portno_names)
> >  {
> >      static const struct nl_policy ovs_userspace_policy[] = {
> >          [OVS_USERSPACE_ATTR_PID] = { .type = NL_A_U32 },
> > @@ -336,12 +339,13 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
> >                                ",collector_set_id=%"PRIu32
> >                                ",obs_domain_id=%"PRIu32
> >                                ",obs_point_id=%"PRIu32
> > -                              ",output_port=%"PRIu32,
> > +                              ",output_port=",
> >                                cookie.flow_sample.probability,
> >                                cookie.flow_sample.collector_set_id,
> >                                cookie.flow_sample.obs_domain_id,
> > -                              cookie.flow_sample.obs_point_id,
> > -                              cookie.flow_sample.output_odp_port);
> > +                              cookie.flow_sample.obs_point_id);
> > +                odp_portno_name_format(portno_names,
> > +                                       cookie.flow_sample.output_odp_port, ds);
> >                  if (cookie.flow_sample.direction == NX_ACTION_SAMPLE_INGRESS) {
> >                      ds_put_cstr(ds, ",ingress");
> >                  } else if (cookie.flow_sample.direction == NX_ACTION_SAMPLE_EGRESS) {
> > @@ -350,8 +354,10 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
> >                  ds_put_char(ds, ')');
> >              } else if (userdata_len >= sizeof cookie.ipfix
> >                         && cookie.type == USER_ACTION_COOKIE_IPFIX) {
> > -                ds_put_format(ds, ",ipfix(output_port=%"PRIu32")",
> > -                              cookie.ipfix.output_odp_port);
> > +                ds_put_format(ds, ",ipfix(output_port=");
> > +                odp_portno_name_format(portno_names,
> > +                                       cookie.ipfix.output_odp_port, ds);
> > +                ds_put_char(ds, ')');
> >              } else {
> >                  userdata_unspec = true;
> >              }
> > @@ -373,8 +379,9 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
> > 
> >      tunnel_out_port_attr = a[OVS_USERSPACE_ATTR_EGRESS_TUN_PORT];
> >      if (tunnel_out_port_attr) {
> > -        ds_put_format(ds, ",tunnel_out_port=%"PRIu32,
> > -                      nl_attr_get_u32(tunnel_out_port_attr));
> > +        ds_put_format(ds, ",tunnel_out_port=");
> > +        odp_portno_name_format(portno_names,
> > +                               nl_attr_get_odp_port(tunnel_out_port_attr), ds);
> >      }
> > 
> >      ds_put_char(ds, ')');
> > @@ -571,15 +578,20 @@ format_odp_tnl_push_header(struct ds *ds, struct ovs_action_push_tnl *data)
> >  }
> > 
> >  static void
> > -format_odp_tnl_push_action(struct ds *ds, const struct nlattr *attr)
> > +format_odp_tnl_push_action(struct ds *ds, const struct nlattr *attr,
> > +                           const struct hmap *portno_names)
> >  {
> >      struct ovs_action_push_tnl *data;
> > 
> >      data = (struct ovs_action_push_tnl *) nl_attr_get(attr);
> > 
> > -    ds_put_format(ds, "tnl_push(tnl_port(%"PRIu32"),", data->tnl_port);
> > +    ds_put_cstr(ds, "tnl_push(tnl_port(");
> > +    odp_portno_name_format(portno_names, data->tnl_port, ds);
> > +    ds_put_cstr(ds, "),");
> >      format_odp_tnl_push_header(ds, data);
> > -    ds_put_format(ds, ",out_port(%"PRIu32"))", data->out_port);
> > +    ds_put_format(ds, ",out_port(");
> > +    odp_portno_name_format(portno_names, data->out_port, ds);
> > +    ds_put_cstr(ds, "))");
> >  }
> > 
> >  static const struct nl_policy ovs_nat_policy[] = {
> > @@ -799,7 +811,8 @@ format_odp_conntrack_action(struct ds *ds, const struct nlattr *attr)
> >  }
> > 
> >  static void
> > -format_odp_action(struct ds *ds, const struct nlattr *a)
> > +format_odp_action(struct ds *ds, const struct nlattr *a,
> > +                  const struct hmap *portno_names)
> >  {
> >      int expected_len;
> >      enum ovs_action_attr type = nl_attr_type(a);
> > @@ -819,7 +832,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> >          ds_put_format(ds, "meter(%"PRIu32")", nl_attr_get_u32(a));
> >          break;
> >      case OVS_ACTION_ATTR_OUTPUT:
> > -        ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
> > +        odp_portno_name_format(portno_names, nl_attr_get_odp_port(a), ds);
> >          break;
> >      case OVS_ACTION_ATTR_TRUNC: {
> >          const struct ovs_action_trunc *trunc =
> > @@ -828,15 +841,16 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> >          ds_put_format(ds, "trunc(%"PRIu32")", trunc->max_len);
> >          break;
> >      }
> > -    break;
> >      case OVS_ACTION_ATTR_TUNNEL_POP:
> > -        ds_put_format(ds, "tnl_pop(%"PRIu32")", nl_attr_get_u32(a));
> > +        ds_put_cstr(ds, "tnl_pop(");
> > +        odp_portno_name_format(portno_names, nl_attr_get_odp_port(a), ds);
> > +        ds_put_char(ds, ')');
> >          break;
> >      case OVS_ACTION_ATTR_TUNNEL_PUSH:
> > -        format_odp_tnl_push_action(ds, a);
> > +        format_odp_tnl_push_action(ds, a, portno_names);
> >          break;
> >      case OVS_ACTION_ATTR_USERSPACE:
> > -        format_odp_userspace_action(ds, a);
> > +        format_odp_userspace_action(ds, a, portno_names);
> >          break;
> >      case OVS_ACTION_ATTR_RECIRC:
> >          format_odp_recirc_action(ds, nl_attr_get_u32(a));
> > @@ -907,13 +921,13 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> >          break;
> >      }
> >      case OVS_ACTION_ATTR_SAMPLE:
> > -        format_odp_sample_action(ds, a);
> > +        format_odp_sample_action(ds, a, portno_names);
> >          break;
> >      case OVS_ACTION_ATTR_CT:
> >          format_odp_conntrack_action(ds, a);
> >          break;
> >      case OVS_ACTION_ATTR_CLONE:
> > -        format_odp_clone_action(ds, a);
> > +        format_odp_clone_action(ds, a, portno_names);
> >          break;
> >      case OVS_ACTION_ATTR_UNSPEC:
> >      case __OVS_ACTION_ATTR_MAX:
> > @@ -925,7 +939,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
> > 
> >  void
> >  format_odp_actions(struct ds *ds, const struct nlattr *actions,
> > -                   size_t actions_len)
> > +                   size_t actions_len, const struct hmap *portno_names)
> >  {
> >      if (actions_len) {
> >          const struct nlattr *a;
> > @@ -935,7 +949,7 @@ format_odp_actions(struct ds *ds, const struct nlattr *actions,
> >              if (a != actions) {
> >                  ds_put_char(ds, ',');
> >              }
> > -            format_odp_action(ds, a);
> > +            format_odp_action(ds, a, portno_names);
> >          }
> >          if (left) {
> >              int i;
> > @@ -2241,12 +2255,14 @@ odp_portno_names_set(struct hmap *portno_names, odp_port_t port_no,
> >  static char *
> >  odp_portno_names_get(const struct hmap *portno_names, odp_port_t port_no)
> >  {
> > -    struct odp_portno_names *odp_portno_names;
> > +    if (portno_names) {
> > +        struct odp_portno_names *odp_portno_names;
> > 
> > -    HMAP_FOR_EACH_IN_BUCKET (odp_portno_names, hmap_node,
> > -                             hash_odp_port(port_no), portno_names) {
> > -        if (odp_portno_names->port_no == port_no) {
> > -            return odp_portno_names->name;
> > +        HMAP_FOR_EACH_IN_BUCKET (odp_portno_names, hmap_node,
> > +                                 hash_odp_port(port_no), portno_names) {
> > +            if (odp_portno_names->port_no == port_no) {
> > +                return odp_portno_names->name;
> > +            }
> >          }
> >      }
> >      return NULL;
> > @@ -2263,6 +2279,18 @@ odp_portno_names_destroy(struct hmap *portno_names)
> >      }
> >  }
> > 
> > +void
> > +odp_portno_name_format(const struct hmap *portno_names, odp_port_t port_no,
> > +                       struct ds *s)
> > +{
> > +    const char *name = odp_portno_names_get(portno_names, port_no);
> > +    if (name) {
> > +        ds_put_cstr(s, name);
> > +    } else {
> > +        ds_put_format(s, "%"PRIu32, port_no);
> > +    }
> > +}
> > +
> >  /* Format helpers. */
> > 
> >  static void
> > @@ -2944,14 +2972,8 @@ format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
> >          break;
> > 
> >      case OVS_KEY_ATTR_IN_PORT:
> > -        if (portno_names && is_exact) {
> > -            char *name = odp_portno_names_get(portno_names,
> > -                                              nl_attr_get_odp_port(a));
> > -            if (name) {
> > -                ds_put_format(ds, "%s", name);
> > -            } else {
> > -                ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
> > -            }
> > +        if (is_exact) {
> > +            odp_portno_name_format(portno_names, nl_attr_get_odp_port(a), ds);
> >          } else {
> >              ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
> >              if (!is_exact) {
> > diff --git a/lib/odp-util.h b/lib/odp-util.h
> > index 12fd834e7405..fe183a93d91c 100644
> > --- a/lib/odp-util.h
> > +++ b/lib/odp-util.h
> > @@ -78,7 +78,7 @@ const char *slow_path_reason_to_explanation(enum slow_path_reason);
> >  #define ODPP_NONE  ODP_PORT_C(UINT32_MAX)
> > 
> >  void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
> > -                        size_t actions_len);
> > +                        size_t actions_len, const struct hmap *portno_names);
> >  int odp_actions_from_string(const char *, const struct simap *port_names,
> >                              struct ofpbuf *odp_actions);
> > 
> > @@ -92,6 +92,9 @@ struct odp_portno_names {
> >  void odp_portno_names_set(struct hmap *portno_names, odp_port_t port_no,
> >                            char *port_name);
> >  void odp_portno_names_destroy(struct hmap *portno_names);
> > +void odp_portno_name_format(const struct hmap *portno_names,
> > +                            odp_port_t, struct ds *);
> > +
> >  /* The maximum number of bytes that odp_flow_key_from_flow() appends to a
> >   * buffer.  This is the upper bound on the length of a nlattr-formatted flow
> >   * key that ovs-vswitchd fully understands.
> > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> > index c7f0e48caa16..56bf93075636 100644
> > --- a/ofproto/ofproto-dpif-trace.c
> > +++ b/ofproto/ofproto-dpif-trace.c
> > @@ -475,7 +475,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
> >      ds_put_char(output, '\n');
> > 
> >      ds_put_cstr(output, "Datapath actions: ");
> > -    format_odp_actions(output, odp_actions.data, odp_actions.size);
> > +    format_odp_actions(output, odp_actions.data, odp_actions.size, NULL);
> > 
> >      if (error != XLATE_OK) {
> >          ds_put_format(output,
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 28b24b9a0fd7..cc325ddd7a37 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5303,7 +5303,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
> >          ds_put_cstr(&ds, ", ");
> >          dpif_flow_stats_format(&f.stats, &ds);
> >          ds_put_cstr(&ds, ", actions:");
> > -        format_odp_actions(&ds, f.actions, f.actions_len);
> > +        format_odp_actions(&ds, f.actions, f.actions_len, portno_names);
> >          ds_put_char(&ds, '\n');
> >      }
> >      dpif_flow_dump_thread_destroy(flow_dump_thread);
> > diff --git a/tests/test-odp.c b/tests/test-odp.c
> > index db95cfbae5ae..bccdcf8bde33 100644
> > --- a/tests/test-odp.c
> > +++ b/tests/test-odp.c
> > @@ -138,7 +138,7 @@ parse_actions(void)
> > 
> >          /* Convert odp_actions back to string. */
> >          ds_init(&out);
> > -        format_odp_actions(&out, odp_actions.data, odp_actions.size);
> > +        format_odp_actions(&out, odp_actions.data, odp_actions.size, NULL);
> >          puts(ds_cstr(&out));
> >          ds_destroy(&out);
> > 
> > --
> > 2.10.2
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list