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@ovn.org>
Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>
Tested-by: Jan Scheurich <jan.scheurich@ericsson.com>

7 files changed, 125 insertions, 42 deletions

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 2ad475b75..5a9d847c1 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 *);
@@ -764,6 +766,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)
 {
@@ -776,10 +808,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;
@@ -809,15 +837,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)),
@@ -892,7 +919,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));
     }
@@ -905,8 +932,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);
@@ -1034,11 +1060,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];
@@ -1057,10 +1080,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) {
@@ -1076,13 +1097,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;
@@ -1682,6 +1702,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)) {
@@ -1694,6 +1715,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;
@@ -1728,6 +1755,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 4ee083fa7..9d0052152 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 f6e4a7a2f..9ebd39637 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 8e4df797a..d15095558 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 23ba1a303..28b24b9a0 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 c1be05e13..f054bd2d7 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 843d305cc..aae8c93c9 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"