[ovs-dev] [PATCH 3/3] odp-util: Improve log messages and error reporting for Netlink parsing.

Ben Pfaff blp at ovn.org
Mon Feb 25 23:48:00 UTC 2019


On Sat, Feb 02, 2019 at 03:25:45PM -0800, Justin Pettit wrote:
> 
> > On Dec 14, 2018, at 6:16 PM, Ben Pfaff <blp at ovn.org> 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?

Thanks, fixed.

> > 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.

Thanks, I did that.

While I was doing it, I realized that it didn't need to be a macro at
all, so I changed it into a function.

> 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.

OK, done.

> > 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.

Good idea.  I added several comments.

> > @@ -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.

Done.

> > @@ -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".

I rephrased it.

> > 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.

Ditto.

> > 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.

It *was* confusing.  I reduced it to one function-level variable and a
few block-level variables where necessary.

> > 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.

Thanks, done.

I applied this to master.


More information about the dev mailing list