ovs-dpctl, ofproto/trace: Show and handle the in_port name in flows.

With this commit, whenever the verbosity is enabled with '-m'
option, the ovs-dpctl dump-flows command will display the flows with
in_port field showing the name instead of a port number.

Conversely, one can also use a name in the in_port field with del-flow,
add-flow and mod-flow commands of ovs-dpctl. One should also be able
to use the port name when supplying the datapath flow as an input
to ofproto/trace command.

Signed-off-by: Gurucharan Shetty <gshetty@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

7 files changed, 174 insertions, 64 deletions

diff --git a/lib/dpif.c b/lib/dpif.c
index add2a0bdf..b66e3bc03 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1349,7 +1349,7 @@ log_flow_message(const struct dpif *dpif, int error, const char *operation,
     if (error) {
         ds_put_format(&ds, "(%s) ", ovs_strerror(error));
     }
-    odp_flow_format(key, key_len, mask, mask_len, &ds, true);
+    odp_flow_format(key, key_len, mask, mask_len, NULL, &ds, true);
     if (stats) {
         ds_put_cstr(&ds, ", ");
         dpif_flow_stats_format(stats, &ds);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0785c6a46..5c7ccfb63 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -51,7 +51,8 @@ static const char *delimiters = ", \t\r\n";
 static int parse_odp_key_mask_attr(const char *, const struct simap *port_names,
                               struct ofpbuf *, struct ofpbuf *);
 static void format_odp_key_attr(const struct nlattr *a,
-                                const struct nlattr *ma, struct ds *ds,
+                                const struct nlattr *ma,
+                                const struct hmap *portno_names, struct ds *ds,
                                 bool verbose);
 
 /* Returns one the following for the action with the given OVS_ACTION_ATTR_*
@@ -401,7 +402,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
         break;
     case OVS_ACTION_ATTR_SET:
         ds_put_cstr(ds, "set(");
-        format_odp_key_attr(nl_attr_get(a), NULL, ds, true);
+        format_odp_key_attr(nl_attr_get(a), NULL, NULL, ds, true);
         ds_put_cstr(ds, ")");
         break;
     case OVS_ACTION_ATTR_PUSH_VLAN:
@@ -935,10 +936,49 @@ odp_mask_attr_is_exact(const struct nlattr *ma)
     return is_exact;
 }
 
+void
+odp_portno_names_set(struct hmap *portno_names, odp_port_t port_no,
+                     char *port_name)
+{
+    struct odp_portno_names *odp_portno_names;
+
+    odp_portno_names = xmalloc(sizeof *odp_portno_names);
+    odp_portno_names->port_no = port_no;
+    odp_portno_names->name = xstrdup(port_name);
+    hmap_insert(portno_names, &odp_portno_names->hmap_node,
+                hash_odp_port(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;
+
+    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;
+}
+
+void
+odp_portno_names_destroy(struct hmap *portno_names)
+{
+    struct odp_portno_names *odp_portno_names, *odp_portno_names_next;
+    HMAP_FOR_EACH_SAFE (odp_portno_names, odp_portno_names_next,
+                        hmap_node, portno_names) {
+        hmap_remove(portno_names, &odp_portno_names->hmap_node);
+        free(odp_portno_names->name);
+        free(odp_portno_names);
+    }
+}
 
 static void
 format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
-                    struct ds *ds, bool verbose)
+                    const struct hmap *portno_names, struct ds *ds,
+                    bool verbose)
 {
     struct flow_tnl tun_key;
     enum ovs_key_attr attr = nl_attr_type(a);
@@ -981,10 +1021,11 @@ format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
     case OVS_KEY_ATTR_ENCAP:
         if (ma && nl_attr_get_size(ma) && nl_attr_get_size(a)) {
             odp_flow_format(nl_attr_get(a), nl_attr_get_size(a),
-                            nl_attr_get(ma), nl_attr_get_size(ma), ds, verbose);
-        } else if (nl_attr_get_size(a)) {
-            odp_flow_format(nl_attr_get(a), nl_attr_get_size(a), NULL, 0, ds,
+                            nl_attr_get(ma), nl_attr_get_size(ma), NULL, ds,
                             verbose);
+        } else if (nl_attr_get_size(a)) {
+            odp_flow_format(nl_attr_get(a), nl_attr_get_size(a), NULL, 0, NULL,
+                            ds, verbose);
         }
         break;
 
@@ -1038,9 +1079,19 @@ format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
         break;
 
     case OVS_KEY_ATTR_IN_PORT:
-        ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
-        if (!is_exact) {
-            ds_put_format(ds, "/%#"PRIx32, nl_attr_get_u32(ma));
+        if (portno_names && verbose && is_exact) {
+            char *name = odp_portno_names_get(portno_names,
+                            u32_to_odp(nl_attr_get_u32(a)));
+            if (name) {
+                ds_put_format(ds, "%s", name);
+            } else {
+                ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
+            }
+        } else {
+            ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a));
+            if (!is_exact) {
+                ds_put_format(ds, "/%#"PRIx32, nl_attr_get_u32(ma));
+            }
         }
         break;
 
@@ -1364,7 +1415,7 @@ generate_all_wildcard_mask(struct ofpbuf *ofp, const struct nlattr *key)
 void
 odp_flow_format(const struct nlattr *key, size_t key_len,
                 const struct nlattr *mask, size_t mask_len,
-                struct ds *ds, bool verbose)
+                const struct hmap *portno_names, struct ds *ds, bool verbose)
 {
     if (key_len) {
         const struct nlattr *a;
@@ -1398,7 +1449,7 @@ odp_flow_format(const struct nlattr *key, size_t key_len,
                 if (!first_field) {
                     ds_put_char(ds, ',');
                 }
-                format_odp_key_attr(a, ma, ds, verbose);
+                format_odp_key_attr(a, ma, portno_names, ds, verbose);
                 first_field = false;
             }
             ofpbuf_clear(&ofp);
@@ -1435,7 +1486,7 @@ void
 odp_flow_key_format(const struct nlattr *key,
                     size_t key_len, struct ds *ds)
 {
-    odp_flow_format(key, key_len, NULL, 0, ds, true);
+    odp_flow_format(key, key_len, NULL, 0, NULL, ds, true);
 }
 
 static void
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 4abf54344..2712cb007 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <linux/openvswitch.h>
 #include "hash.h"
+#include "hmap.h"
 #include "openflow/openflow.h"
 #include "util.h"
 
@@ -42,6 +43,16 @@ void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
 int odp_actions_from_string(const char *, const struct simap *port_names,
                             struct ofpbuf *odp_actions);
 
+/* A map from odp port number to its name. */
+struct odp_portno_names {
+    struct hmap_node hmap_node; /* A node in a port number to name hmap. */
+    odp_port_t port_no;         /* Port number in the datapath. */
+    char *name;                 /* Name associated with the above 'port_no'. */
+};
+
+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);
 /* 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.
@@ -94,7 +105,8 @@ enum odp_key_fitness odp_tun_key_from_attr(const struct nlattr *,
 
 void odp_flow_format(const struct nlattr *key, size_t key_len,
                      const struct nlattr *mask, size_t mask_len,
-                     struct ds *, bool verbose);
+                     const struct hmap *portno_names, struct ds *,
+                     bool verbose);
 void odp_flow_key_format(const struct nlattr *, size_t, struct ds *);
 int odp_flow_from_string(const char *s,
                          const struct simap *port_names,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 037ee4b8e..80e97e04c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5220,12 +5220,13 @@ static void
 ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
                       void *aux OVS_UNUSED)
 {
-    const struct dpif_backer *backer;
+    const struct dpif_backer *backer = NULL;
     struct ofproto_dpif *ofproto;
     struct ofpbuf odp_key, odp_mask;
     struct ofpbuf *packet;
     struct ds result;
     struct flow flow;
+    struct simap port_names;
     char *s;
 
     packet = NULL;
@@ -5233,6 +5234,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
     ds_init(&result);
     ofpbuf_init(&odp_key, 0);
     ofpbuf_init(&odp_mask, 0);
+    simap_init(&port_names);
 
     /* Handle "-generate" or a hex string as the last argument. */
     if (!strcmp(argv[argc - 1], "-generate")) {
@@ -5249,37 +5251,42 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
         }
     }
 
+    /* odp_flow can have its in_port specified as a name instead of port no.
+     * We do not yet know whether a given flow is a odp_flow or a br_flow.
+     * But, to know whether a flow is odp_flow through odp_flow_from_string(),
+     * we need to create a simap of name to port no. */
+    if (argc == 3) {
+        const char *dp_type;
+        if (!strncmp(argv[1], "ovs-", 4)) {
+            dp_type = argv[1] + 4;
+        } else {
+            dp_type = argv[1];
+        }
+        backer = shash_find_data(&all_dpif_backers, dp_type);
+    } else {
+        struct shash_node *node;
+        if (shash_count(&all_dpif_backers) == 1) {
+            node = shash_first(&all_dpif_backers);
+            backer = node->data;
+        }
+    }
+    if (backer && backer->dpif) {
+        struct dpif_port dpif_port;
+        struct dpif_port_dump port_dump;
+        DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, backer->dpif) {
+            simap_put(&port_names, dpif_port.name,
+                      odp_to_u32(dpif_port.port_no));
+        }
+    }
+
     /* Parse the flow and determine whether a datapath or
      * bridge is specified. If function odp_flow_key_from_string()
      * returns 0, the flow is a odp_flow. If function
      * parse_ofp_exact_flow() returns 0, the flow is a br_flow. */
