> 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
