[ovs-dev] [multiple tables 3/8] ofproto-dpif: Make ofproto/trace accept an odp_flow in place of a packet.

[Ethan Jackson]

Looks good.

Ethan

On Mon, Aug 8, 2011 at 16:02, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Aug 08, 2011 at 03:35:39PM -0700, Ethan Jackson wrote:
>> The indentation is slightly messed up in the man page (when viewed
>> with "man").  The first occurrence of "ovs-vswitchd  will  respond
>> with extensive" should be indented further like the second occurrence
>> is.
>
> Thanks, fixed.
>
> (Have I mentioned just how much "nroff" syntax blows?)
>
>> Also, I noticed another problem in ofproto_unixctl_trace() which I
>> don't think was introduced by this patch.  trace_format_rule()
>> dereferences the result of rule_dpif_lookup() before checking if it's
>> NULL.  Seems like trace_format_rule should really take a rule_dpif
>> instead.
>
> You're right.
>
> Here's a fix:
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <blp at nicira.com>
> Date: Mon, 8 Aug 2011 16:01:05 -0700
> Subject: [PATCH] ofproto-dpif: Fix pointer arithmetic on null pointer.
>
> rule_dpif_lookup() can return a null pointer as 'rule' so we shouldn't
> try to calculate '&rule->up' before it's been found to be nonnull.
>
> This doesn't appear to fix a real bug because 'up' is the first member
> of 'rule' so when rule is NULL then &rule->up is also NULL.
>
> Reported-by:  Ethan Jackson <ethan at nicira.com>
> ---
>  ofproto/ofproto-dpif.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2aea4e1..4cad7af 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3920,7 +3920,7 @@ struct ofproto_trace {
>  };
>
>  static void
> -trace_format_rule(struct ds *result, int level, const struct rule *rule)
> +trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule)
>  {
>     ds_put_char_multiple(result, '\t', level);
>     if (!rule) {
> @@ -3929,13 +3929,13 @@ trace_format_rule(struct ds *result, int level, const struct rule *rule)
>     }
>
>     ds_put_format(result, "Rule: cookie=%#"PRIx64" ",
> -                  ntohll(rule->flow_cookie));
> -    cls_rule_format(&rule->cr, result);
> +                  ntohll(rule->up.flow_cookie));
> +    cls_rule_format(&rule->up.cr, result);
>     ds_put_char(result, '\n');
>
>     ds_put_char_multiple(result, '\t', level);
>     ds_put_cstr(result, "OpenFlow ");
> -    ofp_print_actions(result, rule->actions, rule->n_actions);
> +    ofp_print_actions(result, rule->up.actions, rule->up.n_actions);
>     ds_put_char(result, '\n');
>  }
>
> @@ -3962,7 +3962,7 @@ trace_resubmit(struct action_xlate_ctx *ctx, struct rule_dpif *rule)
>
>     ds_put_char(result, '\n');
>     trace_format_flow(result, ctx->recurse + 1, "Resubmitted flow", trace);
> -    trace_format_rule(result, ctx->recurse + 1, &rule->up);
> +    trace_format_rule(result, ctx->recurse + 1, rule);
>  }
>
>  static void
> @@ -4050,7 +4050,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, const char *args_,
>     ds_put_char(&result, '\n');
>
>     rule = rule_dpif_lookup(ofproto, &flow);
> -    trace_format_rule(&result, 0, &rule->up);
> +    trace_format_rule(&result, 0, rule);
>     if (rule) {
>         struct ofproto_trace trace;
>         struct ofpbuf *odp_actions;
> --
> 1.7.4.4
>
>