-    if (!odp_flow_from_string(argv[argc - 1], NULL, &odp_key, &odp_mask)) {
-        /* If the odp_flow is the second argument,
-         * the datapath name is the first argument. */
-        if (argc == 3) {
-            const char *dp_type;
-            if (!strncmp(argv[1], "ovs-", 4)) {
-                dp_type = argv[1] + 4;
-            } else {
-                dp_type = argv[1];
-            }
-            backer = shash_find_data(&all_dpif_backers, dp_type);
-            if (!backer) {
-                unixctl_command_reply_error(conn, "Cannot find datapath "
-                               "of this name");
-                goto exit;
-            }
-        } else {
-            /* No datapath name specified, so there should be only one
-             * datapath. */
-            struct shash_node *node;
-            if (shash_count(&all_dpif_backers) != 1) {
-                unixctl_command_reply_error(conn, "Must specify datapath "
-                         "name, there is more than one type of datapath");
-                goto exit;
-            }
-            node = shash_first(&all_dpif_backers);
-            backer = node->data;
+    if (!odp_flow_from_string(argv[argc - 1], &port_names, &odp_key, &odp_mask)) {
+        if (!backer) {
+            unixctl_command_reply_error(conn, "Cannot find the datapath");
+            goto exit;
         }
 
         if (xlate_receive(backer, NULL, odp_key.data, odp_key.size, &flow,
@@ -5332,6 +5339,7 @@ exit:
     ofpbuf_delete(packet);
     ofpbuf_uninit(&odp_key);
     ofpbuf_uninit(&odp_mask);
+    simap_destroy(&port_names);
 }
 
 static void
@@ -5746,7 +5754,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
         }
 
         odp_flow_format(subfacet->key, subfacet->key_len,
-                        mask.data, mask.size, &ds, false);
+                        mask.data, mask.size, NULL, &ds, false);
 
         ds_put_format(&ds, ", packets:%"PRIu64", bytes:%"PRIu64", used:",
                       subfacet->dp_packet_count, subfacet->dp_byte_count);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 205044364..ea3602016 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1161,8 +1161,15 @@ in_port=2 actions=output:1
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
-odp_flow="in_port(1)"
+odp_flow="in_port(p1)"
 br_flow="in_port=1"
+# Test command: ofproto/trace odp_flow with in_port as a name.
+AT_CHECK([ovs-appctl ofproto/trace "$odp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+odp_flow="in_port(1)"
 # Test command: ofproto/trace odp_flow
 AT_CHECK([ovs-appctl ofproto/trace "$odp_flow"], [0], [stdout])
 AT_CHECK([tail -1 stdout], [0], [dnl
@@ -1307,7 +1314,7 @@ m4_foreach(
 [AT_CHECK([ovs-appctl ofproto/trace wrong_name "$odp_flow" option],
   [2], [], [stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
-Cannot find datapath of this name
+Cannot find the datapath
 ovs-appctl: ovs-vswitchd: server returned an error
 ])])
 
@@ -1320,7 +1327,7 @@ m4_foreach(
 [AT_CHECK([ovs-appctl ofproto/trace "" "$odp_flow" option],
   [2], [], [stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
-Cannot find datapath of this name
+Cannot find the datapath
 ovs-appctl: ovs-vswitchd: server returned an error
 ])])
 
@@ -1333,7 +1340,7 @@ m4_foreach(
 [AT_CHECK([ovs-appctl ofproto/trace ovs-system "$odp_flow" option],
   [2], [], [stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
-Cannot find datapath of this name
+Cannot find the datapath
 ovs-appctl: ovs-vswitchd: server returned an error
 ])])
 
@@ -1346,7 +1353,7 @@ m4_foreach(
 [AT_CHECK([ovs-appctl ofproto/trace br0 "$odp_flow" option],
   [2], [], [stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
-Cannot find datapath of this name
+Cannot find the datapath
 ovs-appctl: ovs-vswitchd: server returned an error
 ])])
 
diff --git a/tests/test-odp.c b/tests/test-odp.c
index 45605e4b9..183a3b3cd 100644
--- a/tests/test-odp.c
+++ b/tests/test-odp.c
@@ -86,7 +86,7 @@ parse_keys(bool wc_keys)
         ds_init(&out);
         if (wc_keys) {
             odp_flow_format(odp_key.data, odp_key.size,
-                            odp_mask.data, odp_mask.size, &out, false);
+                            odp_mask.data, odp_mask.size, NULL, &out, false);
         } else {
             odp_flow_key_format(odp_key.data, odp_key.size, &out);
         }
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index af4fc1f51..4fb02dda4 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -740,9 +740,12 @@ dpctl_dump_flows(int argc, char *argv[])
 {
     const struct dpif_flow_stats *stats;
     const struct nlattr *actions;
-    struct dpif_flow_dump dump;
+    struct dpif_flow_dump flow_dump;
     const struct nlattr *key;
     const struct nlattr *mask;
+    struct dpif_port dpif_port;
+    struct dpif_port_dump port_dump;
+    struct hmap portno_names;
     size_t actions_len;
     struct dpif *dpif;
     size_t key_len;
@@ -754,13 +757,19 @@ dpctl_dump_flows(int argc, char *argv[])
     run(parsed_dpif_open(name, false, &dpif), "opening datapath");
     free(name);
 
+    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);
+    }
+
     ds_init(&ds);
-    dpif_flow_dump_start(&dump, dpif);
-    while (dpif_flow_dump_next(&dump, &key, &key_len,
+    dpif_flow_dump_start(&flow_dump, dpif);
+    while (dpif_flow_dump_next(&flow_dump, &key, &key_len,
                                &mask, &mask_len,
                                &actions, &actions_len, &stats)) {
         ds_clear(&ds);
-        odp_flow_format(key, key_len, mask, mask_len, &ds, verbosity);
+        odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds,
+                        verbosity);
         ds_put_cstr(&ds, ", ");
 
         dpif_flow_stats_format(stats, &ds);
@@ -768,7 +777,9 @@ dpctl_dump_flows(int argc, char *argv[])
         format_odp_actions(&ds, actions, actions_len);
         printf("%s\n", ds_cstr(&ds));
     }
-    dpif_flow_dump_done(&dump);
+    dpif_flow_dump_done(&flow_dump);
+    odp_portno_names_destroy(&portno_names);
+    hmap_destroy(&portno_names);
     ds_destroy(&ds);
     dpif_close(dpif);
 }
@@ -779,25 +790,37 @@ dpctl_put_flow(int argc, char *argv[], enum dpif_flow_put_flags flags)
     const char *key_s = argv[argc - 2];
     const char *actions_s = argv[argc - 1];
     struct dpif_flow_stats stats;
+    struct dpif_port dpif_port;
+    struct dpif_port_dump port_dump;
     struct ofpbuf actions;
     struct ofpbuf key;
     struct ofpbuf mask;
     struct dpif *dpif;
     struct ds s;
     char *dp_name;
+    struct simap port_names;
+
+    dp_name = argc == 4 ? xstrdup(argv[1]) : get_one_dp();
+    run(parsed_dpif_open(dp_name, false, &dpif), "opening datapath");
+    free(dp_name);
+
+
+    simap_init(&port_names);
+    DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
+        simap_put(&port_names, dpif_port.name, odp_to_u32(dpif_port.port_no));
+    }
 
     ds_init(&s);
     ofpbuf_init(&key, 0);
     ofpbuf_init(&mask, 0);
-    run(odp_flow_from_string(key_s, NULL, &key, &mask), "parsing flow key");
+    run(odp_flow_from_string(key_s, &port_names, &key, &mask),
+        "parsing flow key");
+
+    simap_destroy(&port_names);
 
     ofpbuf_init(&actions, 0);
     run(odp_actions_from_string(actions_s, NULL, &actions), "parsing actions");
 
-    dp_name = argc == 4 ? xstrdup(argv[1]) : get_one_dp();
-    run(parsed_dpif_open(dp_name, false, &dpif), "opening datapath");
-    free(dp_name);
-
     run(dpif_flow_put(dpif, flags,
                       key.data, key.size,
                       mask.size == 0 ? NULL : mask.data, mask.size,
@@ -846,23 +869,32 @@ dpctl_del_flow(int argc, char *argv[])
 {
     const char *key_s = argv[argc - 1];
     struct dpif_flow_stats stats;
+    struct dpif_port dpif_port;
+    struct dpif_port_dump port_dump;
     struct ofpbuf key;
     struct ofpbuf mask; /* To be ignored. */
     struct dpif *dpif;
     char *dp_name;
-
-    ofpbuf_init(&key, 0);
-    ofpbuf_init(&mask, 0);
-    run(odp_flow_from_string(key_s, NULL, &key, &mask), "parsing flow key");
+    struct simap port_names;
 
     dp_name = argc == 3 ? xstrdup(argv[1]) : get_one_dp();
     run(parsed_dpif_open(dp_name, false, &dpif), "opening datapath");
     free(dp_name);
 
+    simap_init(&port_names);
+    DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
+        simap_put(&port_names, dpif_port.name, odp_to_u32(dpif_port.port_no));
+    }
+
+    ofpbuf_init(&key, 0);
+    ofpbuf_init(&mask, 0);
+    run(odp_flow_from_string(key_s, &port_names, &key, &mask), "parsing flow key");
+
     run(dpif_flow_del(dpif,
                       key.data, key.size,
                       print_statistics ? &stats : NULL), "deleting flow");
 
+    simap_destroy(&port_names);
     ofpbuf_uninit(&key);
     ofpbuf_uninit(&mask);
 
@@ -1049,7 +1081,7 @@ dpctl_normalize_actions(int argc, char *argv[])
         "odp_flow_key_from_string");
 
     ds_clear(&s);
-    odp_flow_format(keybuf.data, keybuf.size, NULL, 0, &s, verbosity);
+    odp_flow_format(keybuf.data, keybuf.size, NULL, 0, NULL, &s, verbosity);
     printf("input flow: %s\n", ds_cstr(&s));
 
     run(odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow),