[ovs-dev] [PATCH 1/2] ovs-dpctl: New --names option to use port names in flow dumps.
Jan Scheurich
jan.scheurich at ericsson.com
Fri Jun 23 08:11:42 UTC 2017
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 <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 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 <blp at ovn.org>
> ---
> 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list