[ovs-dev,3/3] odp-util: Improve log messages and error reporting for Netlink parsing.

Message ID 20181215021655.29228-3-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev,1/3] Fix bugs in L3 protocol support.
Related show

Commit Message

Ben Pfaff Dec. 15, 2018, 2:16 a.m.
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/dpctl.c                   |  28 +++--
 lib/dpif-netdev.c             |   4 +-
 lib/netdev-dummy.c            |  15 ++-
 lib/odp-execute.c             |   6 +-
 lib/odp-util.c                | 255 +++++++++++++++++++++++++++++++-----------
 lib/odp-util.h                |  15 +--
 ofproto/ofproto-dpif-sflow.c  |   2 +-
 ofproto/ofproto-dpif-trace.c  |  22 ++--
 ofproto/ofproto-dpif-upcall.c |  15 ++-
 ofproto/ofproto-dpif-xlate.c  |  25 ++++-
 ofproto/ofproto-dpif-xlate.h  |   3 +-
 ofproto/ofproto-dpif.c        |   6 +-
 tests/odp.at                  |   2 +-
 tests/ofproto-dpif.at         |   2 +-
 tests/test-odp.c              |  23 ++--
 tests/tunnel.at               |   4 +-
 16 files changed, 294 insertions(+), 133 deletions(-)

Comments

0-day Robot Dec. 15, 2018, 3:01 a.m. | #1
Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Inappropriate bracing around statement
#203 FILE: lib/odp-util.c:2646:
        if (OVS_UNLIKELY(ERRORP)) {             \

WARNING: Line is 80 characters long (recommended limit is 79)
#667 FILE: lib/odp-util.c:6901:
                flow->vlans[0].tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]);

Lines checked: 1166, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 59071cdba83d..1632fad89a57 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1022,8 +1022,8 @@  dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
             struct match match, match_filter;
             struct minimatch minimatch;
 
-            odp_flow_key_to_flow(f.key, f.key_len, &flow);
-            odp_flow_key_to_mask(f.mask, f.mask_len, &wc, &flow);
+            odp_flow_key_to_flow(f.key, f.key_len, &flow, NULL);
+            odp_flow_key_to_mask(f.mask, f.mask_len, &wc, &flow, NULL);
             match_init(&match, &flow, &wc);
 
             match_init(&match_filter, &flow_filter, &wc);
@@ -1111,10 +1111,12 @@  dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
 
     ofpbuf_init(&key, 0);
     ofpbuf_init(&mask, 0);
-    error = odp_flow_from_string(key_s, &port_names, &key, &mask);
+    char *error_s;
+    error = odp_flow_from_string(key_s, &port_names, &key, &mask, &error_s);
     simap_destroy(&port_names);
     if (error) {
-        dpctl_error(dpctl_p, error, "parsing flow key");
+        dpctl_error(dpctl_p, error, "parsing flow key (%s)", error_s);
+        free(error_s);
         goto out_freekeymask;
     }
 
@@ -1263,9 +1265,11 @@  dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     ofpbuf_init(&key, 0);
     ofpbuf_init(&mask, 0);
 
-    error = odp_flow_from_string(key_s, &port_names, &key, &mask);
+    char *error_s;
+    error = odp_flow_from_string(key_s, &port_names, &key, &mask, &error_s);
     if (error) {
-        dpctl_error(dpctl_p, error, "parsing flow key");
+        dpctl_error(dpctl_p, error, "%s", error_s);
+        free(error_s);
         goto out;
     }
 
