From patchwork Sat Dec 15 02:16:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1013801 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43GrgQ1Tpkz9s3Z for ; Sat, 15 Dec 2018 13:18:26 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id C5EE4E9E; Sat, 15 Dec 2018 02:17:10 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 82D38D33 for ; Sat, 15 Dec 2018 02:17:09 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3DCDBE7 for ; Sat, 15 Dec 2018 02:17:06 +0000 (UTC) X-Originating-IP: 75.54.222.30 Received: from sigabrt.attlocal.net (75-54-222-30.lightspeed.rdcyca.sbcglobal.net [75.54.222.30]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 356C660008; Sat, 15 Dec 2018 02:17:02 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Fri, 14 Dec 2018 18:16:55 -0800 Message-Id: <20181215021655.29228-3-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181215021655.29228-1-blp@ovn.org> References: <20181215021655.29228-1-blp@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 3/3] odp-util: Improve log messages and error reporting for Netlink parsing. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- 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(-) 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"])