odp-util: Improve log messages and error reporting for Netlink parsing.

As a side effect, this also reduces a lot of log messages' severities from
ERR to WARN.  They just didn't seem like messages that in general reported
anything that would prevent functioning.

Signed-off-by: Ben Pfaff <blp@ovn.org>

6 files changed, 72 insertions, 57 deletions

diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index bc4ffee62..22cbdae7a 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 4a981e1e4..8ae8a221a 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -195,8 +195,7 @@ parse_flow_and_packet(int argc, const char *argv[],
                       bool *consistent)
 {
     const struct dpif_backer *backer = NULL;
-    const char *error = NULL;
-    char *m_err = NULL;
+    char *error = NULL;
     struct simap port_names = SIMAP_INITIALIZER(&port_names);
     struct dp_packet *packet = NULL;
     uint8_t *l7 = NULL;
@@ -219,7 +218,7 @@ parse_flow_and_packet(int argc, const char *argv[],
             generate_packet = true;
         } else if (!strcmp(arg, "--l7")) {
             if (i + 1 >= argc) {
-                m_err = xasprintf("Missing argument for option %s", arg);
+                error = xasprintf("Missing argument for option %s", arg);
                 goto exit;
             }
 
@@ -228,7 +227,7 @@ parse_flow_and_packet(int argc, const char *argv[],
             dp_packet_init(&payload, 0);
             if (dp_packet_put_hex(&payload, argv[++i], NULL)[0] != '\0') {
                 dp_packet_uninit(&payload);
-                error = "Trailing garbage in packet data";
+                error = xstrdup("Trailing garbage in packet data");
                 goto exit;
             }
             free(l7);
@@ -236,14 +235,14 @@ parse_flow_and_packet(int argc, const char *argv[],
             l7 = dp_packet_steal_data(&payload);
         } else if (!strcmp(arg, "--l7-len")) {
             if (i + 1 >= argc) {
-                m_err = xasprintf("Missing argument for option %s", arg);
+                error = xasprintf("Missing argument for option %s", arg);
                 goto exit;
             }
             free(l7);
             l7 = NULL;
             l7_len = atoi(argv[++i]);
             if (l7_len > 64000) {
-                m_err = xasprintf("%s: too much L7 data", argv[i]);
+                error = xasprintf("%s: too much L7 data", argv[i]);
                 goto exit;
             }
         } else if (consistent
@@ -252,7 +251,7 @@ parse_flow_and_packet(int argc, const char *argv[],
             *consistent = true;
         } else if (!strcmp(arg, "--ct-next")) {
             if (i + 1 >= argc) {
-                m_err = xasprintf("Missing argument for option %s", arg);
+                error = xasprintf("Missing argument for option %s", arg);
                 goto exit;
             }
 
@@ -260,15 +259,15 @@ parse_flow_and_packet(int argc, const char *argv[],
             struct ds ds = DS_EMPTY_INITIALIZER;
             if (!parse_ct_state(argv[++i], 0, &ct_state, &ds)
                 || !validate_ct_state(ct_state, &ds)) {
-                m_err = ds_steal_cstr(&ds);
+                error = ds_steal_cstr(&ds);
                 goto exit;
             }
             oftrace_push_ct_state(next_ct_states, ct_state);
         } else if (arg[0] == '-') {
-            m_err = xasprintf("%s: unknown option", arg);
+            error = xasprintf("%s: unknown option", arg);
             goto exit;
         } else if (n_args >= ARRAY_SIZE(args)) {
-            m_err = xstrdup("too many arguments");
+            error = xstrdup("too many arguments");
             goto exit;
         } else {
             args[n_args++] = arg;
@@ -293,14 +292,14 @@ parse_flow_and_packet(int argc, const char *argv[],
      * If there is a packet, we strip it off.
      */
     if (!generate_packet && n_args > 1) {
-        error = eth_from_hex(args[n_args - 1], &packet);
-        if (!error) {
+        const char *const_error = eth_from_hex(args[n_args - 1], &packet);
+        if (!const_error) {
             n_args--;
         } else if (n_args > 2) {
             /* The 3-argument form must end in a hex string. */
+            error = xstrdup(const_error);
             goto exit;
         }
-        error = NULL;
     }
 
     /* We stripped off the packet if there was one, so 'args' now has one of
@@ -331,7 +330,7 @@ parse_flow_and_packet(int argc, const char *argv[],
             backer = node->data;
         }
     } else {
-        error = "Syntax error";
+        error = xstrdup("Syntax error");
         goto exit;
     }
     if (backer && backer->dpif) {
@@ -348,21 +347,20 @@ parse_flow_and_packet(int argc, const char *argv[],
      * returns 0, the flow is a odp_flow. If function
      * parse_ofp_exact_flow() returns NULL, the flow is a br_flow. */
     if (!odp_flow_from_string(args[n_args - 1], &port_names,
-                              &odp_key, &odp_mask)) {
+                              &odp_key, &odp_mask, &error)) {
         if (!backer) {
-            error = "Cannot find the datapath";
+            error = xstrdup("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, &error)
+            == ODP_FIT_ERROR) {
             goto exit;
         }
 
         *ofprotop = xlate_lookup_ofproto(backer, flow,
-                                         &flow->in_port.ofp_port);
+                                         &flow->in_port.ofp_port, &error);
         if (*ofprotop == NULL) {
-            error = "Invalid datapath flow";
             goto exit;
         }
 
@@ -375,27 +373,26 @@ parse_flow_and_packet(int argc, const char *argv[],
          * in OpenFlow format, which is what we use here. */
         if (flow->tunnel.flags & FLOW_TNL_F_UDPIF) {
             struct flow_tnl tnl;
-            int err;
-
             memcpy(&tnl, &flow->tunnel, sizeof tnl);
-            err = tun_metadata_from_geneve_udpif(flow->tunnel.metadata.tab,
-                                                 &tnl, &tnl, &flow->tunnel);
+            int err = tun_metadata_from_geneve_udpif(
+                flow->tunnel.metadata.tab, &tnl, &tnl, &flow->tunnel);
             if (err) {
-                error = "Failed to parse Geneve options";
+                error = xstrdup("Failed to parse Geneve options");
                 goto exit;
             }
         }
+    } else if (n_args != 2) {
+        char *s = error;
+        error = xasprintf("%s (or the bridge name was omitted)", s);
+        free(s);
+        goto exit;
     } else {
-        char *err;
-
-        if (n_args != 2) {
-            error = "Must specify bridge name";
-            goto exit;
-        }
+        free(error);
+        error = NULL;
 
         *ofprotop = ofproto_dpif_lookup_by_name(args[0]);
         if (!*ofprotop) {
-            m_err = xasprintf("%s: unknown bridge", args[0]);
+            error = xasprintf("%s: unknown bridge", args[0]);
             goto exit;
         }
 
@@ -405,12 +402,12 @@ parse_flow_and_packet(int argc, const char *argv[],
             ofputil_port_map_put(&map, ofport->ofp_port,
                                  netdev_get_name(ofport->netdev));
         }
-        err = parse_ofp_exact_flow(flow, NULL,
-                                   ofproto_get_tun_tab(&(*ofprotop)->up),
-                                   args[n_args - 1], &map);
+        char *err = parse_ofp_exact_flow(flow, NULL,
+                                         ofproto_get_tun_tab(&(*ofprotop)->up),
+                                         args[n_args - 1], &map);
         ofputil_port_map_destroy(&map);
         if (err) {
-            m_err = xasprintf("Bad openflow flow syntax: %s", err);
+            error = xasprintf("Bad openflow flow syntax: %s", err);
             free(err);
             goto exit;
         }
@@ -428,10 +425,7 @@ parse_flow_and_packet(int argc, const char *argv[],
     }
 
 exit:
-    if (error && !m_err) {
-        m_err = xstrdup(error);
-    }
-    if (m_err) {
+    if (error) {
         dp_packet_delete(packet);
         packet = NULL;
     }
@@ -440,7 +434,7 @@ exit:
     ofpbuf_uninit(&odp_mask);
     simap_destroy(&port_names);
     free(l7);
-    return m_err;
+    return error;
 }
 
 static void
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index dc3082477..a19aa9576 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 81b72be37..c4014d71b 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 0a5a52887..5a51d7b30 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 50c959bbf..21dd54bab 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5695,8 +5695,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;
         }