@@ -2076,9 +2080,11 @@  dpctl_normalize_actions(int argc, const char *argv[],
 
     /* Parse flow key. */
     ofpbuf_init(&keybuf, 0);
-    error = odp_flow_from_string(argv[1], &port_names, &keybuf, NULL);
+    char *error_s;
+    error = odp_flow_from_string(argv[1], &port_names, &keybuf, NULL,
+                                 &error_s);
     if (error) {
-        dpctl_error(dpctl_p, error, "odp_flow_key_from_string");
+        dpctl_error(dpctl_p, error, "odp_flow_key_from_string (%s)", error_s);
         goto out_freekeybuf;
     }
 
@@ -2087,9 +2093,11 @@  dpctl_normalize_actions(int argc, const char *argv[],
                     &s, dpctl_p->verbosity);
     dpctl_print(dpctl_p, "input flow: %s\n", ds_cstr(&s));
 
-    error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow);
+    error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow, &error_s);
     if (error) {
-        dpctl_error(dpctl_p, error, "odp_flow_key_to_flow");
+        dpctl_error(dpctl_p, error, "odp_flow_key_to_flow failed (%s)",
+                    error_s ? error_s : "reason unknown");
+        free(error_s);
         goto out_freekeybuf;
     }
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1564db9c6e44..d94ac7951fe0 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3048,7 +3048,7 @@  dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
 {
     enum odp_key_fitness fitness;
 
-    fitness = odp_flow_key_to_mask(mask_key, mask_key_len, wc, flow);
+    fitness = odp_flow_key_to_mask(mask_key, mask_key_len, wc, flow, NULL);
     if (fitness) {
         if (!probe) {
             /* This should not happen: it indicates that
@@ -3079,7 +3079,7 @@  static int
 dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
                               struct flow *flow, bool probe)
 {
-    if (odp_flow_key_to_flow(key, key_len, flow)) {
+    if (odp_flow_key_to_flow(key, key_len, flow, NULL)) {
         if (!probe) {
             /* This should not happen: it indicates that
              * odp_flow_key_from_flow() and odp_flow_key_to_flow() disagree on
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 72b4f7adc62f..78aa34957a20 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1462,8 +1462,10 @@  eth_from_packet(const char *s)
 }
 
 static struct dp_packet *
-eth_from_flow(const char *s, size_t packet_size)
+eth_from_flow(const char *s, size_t packet_size, char **errorp)
 {
+    *errorp = NULL;
+
     enum odp_key_fitness fitness;
     struct dp_packet *packet;
     struct ofpbuf odp_key;
@@ -1477,14 +1479,14 @@  eth_from_flow(const char *s, size_t packet_size)
      * settle for parsing a datapath key for now.
      */
     ofpbuf_init(&odp_key, 0);
-    error = odp_flow_from_string(s, NULL, &odp_key, NULL);
+    error = odp_flow_from_string(s, NULL, &odp_key, NULL, errorp);
     if (error) {
         ofpbuf_uninit(&odp_key);
         return NULL;
     }
 
     /* Convert odp_key to flow. */
-    fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
+    fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, errorp);
     if (fitness == ODP_FIT_ERROR) {
         ofpbuf_uninit(&odp_key);
         return NULL;
@@ -1592,10 +1594,11 @@  netdev_dummy_receive(struct unixctl_conn *conn,
                 i += 2;
             }
             /* Try parse 'argv[i]' as odp flow. */
-            packet = eth_from_flow(flow_str, packet_size);
-
+            char *error_s;
+            packet = eth_from_flow(flow_str, packet_size, &error_s);
             if (!packet) {
-                unixctl_command_reply_error(conn, "bad packet or flow syntax");
+                unixctl_command_reply_error(conn, error_s);
+                free(error_s);
                 goto exit;
             }
         }
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 3b6890e952c2..ddd041e82522 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -198,7 +198,7 @@  odp_set_sctp(struct dp_packet *packet, const struct ovs_key_sctp *key,
 static void
 odp_set_tunnel_action(const struct nlattr *a, struct flow_tnl *tun_key)
 {
-    ovs_assert(odp_tun_key_from_attr(a, tun_key) != ODP_FIT_ERROR);
+    ovs_assert(odp_tun_key_from_attr(a, tun_key, NULL) != ODP_FIT_ERROR);
 }
 
 static void
@@ -280,9 +280,9 @@  odp_set_nsh(struct dp_packet *packet, const struct nlattr *a, bool has_mask)
     ovs_be32 path_hdr;
 
     if (has_mask) {
-        odp_nsh_key_from_attr(a, &key, &mask);
+        odp_nsh_key_from_attr(a, &key, &mask, NULL);
     } else {
-        odp_nsh_key_from_attr(a, &key, NULL);
+        odp_nsh_key_from_attr(a, &key, NULL, NULL);
     }
 
     if (!has_mask) {
diff --git a/lib/odp-util.c b/lib/odp-util.c
index a3d0ab9362c1..2f4e8b4b418d 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2641,10 +2641,25 @@  odp_nsh_hdr_from_attr(const struct nlattr *attr,
     return ODP_FIT_PERFECT;
 }
 
+#define ODP_PARSE_ERROR(RL, ERRORP, ...)        \
+    do {                                        \
+        if (OVS_UNLIKELY(ERRORP)) {             \
+            free(*(ERRORP));                    \
+            *(ERRORP) = xasprintf(__VA_ARGS__); \
+        } else {                                \
+            VLOG_WARN_RL(RL, __VA_ARGS__);       \
+        }                                       \
+    } while (0)
+
 enum odp_key_fitness
 odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
-                      struct ovs_key_nsh *nsh_mask)
+                      struct ovs_key_nsh *nsh_mask, char **errorp)
 {
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    if (errorp) {
+        *errorp = NULL;
+    }
+
     unsigned int left;
     const struct nlattr *a;
     bool unknown = false;
@@ -2655,17 +2670,18 @@  odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
         size_t len = nl_attr_get_size(a);
         int expected_len = odp_key_attr_len(ovs_nsh_key_attr_lens,
                                             OVS_NSH_KEY_ATTR_MAX, type);
-
-        /* the attribute can have mask, len is 2 * expected_len for that case.
-         */
-        if ((len != expected_len) && (len != 2 * expected_len) &&
-            (expected_len >= 0)) {
-            return ODP_FIT_ERROR;
-        }
-
-        if ((nsh_mask && (expected_len >= 0) && (len != 2 * expected_len)) ||
-            (!nsh_mask && (expected_len >= 0) && (len == 2 * expected_len))) {
-            return ODP_FIT_ERROR;
+        if (expected_len) {
+            if (nsh_mask) {
+                expected_len *= 2;
+            }
+            if (len != expected_len) {
+                ODP_PARSE_ERROR(&rl, errorp, "NSH %s attribute %"PRIu16" "
+                                "should have length %d but actually has "
+                                "%"PRIuSIZE,
+                                nsh_mask ? "mask" : "key",
+                                type, expected_len, len);
+                return ODP_FIT_ERROR;
+            }
         }
 
         switch (type) {
@@ -2713,6 +2729,9 @@  odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
     }
 
     if (has_md1 && nsh->mdtype != NSH_M_TYPE1 && !nsh_mask) {
+        ODP_PARSE_ERROR(&rl, errorp, "OVS_NSH_KEY_ATTR_MD1 present but "
+                        "declared mdtype %"PRIu8" is not %d (NSH_M_TYPE1)",
+                        nsh->mdtype, NSH_M_TYPE1);
         return ODP_FIT_ERROR;
     }
 
@@ -2721,8 +2740,9 @@  odp_nsh_key_from_attr(const struct nlattr *attr, struct ovs_key_nsh *nsh,
 
 static enum odp_key_fitness
 odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
-                        struct flow_tnl *tun)
+                        struct flow_tnl *tun, char **errorp)
 {
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     unsigned int left;
     const struct nlattr *a;
     bool ttl = false;
@@ -2735,6 +2755,9 @@  odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
                                             OVS_TUNNEL_ATTR_MAX, type);
 
         if (len != expected_len && expected_len >= 0) {
+            ODP_PARSE_ERROR(&rl, errorp, "tunnel key attribute %"PRIu16" "
+                            "should have length %d but actually has %"PRIuSIZE,
+                            type, expected_len, len);
             return ODP_FIT_ERROR;
         }
 
@@ -2784,6 +2807,7 @@  odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
             struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)];
 
             if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) {
+                ODP_PARSE_ERROR(&rl, errorp, "error parsing VXLAN options");
                 return ODP_FIT_ERROR;
             }
 
@@ -2823,6 +2847,7 @@  odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
     }
 
     if (!ttl) {
+        ODP_PARSE_ERROR(&rl, errorp, "tunnel options missing TTL");
         return ODP_FIT_ERROR;
     }
     if (unknown) {
@@ -2832,10 +2857,14 @@  odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
 }
 
 enum odp_key_fitness
-odp_tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun)
+odp_tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun,
+                      char **errorp)
 {
+    if (errorp) {
+        *errorp = NULL;
+    }
     memset(tun, 0, sizeof *tun);
-    return odp_tun_key_from_attr__(attr, false, tun);
+    return odp_tun_key_from_attr__(attr, false, tun, errorp);
 }
 
 static void
@@ -5631,8 +5660,11 @@  parse_odp_key_mask_attr(struct parse_odp_context *context, const char *s,
  * have duplicated keys.  odp_flow_key_to_flow() will detect those errors. */
 int
 odp_flow_from_string(const char *s, const struct simap *port_names,
-                     struct ofpbuf *key, struct ofpbuf *mask)
+                     struct ofpbuf *key, struct ofpbuf *mask,
+                     char **errorp)
 {
+    *errorp = NULL;
+
     const size_t old_size = key->size;
     struct parse_odp_context context = (struct parse_odp_context) {
         .port_names = port_names,
@@ -5649,6 +5681,7 @@  odp_flow_from_string(const char *s, const struct simap *port_names,
         ovs_u128 ufid;
         retval = odp_ufid_from_string(s, &ufid);
         if (retval < 0) {
+            *errorp = xasprintf("syntax error at %s", s);
             key->size = old_size;
             return -retval;
         } else if (retval > 0) {
@@ -5658,6 +5691,7 @@  odp_flow_from_string(const char *s, const struct simap *port_names,
 
         retval = parse_odp_key_mask_attr(&context, s, key, mask);
         if (retval < 0) {
+            *errorp = xasprintf("syntax error at %s", s);
             key->size = old_size;
             return -retval;
         }
@@ -6091,7 +6125,7 @@  odp_key_to_dp_packet(const struct nlattr *key, size_t key_len,
         case OVS_KEY_ATTR_TUNNEL: {
             enum odp_key_fitness res;
 
-            res = odp_tun_key_from_attr(nla, &md->tunnel);
+            res = odp_tun_key_from_attr(nla, &md->tunnel, NULL);
             if (res == ODP_FIT_ERROR) {
                 memset(&md->tunnel, 0, sizeof md->tunnel);
             }
@@ -6200,7 +6234,7 @@  odp_to_ovs_frag(uint8_t odp_frag, bool is_mask)
 static bool
 parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
                    const struct nlattr *attrs[], uint64_t *present_attrsp,
-                   int *out_of_range_attrp)
+                   int *out_of_range_attrp, char **errorp)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 10);
     const struct nlattr *nla;
@@ -6219,10 +6253,11 @@  parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
         if (len != expected_len && expected_len >= 0) {
             char namebuf[OVS_KEY_ATTR_BUFSIZE];
 
-            VLOG_ERR_RL(&rl, "attribute %s has length %"PRIuSIZE" but should have "
-                        "length %d", ovs_key_attr_to_string(type, namebuf,
-                                                            sizeof namebuf),
-                        len, expected_len);
+            ODP_PARSE_ERROR(&rl, errorp, "attribute %s has length %"PRIuSIZE" "
+                            "but should have length %d",
+                            ovs_key_attr_to_string(type, namebuf,
+                                                   sizeof namebuf),
+                            len, expected_len);
             return false;
         }
 
@@ -6232,9 +6267,10 @@  parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
             if (present_attrs & (UINT64_C(1) << type)) {
                 char namebuf[OVS_KEY_ATTR_BUFSIZE];
 
-                VLOG_ERR_RL(&rl, "duplicate %s attribute in flow key",
-                            ovs_key_attr_to_string(type,
-                                                   namebuf, sizeof namebuf));
+                ODP_PARSE_ERROR(&rl, errorp,
+                                "duplicate %s attribute in flow key",
+                                ovs_key_attr_to_string(type, namebuf,
+                                                       sizeof namebuf));
                 return false;
             }
 
@@ -6243,7 +6279,7 @@  parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
         }
     }
     if (left) {
-        VLOG_ERR_RL(&rl, "trailing garbage in flow key");
+        ODP_PARSE_ERROR(&rl, errorp, "trailing garbage in flow key");
         return false;
     }
 
@@ -6281,7 +6317,8 @@  check_expectations(uint64_t present_attrs, int out_of_range_attr,
 static bool
 parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                 uint64_t present_attrs, uint64_t *expected_attrs,
-                struct flow *flow, const struct flow *src_flow)
+                struct flow *flow, const struct flow *src_flow,
+                char **errorp)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     bool is_mask = flow != src_flow;
@@ -6289,12 +6326,16 @@  parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
     if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
         flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]);
         if (!is_mask && ntohs(flow->dl_type) < ETH_TYPE_MIN) {
-            VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key",
-                        ntohs(flow->dl_type));
+            ODP_PARSE_ERROR(&rl, errorp,
+                            "invalid Ethertype %"PRIu16" in flow key",
+                            ntohs(flow->dl_type));
             return false;
         }
         if (is_mask && ntohs(src_flow->dl_type) < ETH_TYPE_MIN &&
             flow->dl_type != htons(0xffff)) {
+            ODP_PARSE_ERROR(&rl, errorp, "can't bitwise match non-Ethernet II "
+                            "\"Ethertype\" %#"PRIx16" (with mask %#"PRIx16")",
+                            ntohs(src_flow->dl_type), ntohs(flow->dl_type));
             return false;
         }
         *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
@@ -6315,7 +6356,8 @@  parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             flow->dl_type = htons(0xffff);
         } else if (ntohs(src_flow->dl_type) < ETH_TYPE_MIN) {
             /* See comments in odp_flow_key_from_flow__(). */
-            VLOG_ERR_RL(&rl, "mask expected for non-Ethernet II frame");
+            ODP_PARSE_ERROR(&rl, errorp,
+                            "mask expected for non-Ethernet II frame");
             return false;
         }
     }
@@ -6327,7 +6369,7 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                   uint64_t present_attrs, int out_of_range_attr,
                   uint64_t expected_attrs, struct flow *flow,
                   const struct nlattr *key, size_t key_len,
-                  const struct flow *src_flow)
+                  const struct flow *src_flow, char **errorp)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     bool is_mask = src_flow != flow;
@@ -6346,9 +6388,15 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             int i;
 
             if (!size || size % sizeof(ovs_be32)) {
+                ODP_PARSE_ERROR(&rl, errorp,
+                                "MPLS LSEs have invalid length %"PRIuSIZE,
+                                size);
                 return ODP_FIT_ERROR;
             }
             if (flow->mpls_lse[0] && flow->dl_type != htons(0xffff)) {
+                ODP_PARSE_ERROR(&rl, errorp,
+                                "unexpected MPLS Ethertype mask %x"PRIx16,
+                                ntohs(flow->dl_type));
                 return ODP_FIT_ERROR;
             }
 
@@ -6363,6 +6411,8 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                 /* BOS may be set only in the innermost label. */
                 for (i = 0; i < n - 1; i++) {
                     if (flow->mpls_lse[i] & htonl(MPLS_BOS_MASK)) {
+                        ODP_PARSE_ERROR(&rl, errorp,
+                                        "MPLS BOS set in non-innermost label");
                         return ODP_FIT_ERROR;
                     }
                 }
@@ -6386,8 +6436,11 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             ipv4_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV4]);
             put_ipv4_key(ipv4_key, flow, is_mask);
             if (flow->nw_frag > FLOW_NW_FRAG_MASK) {
+                ODP_PARSE_ERROR(&rl, errorp, "OVS_KEY_ATTR_IPV4 has invalid "
+                                "nw_frag %#"PRIx8, flow->nw_frag);
                 return ODP_FIT_ERROR;
             }
+
             if (is_mask) {
                 check_start = ipv4_key;
                 check_len = sizeof *ipv4_key;
@@ -6404,6 +6457,8 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             ipv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV6]);
             put_ipv6_key(ipv6_key, flow, is_mask);
             if (flow->nw_frag > FLOW_NW_FRAG_MASK) {
+                ODP_PARSE_ERROR(&rl, errorp, "OVS_KEY_ATTR_IPV6 has invalid "
+                                "nw_frag %#"PRIx8, flow->nw_frag);
                 return ODP_FIT_ERROR;
             }
             if (is_mask) {
@@ -6422,8 +6477,9 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
 
             arp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ARP]);
             if (!is_mask && (arp_key->arp_op & htons(0xff00))) {
-                VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16" in flow "
-                            "key", ntohs(arp_key->arp_op));
+                ODP_PARSE_ERROR(&rl, errorp,
+                                "unsupported ARP opcode %"PRIu16" in flow "
+                                "key", ntohs(arp_key->arp_op));
                 return ODP_FIT_ERROR;
             }
             put_arp_key(arp_key, flow);
@@ -6438,7 +6494,10 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
             expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_NSH;
         }
         if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_NSH)) {
-            odp_nsh_key_from_attr(attrs[OVS_KEY_ATTR_NSH], &flow->nsh, NULL);
+            if (odp_nsh_key_from_attr(attrs[OVS_KEY_ATTR_NSH], &flow->nsh,
+                                      NULL, errorp) == ODP_FIT_ERROR) {
+                return ODP_FIT_ERROR;
+            }
             if (is_mask) {
                 check_start = nl_attr_get(attrs[OVS_KEY_ATTR_NSH]);
                 check_len = nl_attr_get_size(attrs[OVS_KEY_ATTR_NSH]);
@@ -6451,6 +6510,10 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
     if (check_len > 0) { /* Happens only when 'is_mask'. */
         if (!is_all_zeros(check_start, check_len) &&
             flow->dl_type != htons(0xffff)) {
+            ODP_PARSE_ERROR(&rl, errorp, "unexpected L3 matching with "
+                            "masked Ethertype %#"PRIx16"/%#"PRIx16,
+                            ntohs(src_flow->dl_type),
+                            ntohs(flow->dl_type));
             return ODP_FIT_ERROR;
         } else {
             expected_attrs |= UINT64_C(1) << expected_bit;
@@ -6551,6 +6614,12 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                         if (!is_all_zeros(nd_key, sizeof *nd_key) &&
                             (flow->tp_src != htons(0xff) ||
                              flow->tp_dst != htons(0xff))) {
+                            ODP_PARSE_ERROR(&rl, errorp,
+                                            "ICMP (src,dst) masks should be "
+                                            "(0xff,0xff) but are actually "
+                                            "(%#"PRIx16",%#"PRIx16")",
+                                            ntohs(flow->tp_src),
+                                            ntohs(flow->tp_dst));
                             return ODP_FIT_ERROR;
                         } else {
                             expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND;
@@ -6567,6 +6636,8 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
     }
     if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) {
         if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) {
+            ODP_PARSE_ERROR(&rl, errorp, "flow matches on L4 ports but does "
+                            "not define an L4 protocol");
             return ODP_FIT_ERROR;
         } else {
             expected_attrs |= UINT64_C(1) << expected_bit;
@@ -6584,7 +6655,7 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                    uint64_t present_attrs, int out_of_range_attr,
                    uint64_t expected_attrs, struct flow *flow,
                    const struct nlattr *key, size_t key_len,
-                   const struct flow *src_flow)
+                   const struct flow *src_flow, char **errorp)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     bool is_mask = src_flow != flow;
@@ -6636,9 +6707,9 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                 }
                 return fitness;
             } else if (!(flow->vlans[encaps].tci & htons(VLAN_CFI))) {
-                VLOG_ERR_RL(&rl, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is nonzero "
-                            "but CFI bit is not set",
-                            ntohs(flow->vlans[encaps].tci));
+                ODP_PARSE_ERROR(
+                    &rl, errorp, "OVS_KEY_ATTR_VLAN 0x%04"PRIx16" is nonzero "
+                    "but CFI bit is not set", ntohs(flow->vlans[encaps].tci));
                 return ODP_FIT_ERROR;
             }
         } else {
@@ -6649,13 +6720,14 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
 
         /* Now parse the encapsulated attributes. */
         if (!parse_flow_nlattrs(nl_attr_get(encap), nl_attr_get_size(encap),
-                                attrs, &present_attrs, &out_of_range_attr)) {
+                                attrs, &present_attrs, &out_of_range_attr,
+                                errorp)) {
             return ODP_FIT_ERROR;
         }
         expected_attrs = 0;
 
         if (!parse_ethertype(attrs, present_attrs, &expected_attrs,
-                             flow, src_flow)) {
+                             flow, src_flow, errorp)) {
             return ODP_FIT_ERROR;
         }
 
@@ -6664,7 +6736,7 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
 
     encap_fitness = parse_l2_5_onward(attrs, present_attrs, out_of_range_attr,
                                       expected_attrs, flow, key, key_len,
-                                      src_flow);
+                                      src_flow, errorp);
 
     /* The overall fitness is the worse of the outer and inner attributes. */
     return MAX(fitness, encap_fitness);
@@ -6672,8 +6744,14 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
 
 static enum odp_key_fitness
 odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
-                       struct flow *flow, const struct flow *src_flow)
+                       struct flow *flow, const struct flow *src_flow,
+                       char **errorp)
 {
+    enum odp_key_fitness fitness = ODP_FIT_ERROR;
+    if (errorp) {
+        *errorp = NULL;
+    }
+
     const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1];
     uint64_t expected_attrs;
     uint64_t present_attrs;
@@ -6684,8 +6762,8 @@  odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
 
     /* Parse attributes. */
     if (!parse_flow_nlattrs(key, key_len, attrs, &present_attrs,
-                            &out_of_range_attr)) {
-        return ODP_FIT_ERROR;
+                            &out_of_range_attr, errorp)) {
+        goto exit;
     }
     expected_attrs = 0;
 
@@ -6754,9 +6832,9 @@  odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
         enum odp_key_fitness res;
 
         res = odp_tun_key_from_attr__(attrs[OVS_KEY_ATTR_TUNNEL], is_mask,
-                                      &flow->tunnel);
+                                      &flow->tunnel, errorp);
         if (res == ODP_FIT_ERROR) {
-            return ODP_FIT_ERROR;
+            goto exit;
         } else if (res == ODP_FIT_PERFECT) {
             expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TUNNEL;
         }
@@ -6803,27 +6881,55 @@  odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
 
     /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */
     if (!parse_ethertype(attrs, present_attrs, &expected_attrs, flow,
-                         src_flow)) {
-        return ODP_FIT_ERROR;
+                         src_flow, errorp)) {
+        goto exit;
     }
 
     if (is_mask
         ? (src_flow->vlans[0].tci & htons(VLAN_CFI)) != 0
         : eth_type_vlan(src_flow->dl_type)) {
-        return parse_8021q_onward(attrs, present_attrs, out_of_range_attr,
-                                  expected_attrs, flow, key, key_len, src_flow);
+        fitness = parse_8021q_onward(attrs, present_attrs, out_of_range_attr,
+                                     expected_attrs, flow, key, key_len,
+                                     src_flow, errorp);
+    } else {
+        if (is_mask) {
+            /* A missing VLAN mask means exact match on vlan_tci 0 (== no
+             * VLAN). */
+            flow->vlans[0].tpid = htons(0xffff);
+            flow->vlans[0].tci = htons(0xffff);
+            if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) {
+                flow->vlans[0].tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]);
+                expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_VLAN);
+            }
+        }
+        fitness = parse_l2_5_onward(attrs, present_attrs, out_of_range_attr,
+                                    expected_attrs, flow, key, key_len,
+                                    src_flow, errorp);
     }
-    if (is_mask) {
-        /* A missing VLAN mask means exact match on vlan_tci 0 (== no VLAN). */
-        flow->vlans[0].tpid = htons(0xffff);
-        flow->vlans[0].tci = htons(0xffff);
-        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_VLAN)) {
-            flow->vlans[0].tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]);
-            expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_VLAN);
+
+exit:;
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    if (fitness == ODP_FIT_ERROR && (errorp || !VLOG_DROP_WARN(&rl))) {
+        struct ds s = DS_EMPTY_INITIALIZER;
+        if (is_mask) {
+            ds_put_cstr(&s, "the flow mask in error is: ");
+            odp_flow_key_format(key, key_len, &s);
+            ds_put_cstr(&s, ", for the following flow key: ");
+            flow_format(&s, src_flow, NULL);
+        } else {
+            ds_put_cstr(&s, "the flow key in error is: ");
+            odp_flow_key_format(key, key_len, &s);
+        }
+        if (errorp) {
+            char *old_error = *errorp;
+            *errorp = xasprintf("%s; %s", old_error, ds_cstr(&s));
+            free(old_error);
+        } else {
+            VLOG_WARN("%s", ds_cstr(&s));
         }
+        ds_destroy(&s);
     }
-    return parse_l2_5_onward(attrs, present_attrs, out_of_range_attr,
-                             expected_attrs, flow, key, key_len, src_flow);
+    return fitness;
 }
 
 /* Converts the 'key_len' bytes of OVS_KEY_ATTR_* attributes in 'key' to a flow
@@ -6840,28 +6946,40 @@  odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
  * by looking at the attributes for lower-level protocols, e.g. if the network
  * protocol in OVS_KEY_ATTR_IPV4 or OVS_KEY_ATTR_IPV6 is IPPROTO_TCP then we
  * know that a OVS_KEY_ATTR_TCP attribute must appear and that otherwise it
- * must be absent. */
+ * must be absent.
+ *
+ * If 'errorp' is nonull, this function stores a detailed error report in it,
+ * which the caller must eventually free(), when it returns ODP_FIT_ERROR, When
+ * it returns anything else, it sets '*errorp' to NULL. */
 enum odp_key_fitness
 odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
-                     struct flow *flow)
+                     struct flow *flow, char **errorp)
 {
-   return odp_flow_key_to_flow__(key, key_len, flow, flow);
+    return odp_flow_key_to_flow__(key, key_len, flow, flow, errorp);
 }
 
 /* Converts the 'mask_key_len' bytes of OVS_KEY_ATTR_* attributes in 'mask_key'
  * to a mask structure in 'mask'.  'flow' must be a previously translated flow
  * corresponding to 'mask' and similarly flow_key/flow_key_len must be the
  * attributes from that flow.  Returns an ODP_FIT_* value that indicates how
- * well 'key' fits our expectations for what a flow key should contain. */
+ * well 'key' fits our expectations for what a flow key should contain.
+ *
+ * If 'errorp' is nonull, this function stores a detailed error report in it,
+ * which the caller must eventually free(), when it returns ODP_FIT_ERROR, When
+ * it returns anything else, it sets '*errorp' to NULL. */
 enum odp_key_fitness
 odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len,
-                     struct flow_wildcards *mask, const struct flow *src_flow)
+                     struct flow_wildcards *mask, const struct flow *src_flow,
+                     char **errorp)
 {
     if (mask_key_len) {
         return odp_flow_key_to_flow__(mask_key, mask_key_len,
-                                      &mask->masks, src_flow);
-
+                                      &mask->masks, src_flow, errorp);
     } else {
+        if (errorp) {
+            *errorp = NULL;
+        }
+
         /* A missing mask means that the flow should be exact matched.
          * Generate an appropriate exact wildcard for the flow. */
         flow_wildcards_init_for_packet(mask, src_flow);
@@ -6880,7 +6998,7 @@  parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
 {
     enum odp_key_fitness fitness;
 
-    fitness = odp_flow_key_to_flow(key, key_len, &match->flow);
+    fitness = odp_flow_key_to_flow(key, key_len, &match->flow, NULL);
     if (fitness) {
         /* This should not happen: it indicates that
          * odp_flow_key_from_flow() and odp_flow_key_to_flow() disagree on
@@ -6900,7 +7018,8 @@  parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
         return EINVAL;
     }
 
-    fitness = odp_flow_key_to_mask(mask, mask_len, &match->wc, &match->flow);
+    fitness = odp_flow_key_to_mask(mask, mask_len, &match->wc, &match->flow,
+                                   NULL);
     if (fitness) {
         /* This should not happen: it indicates that
          * odp_flow_key_from_mask() and odp_flow_key_to_mask()
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 6e684f8e6d8b..722ddfabc40d 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -157,10 +157,11 @@  struct odputil_keybuf {
 };
 
 enum odp_key_fitness odp_tun_key_from_attr(const struct nlattr *,
-                                           struct flow_tnl *);
+                                           struct flow_tnl *, char **errorp);
 enum odp_key_fitness odp_nsh_key_from_attr(const struct nlattr *,
                                            struct ovs_key_nsh *,
-                                           struct ovs_key_nsh *);
+                                           struct ovs_key_nsh *,
+                                           char **errorp);
 enum odp_key_fitness odp_nsh_hdr_from_attr(const struct nlattr *,
                                            struct nsh_hdr *, size_t);
 
@@ -172,9 +173,8 @@  void odp_flow_format(const struct nlattr *key, size_t key_len,
                      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,
-                         struct ofpbuf *, struct ofpbuf *);
+int odp_flow_from_string(const char *s, const struct simap *port_names,
+                         struct ofpbuf *, struct ofpbuf *, char **errorp);
 
 /* ODP_SUPPORT_FIELD(TYPE, FIELD_NAME, FIELD_DESCRIPTION)
  *
@@ -261,11 +261,12 @@  enum odp_key_fitness {
     ODP_FIT_ERROR,              /* The key was invalid. */
 };
 enum odp_key_fitness odp_flow_key_to_flow(const struct nlattr *, size_t,
-                                          struct flow *);
+                                          struct flow *, char **errorp);
 enum odp_key_fitness odp_flow_key_to_mask(const struct nlattr *mask_key,
                                           size_t mask_key_len,
                                           struct flow_wildcards *mask,
-                                          const struct flow *flow);
+                                          const struct flow *flow,
+                                          char **errorp);
 int parse_key_and_mask_to_match(const struct nlattr *key, size_t key_len,
                                 const struct nlattr *mask, size_t mask_len,
                                 struct match *match);
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 7da31753c758..c2d2d10ef65c 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -988,7 +988,7 @@  sflow_read_set_action(const struct nlattr *attr,
             /* Do not handle multi-encap for now. */
             sflow_actions->tunnel_err = true;
         } else {
-            if (odp_tun_key_from_attr(attr, &sflow_actions->tunnel)
+            if (odp_tun_key_from_attr(attr, &sflow_actions->tunnel, NULL)
                 == ODP_FIT_ERROR) {
                 /* Tunnel parsing error. */
                 sflow_actions->tunnel_err = true;
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index eca61cee250d..5bdb91741e37 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -347,22 +347,22 @@  parse_flow_and_packet(int argc, const char *argv[],
      * 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 NULL, the flow is a br_flow. */
+    char *error_s;
     if (!odp_flow_from_string(args[n_args - 1], &port_names,
-                              &odp_key, &odp_mask)) {
+                              &odp_key, &odp_mask, &error_s)) {
         if (!backer) {
             error = "Cannot find the datapath";
             goto exit;
         }
 
-        if (odp_flow_key_to_flow(odp_key.data, odp_key.size, flow) == ODP_FIT_ERROR) {
-            error = "Failed to parse datapath flow key";
+        if (odp_flow_key_to_flow(odp_key.data, odp_key.size, flow, &m_err)
+            == ODP_FIT_ERROR) {
             goto exit;
         }
 
         *ofprotop = xlate_lookup_ofproto(backer, flow,
-                                         &flow->in_port.ofp_port);
+                                         &flow->in_port.ofp_port, &m_err);
         if (*ofprotop == NULL) {
-            error = "Invalid datapath flow";
             goto exit;
         }
 
@@ -385,13 +385,15 @@  parse_flow_and_packet(int argc, const char *argv[],
                 goto exit;
             }
         }
+    } else if (n_args != 2) {
+        m_err = xasprintf("%s (or the bridge name was omitted)", error_s);
+        free(error_s);
+        goto exit;
     } else {
-        char *err;
+        free(m_err);
+        m_err = NULL;
 
-        if (n_args != 2) {
-            error = "Must specify bridge name";
-            goto exit;
-        }
+        char *err;
 
         *ofprotop = ofproto_dpif_lookup_by_name(args[0]);
         if (!*ofprotop) {
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index dc3082477106..a19aa9576b5c 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -796,7 +796,7 @@  recv_upcalls(struct handler *handler)
         }
 
         upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,
-                                               flow);
+                                               flow, NULL);
         if (upcall->fitness == ODP_FIT_ERROR) {
             goto free_dupcall;
         }
@@ -1437,7 +1437,8 @@  process_upcall(struct udpif *udpif, struct upcall *upcall,
             memset(&ipfix_actions, 0, sizeof ipfix_actions);
 
             if (upcall->out_tun_key) {
-                odp_tun_key_from_attr(upcall->out_tun_key, &output_tunnel_key);
+                odp_tun_key_from_attr(upcall->out_tun_key, &output_tunnel_key,
+                                      NULL);
             }
 
             actions_len = dpif_read_actions(udpif, upcall, flow,
@@ -2093,7 +2094,7 @@  xlate_key(struct udpif *udpif, const struct nlattr *key, unsigned int len,
     struct xlate_in xin;
     int error;
 
-    fitness = odp_flow_key_to_flow(key, len, &ctx->flow);
+    fitness = odp_flow_key_to_flow(key, len, &ctx->flow, NULL);
     if (fitness == ODP_FIT_ERROR) {
         return EINVAL;
     }
@@ -2186,7 +2187,8 @@  revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
         struct ofproto_dpif *ofproto;
         ofp_port_t ofp_in_port;
 
-        ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port);
+        ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port,
+                                       NULL);
 
         ofpbuf_clear(odp_actions);
 
@@ -2199,7 +2201,8 @@  revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
                           ofproto->up.slowpath_meter_id, &ofproto->uuid);
     }
 
-    if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow)
+    if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow,
+                             NULL)
         == ODP_FIT_ERROR) {
         goto exit;
     }
@@ -2523,7 +2526,7 @@  ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey)
                 netdev_close(ukey->in_netdev);
                 ukey->in_netdev = NULL;
             }
-            res = odp_tun_key_from_attr(k, &tnl);
+            res = odp_tun_key_from_attr(k, &tnl, NULL);
             if (res != ODP_FIT_ERROR) {
                 ukey->in_netdev = flow_get_tunnel_netdev(&tnl);
                 break;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8c13d45d385b..422b38a19bcc 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1482,8 +1482,10 @@  xlate_ofport_remove(struct ofport_dpif *ofport)
 }
 
 static struct ofproto_dpif *
-xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow,
-                      ofp_port_t *ofp_in_port, const struct xport **xportp)
+xlate_lookup_ofproto_(const struct dpif_backer *backer,
+                      const struct flow *flow,
+                      ofp_port_t *ofp_in_port, const struct xport **xportp,
+                      char **errorp)
 {
     struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
     const struct xport *xport;
@@ -1495,6 +1497,10 @@  xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow,
         recirc_id_node = recirc_id_node_find(flow->recirc_id);
 
         if (OVS_UNLIKELY(!recirc_id_node)) {
+            if (errorp) {
+                *errorp = xasprintf("no recirculation data for recirc_id "
+                                    "%"PRIu32, flow->recirc_id);
+            }
             return NULL;
         }
 
@@ -1514,10 +1520,19 @@  xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow,
                          ? tnl_port_receive(flow)
                          : odp_port_to_ofport(backer, flow->in_port.odp_port));
     if (OVS_UNLIKELY(!xport)) {
+        if (errorp) {
+            *errorp = (tnl_port_should_receive(flow)
+                       ? xstrdup("no OpenFlow tunnel port for this packet")
+                       : xasprintf("no OpenFlow tunnel port for datapath "
+                                   "port %"PRIu32, flow->in_port.odp_port));
+        }
         return NULL;
     }
 
 out:
+    if (errorp) {
+        *errorp = NULL;
+    }
     *xportp = xport;
     if (ofp_in_port) {
         *ofp_in_port = xport->ofp_port;
@@ -1529,11 +1544,11 @@  out:
  * returns the corresponding struct ofproto_dpif and OpenFlow port number. */
 struct ofproto_dpif *
 xlate_lookup_ofproto(const struct dpif_backer *backer, const struct flow *flow,
-                     ofp_port_t *ofp_in_port)
+                     ofp_port_t *ofp_in_port, char **errorp)
 {
     const struct xport *xport;
 
-    return xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport);
+    return xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport, errorp);
 }
 
 /* Given a datapath and flow metadata ('backer', and 'flow' respectively),
@@ -1554,7 +1569,7 @@  xlate_lookup(const struct dpif_backer *backer, const struct flow *flow,
     struct ofproto_dpif *ofproto;
     const struct xport *xport;
 
-    ofproto = xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport);
+    ofproto = xlate_lookup_ofproto_(backer, flow, ofp_in_port, &xport, NULL);
 
     if (!ofproto) {
         return ENODEV;
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 0a5a52887b80..5a51d7b303ca 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -199,7 +199,8 @@  void xlate_ofport_remove(struct ofport_dpif *);
 
 struct ofproto_dpif * xlate_lookup_ofproto(const struct dpif_backer *,
                                            const struct flow *,
-                                           ofp_port_t *ofp_in_port);
+                                           ofp_port_t *ofp_in_port,
+                                           char **errorp);
 int xlate_lookup(const struct dpif_backer *, const struct flow *,
                  struct ofproto_dpif **, struct dpif_ipfix **,
                  struct dpif_sflow **, struct netflow **,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2dc5778e1cb3..9c2947a3b9ec 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5694,8 +5694,10 @@  ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
     while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) {
         struct flow flow;
 
-        if (odp_flow_key_to_flow(f.key, f.key_len, &flow) == ODP_FIT_ERROR
-            || xlate_lookup_ofproto(ofproto->backer, &flow, NULL) != ofproto) {
+        if ((odp_flow_key_to_flow(f.key, f.key_len, &flow, NULL)
+             == ODP_FIT_ERROR)
+            || (xlate_lookup_ofproto(ofproto->backer, &flow, NULL, NULL)
+                != ofproto)) {
             continue;
         }
 
diff --git a/tests/odp.at b/tests/odp.at
index 96b4b47dc45a..f5bc064b1a5c 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -396,6 +396,6 @@  AT_DATA([odp-in.txt], [dnl
 encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap()))))))))))))))))))))))))))))))))
 ])
 AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [dnl
-odp_flow_from_string: error
+odp_flow_from_string: error (syntax error at encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap())))))))))))))))))))))))))))))))))
 ])
 AT_CLEANUP
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ea51467cc154..61c42caee049 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5735,7 +5735,7 @@  m4_foreach(
 [AT_CHECK([ovs-appctl ofproto/trace "$br_flow" option],
   [2], [], [stderr])
 AT_CHECK([tail -2 stderr], [0], [dnl
-Must specify bridge name
+syntax error at in_port=1,dl_src=50:54:00:00:00:01,dl_dst=50:54:00:00:00:02 (or the bridge name was omitted)
 ovs-appctl: ovs-vswitchd: server returned an error
 ])])
 
diff --git a/tests/test-odp.c b/tests/test-odp.c
index f61846422051..196d607aef85 100644
--- a/tests/test-odp.c
+++ b/tests/test-odp.c
@@ -46,10 +46,12 @@  parse_keys(bool wc_keys)
         /* Convert string to OVS DP key. */
         ofpbuf_init(&odp_key, 0);
         ofpbuf_init(&odp_mask, 0);
+        char *error_s;
         error = odp_flow_from_string(ds_cstr(&in), NULL,
-                                     &odp_key, &odp_mask);
+                                     &odp_key, &odp_mask, &error_s);
         if (error) {
-            printf("odp_flow_from_string: error\n");
+            printf("odp_flow_from_string: error (%s)\n", error_s);
+            free(error_s);
             goto next;
         }
 
@@ -67,7 +69,8 @@  parse_keys(bool wc_keys)
             };
 
             /* Convert odp_key to flow. */
-            fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
+            fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow,
+                                           &error_s);
             switch (fitness) {
                 case ODP_FIT_PERFECT:
                     break;
@@ -81,7 +84,8 @@  parse_keys(bool wc_keys)
                     break;
 
                 case ODP_FIT_ERROR:
-                    printf("odp_flow_key_to_flow: error\n");
+                    printf("odp_flow_key_to_flow: error (%s)\n", error_s);
+                    free(error_s);
                     goto next;
             }
             /* Convert cls_rule back to odp_key. */
@@ -182,8 +186,10 @@  parse_filter(char *filter_parse)
         /* Convert string to OVS DP key. */
         ofpbuf_init(&odp_key, 0);
         ofpbuf_init(&odp_mask, 0);
-        if (odp_flow_from_string(ds_cstr(&in), NULL, &odp_key, &odp_mask)) {
-            printf("odp_flow_from_string: error\n");
+        char *error_s;
+        if (odp_flow_from_string(ds_cstr(&in), NULL, &odp_key, &odp_mask,
+                                 &error_s)) {
+            printf("odp_flow_from_string: error (%s)\n", error_s);
             goto next;
         }
 
@@ -193,8 +199,9 @@  parse_filter(char *filter_parse)
             struct match match, match_filter;
             struct minimatch minimatch;
 
-            odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
-            odp_flow_key_to_mask(odp_mask.data, odp_mask.size, &wc, &flow);
+            odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, NULL);
+            odp_flow_key_to_mask(odp_mask.data, odp_mask.size, &wc, &flow,
+                                 NULL);
             match_init(&match, &flow, &wc);
 
             match_init(&match_filter, &flow_filter, &wc);
diff --git a/tests/tunnel.at b/tests/tunnel.at
index 55fb1d391ea1..035c54f675ad 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -55,7 +55,7 @@  AT_CHECK([tail -1 stdout], [0],
 
 dnl nonexistent tunnel
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(src=5.5.5.5,dst=6.6.6.6,ttl=64,flags()),in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'], [2], [ignore], [dnl
-Invalid datapath flow
+no OpenFlow tunnel port for this packet
 ovs-appctl: ovs-vswitchd: server returned an error
 ])
 
@@ -314,7 +314,7 @@  set(tunnel(tun_id=0x3,dst=1.1.1.1,ttl=64,flags(df|key))),1
 ])
 
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(tun_id=0xf,src=1.1.1.1,dst=2.2.2.2,ttl=64,flags(key)),in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'], [2], [ignore], [dnl
-Invalid datapath flow
+no OpenFlow tunnel port for this packet
 ovs-appctl: ovs-vswitchd: server returned an error
 ])
 OVS_VSWITCHD_STOP(["/receive tunnel port not found/d"])