classifier: Prepare for "struct cls_rule" needing to be destroyed.

Until now, "struct cls_rule" didn't own any data outside its own memory
block.  An upcoming commit will make "struct cls_rule" sometimes own blocks
of memory, so it needs "destroy" and to a lesser extent "clone" functions.
This commit adds these in advance, even though they are mostly no-ops, to
make it possible to separately review the memory management.

Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/classifier.c b/lib/classifier.c
index 919e05f..f6f7a64 100644 (file)
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -56,6 +56,8 @@ static struct cls_rule *next_rule_in_list(struct cls_rule *);
 /* Initializes 'rule' to match packets specified by 'match' at the given
  * 'priority'.
  *
+ * The caller must eventually destroy 'rule' with cls_rule_destroy().
+ *
  * 'match' must satisfy the invariant described in the comment at the
  * definition of struct match.
  *
@@ -69,6 +71,25 @@ cls_rule_init(struct cls_rule *rule,
     rule->priority = priority;
 }
 
+/* Initializes 'dst' as a copy of 'src'.
+ *
+ * The caller must eventually destroy 'rule' with cls_rule_destroy(). */
+void
+cls_rule_clone(struct cls_rule *dst, const struct cls_rule *src)
+{
+    *dst = *src;
+}
+
+/* Frees memory referenced by 'rule'.  Doesn't free 'rule' itself (it's
+ * normally embedded into a larger structure).
+ *
+ * ('rule' must not currently be in a classifier.) */
+void
+cls_rule_destroy(struct cls_rule *rule OVS_UNUSED)
+{
+    /* Nothing to do yet. */
+}
+
 /* Returns true if 'a' and 'b' match the same packets at the same priority,
  * false if they differ in some way. */
 bool
@@ -137,7 +158,8 @@ classifier_count(const struct classifier *cls)
  * If 'cls' already contains an identical rule (including wildcards, values of
  * fixed fields, and priority), replaces the old rule by 'rule' and returns the
  * rule that was replaced.  The caller takes ownership of the returned rule and
- * is thus responsible for freeing it, etc., as necessary.
+ * is thus responsible for destroying it with cls_rule_destroy(), freeing the
+ * memory block in which it resides, etc., as necessary.
  *
  * Returns NULL if 'cls' does not contain a rule with an identical key, after
  * inserting the new rule.  In this case, no rules are displaced by the new
@@ -175,8 +197,9 @@ classifier_insert(struct classifier *cls, struct cls_rule *rule)
     assert(!displaced_rule);
 }
 
-/* Removes 'rule' from 'cls'.  It is the caller's responsibility to free
- * 'rule', if this is desirable. */
+/* Removes 'rule' from 'cls'.  It is the caller's responsibility to destroy
+ * 'rule' with cls_rule_destroy(), freeing the memory block in which 'rule'
+ * resides, etc., as necessary. */
 void
 classifier_remove(struct classifier *cls, struct cls_rule *rule)
 {
@@ -261,6 +284,7 @@ classifier_find_match_exactly(const struct classifier *cls,
 
     cls_rule_init(&cr, target, priority);
     retval = classifier_find_rule_exactly(cls, &cr);
+    cls_rule_destroy(&cr);
 
     return retval;
 }

diff --git a/lib/classifier.h b/lib/classifier.h
index 3370ce8..341c344 100644 (file)
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -70,6 +70,8 @@ struct cls_rule {
 
 void cls_rule_init(struct cls_rule *,
                    const struct match *, unsigned int priority);
+void cls_rule_clone(struct cls_rule *, const struct cls_rule *);
+void cls_rule_destroy(struct cls_rule *);
 
 bool cls_rule_equal(const struct cls_rule *, const struct cls_rule *);
 uint32_t cls_rule_hash(const struct cls_rule *, uint32_t basis);

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 36de8e3..28384c7 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1901,6 +1901,7 @@ static void
 ofproto_rule_destroy__(struct rule *rule)
 {
     if (rule) {
+        cls_rule_destroy(&rule->cr);
         free(rule->ofpacts);
         rule->ofproto->ofproto_class->rule_dealloc(rule);
     }
@@ -2457,7 +2458,8 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t table_id,
         cls_cursor_init(&cursor, &table->cls, &cr);
         CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
             if (rule->pending) {
-                return OFPROTO_POSTPONE;
+                error = OFPROTO_POSTPONE;
+                goto exit;
             }
             if (!ofproto_rule_is_hidden(rule)
                 && ofproto_rule_has_out_port(rule, out_port)
@@ -2466,7 +2468,10 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t table_id,
             }
         }
     }
-    return 0;
+
+exit:
+    cls_rule_destroy(&cr);
+    return error;
 }
 
 /* Searches 'ofproto' for rules in table 'table_id' (or in all tables, if
@@ -2504,7 +2509,8 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t table_id,
                                                                &cr));
         if (rule) {
             if (rule->pending) {
-                return OFPROTO_POSTPONE;
+                error = OFPROTO_POSTPONE;
+                goto exit;
             }
             if (!ofproto_rule_is_hidden(rule)
                 && ofproto_rule_has_out_port(rule, out_port)
@@ -2513,6 +2519,9 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t table_id,
             }
         }
     }
+
+exit:
+    cls_rule_destroy(&cr);
     return 0;
 }
 
@@ -2922,6 +2931,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
 
     /* Serialize against pending deletion. */
     if (is_flow_deletion_pending(ofproto, &cr, table - ofproto->tables)) {
+        cls_rule_destroy(&rule->cr);
         ofproto->ofproto_class->rule_dealloc(rule);
         return OFPROTO_POSTPONE;
     }
@@ -2929,6 +2939,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
     /* Check for overlap, if requested. */
     if (fm->flags & OFPFF_CHECK_OVERLAP
         && classifier_rule_overlaps(&table->cls, &rule->cr)) {
+        cls_rule_destroy(&rule->cr);
         ofproto->ofproto_class->rule_dealloc(rule);
         return OFPERR_OFPFMFC_OVERLAP;
     }
@@ -3644,6 +3655,7 @@ ofproto_collect_ofmonitor_refresh_rules(const struct ofmonitor *m,
             ofproto_collect_ofmonitor_refresh_rule(m, rule, seqno, rules);
         }
     }
+    cls_rule_destroy(&target);
 }
 
 static void

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index f279bda..5e24dd2 100644 (file)
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -95,6 +95,11 @@ test_rule_from_cls_rule(const struct cls_rule *rule)
     return rule ? CONTAINER_OF(rule, struct test_rule, cls_rule) : NULL;
 }
 
+static struct test_rule *make_rule(int wc_fields, unsigned int priority,
+                                   int value_pat);
+static void free_rule(struct test_rule *);
+static struct test_rule *clone_rule(const struct test_rule *);
+
 /* Trivial (linear) classifier. */
 struct tcls {
     size_t n_rules;
@@ -138,8 +143,8 @@ tcls_insert(struct tcls *tcls, const struct test_rule *rule)
         const struct cls_rule *pos = &tcls->rules[i]->cls_rule;
         if (cls_rule_equal(pos, &rule->cls_rule)) {
             /* Exact match. */
-            free(tcls->rules[i]);
-            tcls->rules[i] = xmemdup(rule, sizeof *rule);
+            free_rule(tcls->rules[i]);
+            tcls->rules[i] = clone_rule(rule);
             return tcls->rules[i];
         } else if (pos->priority < rule->cls_rule.priority) {
             break;
@@ -154,7 +159,7 @@ tcls_insert(struct tcls *tcls, const struct test_rule *rule)
         memmove(&tcls->rules[i + 1], &tcls->rules[i],
                 sizeof *tcls->rules * (tcls->n_rules - i));
     }
-    tcls->rules[i] = xmemdup(rule, sizeof *rule);
+    tcls->rules[i] = clone_rule(rule);
     tcls->n_rules++;
     return tcls->rules[i];
 }
