[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