From 48d28ac16112f72ef0985ec2d013425202af8f5c Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 20 Aug 2012 11:29:43 -0700 Subject: [PATCH] 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 --- lib/classifier.c | 30 ++++++++++++++++++++++++--- lib/classifier.h | 2 ++ ofproto/ofproto.c | 18 +++++++++++++--- tests/test-classifier.c | 46 ++++++++++++++++++++++++++++++----------- utilities/ovs-ofctl.c | 2 ++ 5 files changed, 80 insertions(+), 18 deletions(-) diff --git a/lib/classifier.c b/lib/classifier.c index 919e05fc..f6f7a64d 100644 --- 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 3370ce8b..341c3441 100644 --- 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 36de8e3a..28384c7b 100644 --- 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 f279bda1..5e24dd22 100644 --- 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 c7069c5b..2d982731 100644 --- 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); } } -- 2.30.2