Thanks for implementing this! It's a great usability improvement for trouble-shooting datapath issues and makes it easier to write unit test cases checking on datapath flows.
Acked-by: Jan Scheurich <[email protected]> Tested-by: Jan Scheurich <[email protected]> > -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Ben Pfaff > Sent: Tuesday, 20 June, 2017 04:26 > To: [email protected] > Cc: Ben Pfaff <[email protected]> > Subject: [ovs-dev] [PATCH 1/2] ovs-dpctl: New --names option to use port > names in flow dumps. > > Until now, printing names in "ovs-dpctl dump-flows" was tied to the overall > output verbosity, which in practice meant that to see port names a user had > to see a distracting amount of verbosity. This decouples names from > verbosity. > > I'd like to make showing names the default for interactive usage, but so > far names aren't accepted in input so that would frustrate cut-and-paste, > which is an important use of "ovs-dpctl dump-flows" output. > > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/dpctl.c | 76 > ++++++++++++++++++++++++++++++++++-------------- > lib/dpctl.h | 3 ++ > lib/dpctl.man | 7 +++-- > lib/odp-util.c | 4 +-- > ofproto/ofproto-dpif.c | 48 +++++++++++++++++++++--------- > utilities/ovs-dpctl.8.in | 9 +++++- > utilities/ovs-dpctl.c | 20 +++++++++++++ > 7 files changed, 125 insertions(+), 42 deletions(-) > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 7f44d025dcf1..1e3bf0a517db 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -49,6 +49,8 @@ > #include "unixctl.h" > #include "util.h" > #include "openvswitch/ofp-parse.h" > +#include "openvswitch/vlog.h" > +VLOG_DEFINE_THIS_MODULE(dpctl); > > typedef int dpctl_command_handler(int argc, const char *argv[], > struct dpctl_params *); > @@ -762,6 +764,36 @@ static char *supported_dump_types[] = { > "ovs", > }; > > +static struct hmap * > +dpctl_get_portno_names(struct dpif *dpif, const struct dpctl_params *dpctl_p) > +{ > + if (dpctl_p->names) { > + struct hmap *portno_names = xmalloc(sizeof *portno_names); > + hmap_init(portno_names); > + > + struct dpif_port_dump port_dump; > + struct dpif_port dpif_port; > + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > + odp_portno_names_set(portno_names, dpif_port.port_no, > + dpif_port.name); > + } > + > + return portno_names; > + } else { > + return NULL; > + } > +} > + > +static void > +dpctl_free_portno_names(struct hmap *portno_names) > +{ > + if (portno_names) { > + odp_portno_names_destroy(portno_names); > + hmap_destroy(portno_names); > + free(portno_names); > + } > +} > + > static int > dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > { > @@ -774,10 +806,6 @@ dpctl_dump_flows(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > struct flow flow_filter; > struct flow_wildcards wc_filter; > > - struct dpif_port_dump port_dump; > - struct dpif_port dpif_port; > - struct hmap portno_names; > - > struct dpif_flow_dump_thread *flow_dump_thread; > struct dpif_flow_dump *flow_dump; > struct dpif_flow f; > @@ -807,15 +835,14 @@ dpctl_dump_flows(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > goto out_free; > } > > - > - hmap_init(&portno_names); > - DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > - odp_portno_names_set(&portno_names, dpif_port.port_no, > dpif_port.name); > - } > + struct hmap *portno_names = dpctl_get_portno_names(dpif, dpctl_p); > > if (filter) { > struct ofputil_port_map port_map; > ofputil_port_map_init(&port_map); > + > + struct dpif_port_dump port_dump; > + struct dpif_port dpif_port; > DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > ofputil_port_map_put(&port_map, > u16_to_ofp(odp_to_u32(dpif_port.port_no)), > @@ -890,7 +917,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > } > pmd_id = f.pmd_id; > } > - format_dpif_flow(&ds, &f, &portno_names, type, dpctl_p); > + format_dpif_flow(&ds, &f, portno_names, type, dpctl_p); > > dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); > } > @@ -903,8 +930,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > ds_destroy(&ds); > > out_dpifclose: > - odp_portno_names_destroy(&portno_names); > - hmap_destroy(&portno_names); > + dpctl_free_portno_names(portno_names); > dpif_close(dpif); > out_free: > free(filter); > @@ -1032,11 +1058,8 @@ dpctl_get_flow(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > { > const char *key_s = argv[argc - 1]; > struct dpif_flow flow; > - struct dpif_port dpif_port; > - struct dpif_port_dump port_dump; > struct dpif *dpif; > char *dp_name; > - struct hmap portno_names; > ovs_u128 ufid; > struct ofpbuf buf; > uint64_t stub[DPIF_FLOW_BUFSIZE / 8]; > @@ -1055,10 +1078,8 @@ dpctl_get_flow(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > } > > ofpbuf_use_stub(&buf, &stub, sizeof stub); > - hmap_init(&portno_names); > - DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > - odp_portno_names_set(&portno_names, dpif_port.port_no, > dpif_port.name); > - } > + > + struct hmap *portno_names = dpctl_get_portno_names(dpif, dpctl_p); > > n = odp_ufid_from_string(key_s, &ufid); > if (n <= 0) { > @@ -1074,13 +1095,12 @@ dpctl_get_flow(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > } > > ds_init(&ds); > - format_dpif_flow(&ds, &flow, &portno_names, NULL, dpctl_p); > + format_dpif_flow(&ds, &flow, portno_names, NULL, dpctl_p); > dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); > ds_destroy(&ds); > > out: > - odp_portno_names_destroy(&portno_names); > - hmap_destroy(&portno_names); > + dpctl_free_portno_names(portno_names); > ofpbuf_uninit(&buf); > dpif_close(dpif); > return error; > @@ -1680,6 +1700,7 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int > argc, const char *argv[], > /* Parse options (like getopt). Unfortunately it does > * not seem a good idea to call getopt_long() here, since it uses global > * variables */ > + bool set_names = false; > while (argc > 1 && !error) { > const char *arg = argv[1]; > if (!strncmp(arg, "--", 2)) { > @@ -1692,6 +1713,12 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int > argc, const char *argv[], > dpctl_p.may_create = true; > } else if (!strcmp(arg, "--more")) { > dpctl_p.verbosity++; > + } else if (!strcmp(arg, "--names")) { > + dpctl_p.names = true; > + set_names = true; > + } else if (!strcmp(arg, "--no-names")) { > + dpctl_p.names = false; > + set_names = true; > } else { > ds_put_format(&ds, "Unrecognized option %s", argv[1]); > error = true; > @@ -1726,6 +1753,11 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int > argc, const char *argv[], > argv++; > argc--; > } > + if (!set_names) { > + dpctl_p.names = dpctl_p.verbosity > 0; > + } > + VLOG_INFO("set_names=%d verbosity=%d names=%d", set_names, > + dpctl_p.verbosity, dpctl_p.names); > > if (!error) { > dpctl_command_handler *handler = (dpctl_command_handler *) aux; > diff --git a/lib/dpctl.h b/lib/dpctl.h > index 4ee083fa7861..9d0052152a9a 100644 > --- a/lib/dpctl.h > +++ b/lib/dpctl.h > @@ -39,6 +39,9 @@ struct dpctl_params { > /* -m, --more: Increase output verbosity. */ > int verbosity; > > + /* --names: Use port names in output? */ > + bool names; > + > /* Callback for printing. This function is called from > dpctl_run_command() > * to output data. The 'aux' parameter is set to the 'aux' > * member. The 'error' parameter is true if 'string' is an error > diff --git a/lib/dpctl.man b/lib/dpctl.man > index f6e4a7a2fc2d..9ebd3963757c 100644 > --- a/lib/dpctl.man > +++ b/lib/dpctl.man > @@ -79,7 +79,7 @@ for datapath not implementing mega flow. "hit" displays the > total number > of masks visited for matching incoming packets. "total" displays number of > masks in the datapath. "hit/pkt" displays the average number of masks > visited per packet; the ratio between "hit" and total number of > -packets processed by the datapath". > +packets processed by the datapath. > .IP > If one or more datapaths are specified, information on only those > datapaths are displayed. Otherwise, \fB\*(PN\fR displays information > @@ -99,7 +99,7 @@ default. When multiple datapaths exist, then a datapath > name is > required. > . > .TP > -.DO "[\fB\-m \fR| \fB\-\-more\fR]" \*(DX\fBdump\-flows\fR "[\fIdp\fR] > [\fBfilter=\fIfilter\fR] [\fBtype=\fItype\fR]" > +.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" > \*(DX\fBdump\-flows\fR "[\fIdp\fR] [\fBfilter=\fIfilter\fR] > [\fBtype=\fItype\fR]" > Prints to the console all flow entries in datapath \fIdp\fR's flow > table. Without \fB\-m\fR or \fB\-\-more\fR, output omits match fields > that a flow wildcards entirely; with \fB\-m\fR or \fB\-\-more\fR, > @@ -184,7 +184,8 @@ Deletes the flow from \fIdp\fR's flow table that matches > \fIflow\fR. > If \fB\-s\fR or \fB\-\-statistics\fR is specified, then > \fBdel\-flow\fR prints the deleted flow's statistics. > . > -.IP "\*(DX\fBget\-flow\fR [\fIdp\fR] ufid:\fIufid\fR" > +.TP > +.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" > "\*(DX\fBget\-flow\fR [\fIdp\fR] ufid:\fIufid\fR" > Fetches the flow from \fIdp\fR's flow table with unique identifier > \fIufid\fR. > \fIufid\fR must be specified as a string of 32 hexadecimal characters. > . > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 8e4df797a360..d15095558243 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2944,7 +2944,7 @@ format_odp_key_attr(const struct nlattr *a, const > struct nlattr *ma, > break; > > case OVS_KEY_ATTR_IN_PORT: > - if (portno_names && verbose && is_exact) { > + if (portno_names && is_exact) { > char *name = odp_portno_names_get(portno_names, > nl_attr_get_odp_port(a)); > if (name) { > @@ -3251,7 +3251,7 @@ odp_format_ufid(const ovs_u128 *ufid, struct ds *ds) > /* Appends to 'ds' a string representation of the 'key_len' bytes of > * OVS_KEY_ATTR_* attributes in 'key'. If non-null, additionally formats the > * 'mask_len' bytes of 'mask' which apply to 'key'. If 'portno_names' is > - * non-null and 'verbose' is true, translates odp port number to its name. */ > + * non-null, translates odp port number to its name. */ > void > odp_flow_format(const struct nlattr *key, size_t key_len, > const struct nlattr *mask, size_t mask_len, > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 23ba1a303bd8..28b24b9a0fd7 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5240,11 +5240,6 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn > *conn, > const struct ofproto_dpif *ofproto; > > struct ds ds = DS_EMPTY_INITIALIZER; > - bool verbosity = false; > - > - struct dpif_port dpif_port; > - struct dpif_port_dump port_dump; > - struct hmap portno_names; > > struct dpif_flow_dump *flow_dump; > struct dpif_flow_dump_thread *flow_dump_thread; > @@ -5257,13 +5252,35 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn > *conn, > return; > } > > - if (argc > 2 && !strcmp(argv[1], "-m")) { > - verbosity = true; > + bool verbosity = false; > + bool names = false; > + bool set_names = false; > + for (int i = 1; i < argc - 1; i++) { > + if (!strcmp(argv[i], "-m")) { > + verbosity = true; > + } else if (!strcmp(argv[i], "--names")) { > + names = true; > + set_names = true; > + } else if (!strcmp(argv[i], "--no-names")) { > + names = false; > + set_names = true; > + } > + } > + if (!set_names) { > + names = verbosity; > } > > - hmap_init(&portno_names); > - DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, ofproto->backer->dpif) { > - odp_portno_names_set(&portno_names, dpif_port.port_no, > dpif_port.name); > + struct hmap *portno_names = NULL; > + if (names) { > + portno_names = xmalloc(sizeof *portno_names); > + hmap_init(portno_names); > + > + struct dpif_port dpif_port; > + struct dpif_port_dump port_dump; > + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, ofproto->backer->dpif) { > + odp_portno_names_set(portno_names, dpif_port.port_no, > + dpif_port.name); > + } > } > > ds_init(&ds); > @@ -5282,7 +5299,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn > *conn, > ds_put_cstr(&ds, " "); > } > odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, > - &portno_names, &ds, verbosity); > + portno_names, &ds, verbosity); > ds_put_cstr(&ds, ", "); > dpif_flow_stats_format(&f.stats, &ds); > ds_put_cstr(&ds, ", actions:"); > @@ -5299,8 +5316,11 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn > *conn, > } else { > unixctl_command_reply(conn, ds_cstr(&ds)); > } > - odp_portno_names_destroy(&portno_names); > - hmap_destroy(&portno_names); > + if (portno_names) { > + odp_portno_names_destroy(portno_names); > + hmap_destroy(portno_names); > + free(portno_names); > + } > ds_destroy(&ds); > } > > @@ -5415,7 +5435,7 @@ ofproto_unixctl_init(void) > NULL); > unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1, > ofproto_unixctl_dpif_show_dp_features, NULL); > - unixctl_command_register("dpif/dump-flows", "[-m] bridge", 1, 2, > + unixctl_command_register("dpif/dump-flows", "[-m] [--names | --no-nmaes] > bridge", 1, INT_MAX, > ofproto_unixctl_dpif_dump_flows, NULL); > > unixctl_command_register("ofproto/tnl-push-pop", "[on]|[off]", 1, 1, > diff --git a/utilities/ovs-dpctl.8.in b/utilities/ovs-dpctl.8.in > index c1be05e136be..f054bd2d78e0 100644 > --- a/utilities/ovs-dpctl.8.in > +++ b/utilities/ovs-dpctl.8.in > @@ -58,7 +58,14 @@ each port within the datapaths that it shows. > . > .IP "\fB\-m\fR" > .IQ "\fB\-\-more\fR" > -Increases the verbosity of \fBdump\-flows\fR output. > +Increases verbosity of output for \fBdump\-flows\fR and > +\fBget\-flow\fR. > +. > +.IP "\fB\-\-names\fR" > +.IQ "\fB\-\-no-names\fR" > +Enables or disables showing port names in place of numbers in output > +for \fBdump\-flows\fR and \fBget\-flow\fR. By default, names are > +shown if at least one \fB\-m\fR or \fB\-\-more\fR is specified. > . > .IP "\fB\-t\fR" > .IQ "\fB\-\-timeout=\fIsecs\fR" > diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c > index 843d305ccf21..aae8c93c9eef 100644 > --- a/utilities/ovs-dpctl.c > +++ b/utilities/ovs-dpctl.c > @@ -78,6 +78,8 @@ parse_options(int argc, char *argv[]) > OPT_CLEAR = UCHAR_MAX + 1, > OPT_MAY_CREATE, > OPT_READ_ONLY, > + OPT_NAMES, > + OPT_NO_NAMES, > VLOG_OPTION_ENUMS > }; > static const struct option long_options[] = { > @@ -86,6 +88,8 @@ parse_options(int argc, char *argv[]) > {"may-create", no_argument, NULL, OPT_MAY_CREATE}, > {"read-only", no_argument, NULL, OPT_READ_ONLY}, > {"more", no_argument, NULL, 'm'}, > + {"names", no_argument, NULL, OPT_NAMES}, > + {"no-names", no_argument, NULL, OPT_NO_NAMES}, > {"timeout", required_argument, NULL, 't'}, > {"help", no_argument, NULL, 'h'}, > {"option", no_argument, NULL, 'o'}, > @@ -95,6 +99,7 @@ parse_options(int argc, char *argv[]) > }; > char *short_options = > ovs_cmdl_long_options_to_short_options(long_options); > > + bool set_names = false; > for (;;) { > unsigned long int timeout; > int c; > @@ -125,6 +130,16 @@ parse_options(int argc, char *argv[]) > dpctl_p.verbosity++; > break; > > + case OPT_NAMES: > + dpctl_p.names = true; > + set_names = true; > + break; > + > + case OPT_NO_NAMES: > + dpctl_p.names = false; > + set_names = true; > + break; > + > case 't': > timeout = strtoul(optarg, NULL, 10); > if (timeout <= 0) { > @@ -156,6 +171,10 @@ parse_options(int argc, char *argv[]) > } > } > free(short_options); > + > + if (!set_names) { > + dpctl_p.names = dpctl_p.verbosity > 0; > + } > } > > static void > @@ -190,6 +209,7 @@ usage(void *userdata OVS_UNUSED) > " -s, --statistics print statistics for port or > flow\n" > "\nOptions for dump-flows:\n" > " -m, --more increase verbosity of output\n" > + " --names use port names in output\n" > "\nOptions for mod-flow:\n" > " --may-create create flow if it doesn't exist\n" > " --read-only do not run read/write commands\n" > -- > 2.10.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