@@ -422,7 +427,7 @@ destroy_classifier(struct classifier *cls)
     cls_cursor_init(&cursor, cls, NULL);
     CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
         classifier_remove(cls, &rule->cls_rule);
-        free(rule);
+        free_rule(rule);
     }
     classifier_destroy(cls);
 }
@@ -522,6 +527,24 @@ make_rule(int wc_fields, unsigned int priority, int value_pat)
     return rule;
 }
 
+static struct test_rule *
+clone_rule(const struct test_rule *src)
+{
+    struct test_rule *dst;
+
+    dst = xmalloc(sizeof *dst);
+    dst->aux = src->aux;
+    cls_rule_clone(&dst->cls_rule, &src->cls_rule);
+    return dst;
+}
+
+static void
+free_rule(struct test_rule *rule)
+{
+    cls_rule_destroy(&rule->cls_rule);
+    free(rule);
+}
+
 static void
 shuffle(unsigned int *p, size_t n)
 {
@@ -584,7 +607,7 @@ test_single_rule(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         assert(tcls_is_empty(&tcls));
         compare_classifiers(&cls, &tcls);
 
-        free(rule);
+        free_rule(rule);
         classifier_destroy(&cls);
         tcls_destroy(&tcls);
     }
@@ -619,7 +642,7 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         tcls_insert(&tcls, rule2);
         assert(test_rule_from_cls_rule(
                    classifier_replace(&cls, &rule2->cls_rule)) == rule1);
-        free(rule1);
+        free_rule(rule1);
         check_tables(&cls, 1, 1, 0);
         compare_classifiers(&cls, &tcls);
         tcls_destroy(&tcls);
@@ -760,7 +783,7 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
             tcls_destroy(&tcls);
 
             for (i = 0; i < N_RULES; i++) {
-                free(rules[i]);
+                free_rule(rules[i]);
             }
         } while (next_permutation(ops, ARRAY_SIZE(ops)));
         assert(n_permutations == (factorial(N_RULES * 2) >> N_RULES));
@@ -838,7 +861,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         for (i = 0; i < N_RULES; i++) {
             tcls_remove(&tcls, tcls_rules[i]);
             classifier_remove(&cls, &rules[i]->cls_rule);
-            free(rules[i]);
+            free_rule(rules[i]);
 
             check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0);
             compare_classifiers(&cls, &tcls);
@@ -897,18 +920,17 @@ test_many_rules_in_n_tables(int n_tables)
             struct test_rule *target;
             struct cls_cursor cursor;
 
-            target = xmemdup(tcls.rules[rand() % tcls.n_rules],
-                             sizeof(struct test_rule));
+            target = clone_rule(tcls.rules[rand() % tcls.n_rules]);
 
             cls_cursor_init(&cursor, &cls, &target->cls_rule);
             CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
                 classifier_remove(&cls, &rule->cls_rule);
-                free(rule);
+                free_rule(rule);
             }
             tcls_delete_matches(&tcls, &target->cls_rule);
             compare_classifiers(&cls, &tcls);
             check_tables(&cls, -1, -1, -1);
-            free(target);
+            free_rule(target);
         }
 
         destroy_classifier(&cls);

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index c7069c5..2d98273 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1778,6 +1778,7 @@ fte_free(struct fte *fte)
     if (fte) {
         fte_version_free(fte->versions[0]);
         fte_version_free(fte->versions[1]);
+        cls_rule_destroy(&fte->rule);
         free(fte);
     }
 }
@@ -1816,6 +1817,7 @@ fte_insert(struct classifier *cls, const struct match *match,
     if (old) {
         fte_version_free(old->versions[index]);
         fte->versions[!index] = old->versions[!index];
+        cls_rule_destroy(&old->rule);
         free(old);
     }
 }