Forgot my: Acked-by: Justin Pettit <[email protected]>
Thanks for the patch. It's always nice to have better debugging. --Justin > On Feb 2, 2019, at 3:25 PM, Justin Pettit <[email protected]> wrote: > > >> On Dec 14, 2018, at 6:16 PM, Ben Pfaff <[email protected]> wrote: >> >> diff --git a/lib/dpctl.c b/lib/dpctl.c >> index 59071cdba83d..1632fad89a57 100644 >> --- a/lib/dpctl.c >> +++ b/lib/dpctl.c >> @@ -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; > > Shouldn't 'error_s' be freed here? > >> 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, ...) \ > > The behavior of this macro is a little unusual, so I think it may be worth > providing a brief comment explaining it. > >> + do { \ >> + if (OVS_UNLIKELY(ERRORP)) { \ >> + free(*(ERRORP)); \ >> + *(ERRORP) = xasprintf(__VA_ARGS__); \ >> + } else { \ >> + VLOG_WARN_RL(RL, __VA_ARGS__); \ > > I think there's an extra space before the "\". > > Also, most of the existing functions that log parse errors do it at level > ERR, but this lowers it to WARN. I doubt users expect these normally, but it > may still be worth mentioning in the commit message. > >> 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) > > There are a few instances in this patch where an optional error message > pointer is added as an argument, but under what circumstances and how it > should be freed are not mentioned. These are generally in functions, such as > this one, without an existing description of the function. I won't call out > all the instances, but I just point it out here in case you think it's worth > going through the patch and adding then. > >> @@ -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) > > I think it would be worth explaining under what circumstances '*errorp' is > set and whether the user needs to free it, since there's already a comment. > >> @@ -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. */ > > The phrasing of this new paragraph reads a little strange to me. Also, I > believe it should be a period after "ODP_FIT_ERROR". > >> 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. */ > > Same here. > >> 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; > > I found this logic to be confusing since this function seems to define four > different error strings: "err", "error_s", "error", and "m_err". It would be > nice if that could be simplified somehow. It might be a bit clearer if you > somehow distinguished between "error_s" and "m_err", and moved the definition > of "err" closer to its use. > >> 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 >> >> @@ -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; >> } > > I think "error_s" needs to be freed. > > --Justin > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
