From d40533fc820c30a461e7eefe4bcfee3f799d0f11 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 14 Dec 2018 18:16:55 -0800 Subject: [PATCH] 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 --- lib/dpctl.c | 31 +-- lib/dpif-netdev.c | 4 +- lib/netdev-dummy.c | 15 +- lib/odp-execute.c | 6 +- lib/odp-util.c | 342 ++++++++++++++++++++++++++-------- lib/odp-util.h | 15 +- ofproto/ofproto-dpif-sflow.c | 2 +- ofproto/ofproto-dpif-trace.c | 78 ++++---- 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 | 26 ++- tests/tunnel.at | 4 +- 16 files changed, 401 insertions(+), 175 deletions(-) diff --git a/lib/dpctl.c b/lib/dpctl.c index f5a09b70f..947a49099 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008-2018 Nicira, Inc. + * Copyright (c) 2008-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. @@ -1023,8 +1023,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); @@ -1112,10 +1112,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; } @@ -1264,9 +1266,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; } @@ -2281,9 +2285,12 @@ 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); + free(error_s); goto out_freekeybuf; } @@ -2292,9 +2299,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 77ac1d2c1..ec84378a0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3072,7 +3072,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 @@ -3103,7 +3103,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 72b4f7adc..78aa34957 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 5d0713375..3ad91547c 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 @@ -297,9 +297,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 e288ae8e5..8bcbd2c30 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.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. @@ -2647,10 +2647,48 @@ odp_nsh_hdr_from_attr(const struct nlattr *attr, return ODP_FIT_PERFECT; } +/* Reports the error 'msg', which is formatted as with printf(). + * + * If 'errorp' is nonnull, then some the wants the error report to come + * directly back to it, so the function stores the error message into '*errorp' + * (after first freeing it in case there's something there already). + * + * Otherwise, logs the message at WARN level, rate-limited. */ +static void OVS_PRINTF_FORMAT(3, 4) +odp_parse_error(struct vlog_rate_limit *rl, char **errorp, + const char *msg, ...) +{ + if (OVS_UNLIKELY(errorp)) { + free(*errorp); + + va_list args; + va_start(args, msg); + *errorp = xvasprintf(msg, args); + va_end(args); + } else if (!VLOG_DROP_WARN(rl)) { + va_list args; + va_start(args, msg); + char *error = xvasprintf(msg, args); + va_end(args); + + VLOG_WARN("%s", error); + + free(error); + } +} + +/* Parses OVS_KEY_ATTR_NSH attribute 'attr' into 'nsh' and 'nsh_mask' and + * returns fitness. If 'errorp' is nonnull and the function returns + * ODP_FIT_ERROR, stores a malloc()'d error message in '*errorp'. */ 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; @@ -2661,17 +2699,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) { @@ -2719,16 +2758,24 @@ 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; } return ODP_FIT_PERFECT; } +/* Parses OVS_KEY_ATTR_TUNNEL attribute 'attr' into 'tun' and returns fitness. + * If the attribute is a key, 'is_mask' should be false; if it is a mask, + * 'is_mask' should be true. If 'errorp' is nonnull and the function returns + * ODP_FIT_ERROR, stores a malloc()'d error message in '*errorp'. */ 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; @@ -2741,6 +2788,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; } @@ -2790,6 +2840,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; } @@ -2829,6 +2880,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) { @@ -2837,11 +2889,19 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, return ODP_FIT_PERFECT; } +/* Parses OVS_KEY_ATTR_TUNNEL key attribute 'attr' into 'tun' and returns + * fitness. The attribute should be a key (not a mask). If 'errorp' is + * nonnull, stores NULL into '*errorp' on success, otherwise a malloc()'d error + * message. */ 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 @@ -5646,12 +5706,12 @@ parse_odp_key_mask_attr(struct parse_odp_context *context, const char *s, return -EINVAL; } -/* Parses the string representation of a datapath flow key, in the - * format output by odp_flow_key_format(). Returns 0 if successful, - * otherwise a positive errno value. On success, the flow key is - * appended to 'key' as a series of Netlink attributes. On failure, no - * data is appended to 'key'. Either way, 'key''s data might be - * reallocated. +/* Parses the string representation of a datapath flow key, in the format + * output by odp_flow_key_format(). Returns 0 if successful, otherwise a + * positive errno value. On success, stores NULL into '*errorp' and the flow + * key is appended to 'key' as a series of Netlink attributes. On failure, + * stores a malloc()'d error message in '*errorp' without changing the data in + * 'key'. Either way, 'key''s data might be reallocated. * * If 'port_names' is nonnull, it points to an simap that maps from a port name * to a port number. (Port names may be used instead of port numbers in @@ -5662,8 +5722,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, @@ -5680,6 +5743,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) { @@ -5689,6 +5753,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; } @@ -6133,7 +6198,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); } @@ -6240,10 +6305,22 @@ odp_to_ovs_frag(uint8_t odp_frag, bool is_mask) : FLOW_NW_FRAG_ANY | FLOW_NW_FRAG_LATER; } +/* Parses the attributes in the 'key_len' bytes of 'key' into 'attrs', which + * must have OVS_KEY_ATTR_MAX + 1 elements. Stores each attribute in 'key' + * into the corresponding element of 'attrs'. + * + * Stores a bitmask of the attributes' indexes found in 'key' into + * '*present_attrsp'. + * + * If an attribute beyond OVS_KEY_ATTR_MAX is found, stores its attribute type + * (or one of them, if more than one) into '*out_of_range_attrp', otherwise 0. + * + * If 'errorp' is nonnull and the function returns false, stores a malloc()'d + * error message in '*errorp'. */ 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; @@ -6262,10 +6339,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; } @@ -6275,9 +6353,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; } @@ -6286,7 +6365,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; } @@ -6321,10 +6400,22 @@ check_expectations(uint64_t present_attrs, int out_of_range_attr, return ODP_FIT_PERFECT; } +/* Initializes 'flow->dl_type' based on the attributes in 'attrs', in which the + * attributes in the bit-mask 'present_attrs' are present. Returns true if + * successful, false on failure. + * + * Sets 1-bits in '*expected_attrs' for the attributes in 'attrs' that were + * consulted. 'flow' is assumed to be a flow key unless 'src_flow' is nonnull, + * in which case 'flow' is a flow mask and 'src_flow' is its corresponding + * previously parsed flow key. + * + * If 'errorp' is nonnull and the function returns false, stores a malloc()'d + * error message in '*errorp'. */ 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; @@ -6332,12 +6423,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; @@ -6358,19 +6453,37 @@ 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; } } return true; } +/* Initializes MPLS, L3, and L4 fields in 'flow' based on the attributes in + * 'attrs', in which the attributes in the bit-mask 'present_attrs' are + * present. The caller also indicates an out-of-range attribute + * 'out_of_range_attr' if one was present when parsing (if so, the fitness + * cannot be perfect). + * + * Sets 1-bits in '*expected_attrs' for the attributes in 'attrs' that were + * consulted. 'flow' is assumed to be a flow key unless 'src_flow' is nonnull, + * in which case 'flow' is a flow mask and 'src_flow' is its corresponding + * previously parsed flow key. + * + * Returns fitness based on any discrepancies between present and expected + * attributes, except that a 'need_check' of false overrides this. + * + * If 'errorp' is nonnull and the function returns false, stores a malloc()'d + * error message in '*errorp'. 'key' and 'key_len' are just used for error + * reporting in this case. */ static enum odp_key_fitness 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, bool need_check) + const struct flow *src_flow, bool need_check, char **errorp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); bool is_mask = src_flow != flow; @@ -6389,9 +6502,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; } @@ -6406,6 +6525,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; } } @@ -6429,8 +6550,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; @@ -6447,6 +6571,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) { @@ -6465,8 +6591,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); @@ -6481,7 +6608,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]); @@ -6494,6 +6624,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; @@ -6594,6 +6728,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; @@ -6639,6 +6779,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; @@ -6656,7 +6798,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; @@ -6708,9 +6850,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 { @@ -6721,20 +6863,21 @@ 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; } encap_fitness = parse_l2_5_onward(attrs, present_attrs, out_of_range_attr, &expected_attrs, flow, key, key_len, - src_flow, false); + src_flow, false, errorp); if (encap_fitness != ODP_FIT_PERFECT) { return encap_fitness; } @@ -6747,8 +6890,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; @@ -6759,8 +6908,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; @@ -6829,9 +6978,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; } @@ -6878,28 +7027,56 @@ 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, true, 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, true); + return fitness; } /* Converts the 'key_len' bytes of OVS_KEY_ATTR_* attributes in 'key' to a flow @@ -6916,28 +7093,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 nonnull, this function uses it for detailed error reports: if + * the return value is ODP_FIT_ERROR, it stores a malloc()'d error string in + * '*errorp', otherwise 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 nonnull, this function uses it for detailed error reports: if + * the return value is ODP_FIT_ERROR, it stores a malloc()'d error string in + * '*errorp', otherwise 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); @@ -6956,7 +7145,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 @@ -6976,7 +7165,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 aa8e6efe3..a03e82532 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 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; } diff --git a/tests/odp.at b/tests/odp.at index 558c0c6a0..f92f989ca 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -397,6 +397,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 ded2ef013..03e1fab05 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5707,7 +5707,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 f61846422..09fec706a 100644 --- a/tests/test-odp.c +++ b/tests/test-odp.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc. + * Copyright (c) 2011, 2012, 2013, 2014, 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. @@ -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,11 @@ 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); + free(error_s); goto next; } @@ -193,8 +200,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 55fb1d391..035c54f67 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"]) -- 2.39.2