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

[Ben Pfaff]

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

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev