secchan: Factor common code into new function rule_insert().

This is primarily a code cleanup.  It also fixes a corner case for
statistics that formerly was properly handled in add_flow() but not in
ofproto_add_flow().

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index b21acbb1742a017b6525bf9e7dd715f065912842..28e19ea06f9cae97d10030fbbe5e9e5fd661a7d8 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -163,6 +163,8 @@ static struct rule *rule_create(struct rule *super, const union ofp_action *,
 static void rule_free(struct rule *);
 static void rule_destroy(struct ofproto *, struct rule *);
 static struct rule *rule_from_cls_rule(const struct cls_rule *);
+static void rule_insert(struct ofproto *, struct rule *,
+                        struct ofpbuf *packet);
 static bool rule_make_actions(struct ofproto *, struct rule *,
                               const struct ofpbuf *packet);
 static void rule_install(struct ofproto *, struct rule *,
@@ -956,24 +958,11 @@ ofproto_add_flow(struct ofproto *p,
                  const union ofp_action *actions, size_t n_actions,
                  int idle_timeout)
 {
-    struct rule *rule, *displaced_rule;
-
+    struct rule *rule;
     rule = rule_create(NULL, actions, n_actions,
                        idle_timeout >= 0 ? idle_timeout : 5 /* XXX */, 0);
     cls_rule_from_flow(&rule->cr, flow, wildcards, priority);
-
-    displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
-    if (displaced_rule) {
-        rule_destroy(p, displaced_rule);
-    }
-
-    if (!wildcards) {
-        rule_make_actions(p, rule, NULL);
-        rule_install(p, rule, NULL);
-    } else {
-        /* We might need to change the rules for arbitrary subrules. */
-        p->need_revalidate = true;
-    }
+    rule_insert(p, rule, NULL);
 }
 
 void
@@ -1399,6 +1388,46 @@ rule_has_out_port(const struct rule *rule, uint16_t out_port)
     return false;
 }
 
+static void
+rule_insert(struct ofproto *p, struct rule *rule, struct ofpbuf *packet)
+{
+    struct rule *displaced_rule;
+
+    displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
+    if (rule->cr.wc.wildcards) {
+        if (displaced_rule) {
+            /* The displaced rule matches exactly the same packets as the new
+             * rule, and it has the same priority, so we could usually transfer
+             * all displaced_rule's subrules to the new rule, then update the
+             * subrules' flow in the datapath.  We used to do this.  But there
+             * is one case where this optimization doesn't work: when
+             * 'displaced_rule' is a rule selected by NXAST_RESUBMIT, because
+             * in such a case 'displaced_rule' will not be the super-rule of
+             * the subrule that got actions from 'displaced_rule'.  We could
+             * add more data to track such situations, but we have no evidence
+             * that the optimization would be worthwhile performance-wise
+             * anyhow.  So we just let rule_destroy() check each subrule and
+             * transfer or destroy them as necessary.
+             */
+        }
+
+        /* We might need to change the rules for arbitrary subrules. */
+        p->need_revalidate = true;
+    } else {
+        struct odp_flow_stats stats;
+
+        rule_make_actions(p, rule, packet);
+        rule_install(p, rule, &stats);
+
+        if (displaced_rule && displaced_rule->super) {
+            update_stats(displaced_rule->super, &stats);
+        }
+    }
+    if (displaced_rule) {
+        rule_destroy(p, displaced_rule);
+    }
+}
+
 /* Returns true if the actions changed, false otherwise. */
 static bool
 rule_make_actions(struct ofproto *p, struct rule *rule,
@@ -2386,6 +2415,7 @@ send_buffered(struct ofproto *p, struct ofconn *ofconn, uint32_t buffer_id,
     flow_t flow;
     int error;
 
+    *packetp = NULL;
     if (!ofconn->pktbuf) {
         VLOG_WARN_RL(&rl, "attempt to send buffered packet via connection "
                      "without buffers");
@@ -2421,9 +2451,9 @@ static int
 add_flow(struct ofproto *p, struct ofconn *ofconn,
          struct ofp_flow_mod *ofm, size_t n_actions)
 {
-    struct rule *rule, *displaced_rule;
-    struct ofpbuf *packet = NULL;
-    int error = 0;
+    struct ofpbuf *packet;
+    struct rule *rule;
+    int error;
 
     rule = rule_create(NULL, (const union ofp_action *) ofm->actions,
                        n_actions, ntohs(ofm->idle_timeout),
@@ -2432,41 +2462,12 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
 
     if (ofm->buffer_id != htonl(UINT32_MAX)) {
         error = send_buffered(p, ofconn, ntohl(ofm->buffer_id), rule, &packet);
-    }
-
-    displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
-    if (rule->cr.wc.wildcards) {
-        if (displaced_rule) {
-            /* The displaced rule matches exactly the same packets as the new
-             * rule, and it has the same priority, so we could usually transfer
-             * all displaced_rule's subrules to the new rule, then update the
-             * subrules' flow in the datapath.  We used to do this.  But there
-             * is one case where this optimization doesn't work: when
-             * 'displaced_rule' is a rule selected by NXAST_RESUBMIT, because
-             * in such a case 'displaced_rule' will not be the super-rule of
-             * the subrule that got actions from 'displaced_rule'.  We could
-             * add more data to track such situations, but we have no evidence
-             * that the optimization would be worthwhile performance-wise
-             * anyhow.  So we just let rule_destroy() check each subrule and
-             * transfer or destroy them as necessary.
-             */
-            rule_destroy(p, displaced_rule);
-        }
-
-        /* We might need to change the rules for arbitrary subrules. */
-        p->need_revalidate = true;
     } else {
-        struct odp_flow_stats stats;
-
-        rule_make_actions(p, rule, packet);
-        rule_install(p, rule, &stats);
-        if (displaced_rule) {
-            if (displaced_rule->super) {
-                update_stats(displaced_rule->super, &stats);
-            }
-            rule_destroy(p, displaced_rule);
-        }
+        packet = NULL;
+        error = 0;
     }
+
+    rule_insert(p, rule, packet);
     ofpbuf_delete(packet);
     return error;
 }