classifier: Refactor interface for classifier_remove().

Until now, classifier_remove() returned either null or the classifier rule
passed to it, which is an unusual interface.  This commit changes it to
return true if it succeeds or false on failure.

In addition, most of classifier_remove()'s callers know ahead of time that
it must succeed, even though most of them didn't bother with an assertion,
so this commit adds a classifier_remove_assert() function as a helper.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

8 files changed, 43 insertions, 46 deletions

diff --git a/lib/classifier.c b/lib/classifier.c
index 16c451da1..532f1662d 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -695,15 +695,16 @@ classifier_insert(struct classifier *cls, const struct cls_rule *rule,
     ovs_assert(!displaced_rule);
 }
 
-/* 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.
+/* If 'rule' is in 'cls', removes 'rule' from 'cls' and returns true.  It is
+ * the caller's responsibility to destroy 'rule' with cls_rule_destroy(),
+ * freeing the memory block in which 'rule' resides, etc., as necessary.
  *
- * Does nothing if 'rule' has been already removed, or was never inserted.
+ * If 'rule' is not in any classifier, returns false without making any
+ * changes.
  *
- * Returns the removed rule, or NULL, if it was already removed.
+ * 'rule' must not be in some classifier other than 'cls'.
  */
-const struct cls_rule *
+bool
 classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
 {
     struct cls_match *rule, *prev, *next, *head;
@@ -716,7 +717,7 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
 
     rule = get_cls_match_protected(cls_rule);
     if (!rule) {
-        return NULL;
+        return false;
     }
     /* Mark as removed. */
     ovsrcu_set(&CONST_CAST(struct cls_rule *, cls_rule)->cls_match, NULL);
@@ -820,7 +821,14 @@ check_priority:
     ovsrcu_postpone(cls_match_free_cb, rule);
     cls->n_rules--;
 
-    return cls_rule;
+    return true;
+}
+
+void
+classifier_remove_assert(struct classifier *cls,
+                         const struct cls_rule *cls_rule)
+{
+    ovs_assert(classifier_remove(cls, cls_rule));
 }
 
 /* Prefix tree context.  Valid when 'lookup_done' is true.  Can skip all
diff --git a/lib/classifier.h b/lib/classifier.h
index 71c2e507d..31d4a1b08 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -387,8 +387,8 @@ const struct cls_rule *classifier_replace(struct classifier *,
                                           ovs_version_t,
                                           const struct cls_conjunction *,
                                           size_t n_conjunctions);
-const struct cls_rule *classifier_remove(struct classifier *,
-                                         const struct cls_rule *);
+bool classifier_remove(struct classifier *, const struct cls_rule *);
+void classifier_remove_assert(struct classifier *, const struct cls_rule *);
 static inline void classifier_defer(struct classifier *);
 static inline void classifier_publish(struct classifier *);
 
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index cd2ab15fb..e6cc81fd0 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -245,19 +245,14 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
     ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw);
 }
 
-static bool
-__rt_entry_delete(const struct cls_rule *cr)
+static void
+rt_entry_delete__(const struct cls_rule *cr)
 {
     struct ovs_router_entry *p = ovs_router_entry_cast(cr);
 
     tnl_port_map_delete_ipdev(p->output_bridge);
-    /* Remove it. */
-    cr = classifier_remove(&cls, cr);
-    if (cr) {
-        ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
-        return true;
-    }
-    return false;
+    classifier_remove_assert(&cls, cr);
+    ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
 }
 
 static bool
@@ -277,8 +272,10 @@ rt_entry_delete(uint32_t mark, uint8_t priority,
     cr = classifier_find_rule_exactly(&cls, &rule, OVS_VERSION_MAX);
     if (cr) {
         ovs_mutex_lock(&mutex);
-        res = __rt_entry_delete(cr);
+        rt_entry_delete__(cr);
         ovs_mutex_unlock(&mutex);
+
+        res = true;
     }
 
     cls_rule_destroy(&rule);
@@ -476,7 +473,7 @@ ovs_router_flush(void)
     classifier_defer(&cls);
     CLS_FOR_EACH(rt, cr, &cls) {
         if (rt->priority == rt->plen) {
-            __rt_entry_delete(&rt->cr);
+            rt_entry_delete__(&rt->cr);
         }
     }
     classifier_publish(&cls);
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index 04d2b3f7c..b814f7a0a 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -223,9 +223,8 @@ tnl_port_unref(const struct cls_rule *cr)
     struct tnl_port_in *p = tnl_port_cast(cr);
 
     if (cr && ovs_refcount_unref_relaxed(&p->ref_cnt) == 1) {
-        if (classifier_remove(&cls, cr)) {
-            ovsrcu_postpone(tnl_port_free, p);
-        }
+        classifier_remove_assert(&cls, cr);
+        ovsrcu_postpone(tnl_port_free, p);
     }
 }
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 4f17f79d2..536636393 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1520,10 +1520,8 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
         /* Make sure there is no postponed removal of the rule. */
         ovs_assert(cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
 
-        if (!classifier_remove(&rule->ofproto->tables[rule->table_id].cls,
-                               &rule->cr)) {
-            OVS_NOT_REACHED();
-        }
+        classifier_remove_assert(&rule->ofproto->tables[rule->table_id].cls,
+                                 &rule->cr);
         ofproto_rule_remove__(rule->ofproto, rule);
         if (ofproto->ofproto_class->rule_delete) {
             ofproto->ofproto_class->rule_delete(rule);
@@ -2885,9 +2883,7 @@ remove_rule_rcu__(struct rule *rule)
     struct oftable *table = &ofproto->tables[rule->table_id];
 
     ovs_assert(!cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
-    if (!classifier_remove(&table->cls, &rule->cr)) {
-        OVS_NOT_REACHED();
-    }
+    classifier_remove_assert(&table->cls, &rule->cr);
     if (ofproto->ofproto_class->rule_delete) {
         ofproto->ofproto_class->rule_delete(rule);
     }
@@ -5231,9 +5227,7 @@ replace_rule_revert(struct ofproto *ofproto,
     }
 
     /* Remove the new rule immediately.  It was never visible to lookups. */
-    if (!classifier_remove(&table->cls, &new_rule->cr)) {
-        OVS_NOT_REACHED();
-    }
+    classifier_remove_assert(&table->cls, &new_rule->cr);
     ofproto_rule_remove__(ofproto, new_rule);
     ofproto_rule_unref(new_rule);
 }
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index edaf4a393..ef6693e61 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -466,9 +466,8 @@ destroy_classifier(struct classifier *cls)
 
     classifier_defer(cls);
     CLS_FOR_EACH (rule, cls_rule, cls) {
-        if (classifier_remove(cls, &rule->cls_rule)) {
-            ovsrcu_postpone(free_rule, rule);
-        }
+        classifier_remove_assert(cls, &rule->cls_rule);
+        ovsrcu_postpone(free_rule, rule);
     }
     classifier_destroy(cls);
 }
@@ -816,7 +815,7 @@ test_single_rule(struct ovs_cmdl_context *ctx OVS_UNUSED)
         compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
         check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);
 
-        classifier_remove(&cls, &rule->cls_rule);
+        classifier_remove_assert(&cls, &rule->cls_rule);
         tcls_remove(&tcls, tcls_rule);
         assert(classifier_is_empty(&cls));
         assert(tcls_is_empty(&tcls));
@@ -864,7 +863,7 @@ test_rule_replacement(struct ovs_cmdl_context *ctx OVS_UNUSED)
         compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
         check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);
         classifier_defer(&cls);
-        classifier_remove(&cls, &rule2->cls_rule);
+        classifier_remove_assert(&cls, &rule2->cls_rule);
 
         tcls_destroy(&tcls);
         destroy_classifier(&cls);
@@ -1018,7 +1017,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
                         n_invisible_rules++;
                         removable_rule = &rules[j]->cls_rule;
                     } else {
-                        classifier_remove(&cls, &rules[j]->cls_rule);
+                        classifier_remove_assert(&cls, &rules[j]->cls_rule);
                     }
                     tcls_remove(&tcls, tcls_rules[j]);
                     tcls_rules[j] = NULL;
@@ -1039,7 +1038,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
                     /* Removable rule is no longer visible. */
                     assert(cls_match);
                     assert(!cls_match_visible_in_version(cls_match, version));
-                    classifier_remove(&cls, removable_rule);
+                    classifier_remove_assert(&cls, removable_rule);
                     n_invisible_rules--;
                 }
             }
@@ -1139,7 +1138,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)
                                                    version);
                 n_invisible_rules++;
             } else {
-                classifier_remove(&cls, &rules[i]->cls_rule);
+                classifier_remove_assert(&cls, &rules[i]->cls_rule);
             }
             compare_classifiers(&cls, n_invisible_rules, version, &tcls);
             check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0,
@@ -1151,7 +1150,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
         if (versioned) {
             for (i = 0; i < N_RULES; i++) {
-                classifier_remove(&cls, &rules[i]->cls_rule);
+                classifier_remove_assert(&cls, &rules[i]->cls_rule);
                 n_invisible_rules--;
 
                 compare_classifiers(&cls, n_invisible_rules, version, &tcls);
@@ -1250,7 +1249,7 @@ test_many_rules_in_n_tables(int n_tables)
 
             /* Remove rules that are no longer visible. */
             LIST_FOR_EACH_POP (rule, list_node, &list) {
-                classifier_remove(&cls, &rule->cls_rule);
+                classifier_remove_assert(&cls, &rule->cls_rule);
                 n_invisible_rules--;
 
                 compare_classifiers(&cls, n_invisible_rules, version,
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 539e778e1..85763718d 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -972,7 +972,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
             struct test_rule *test_rule;
 
             CLS_FOR_EACH (test_rule, cr, &cls) {
-                classifier_remove(&cls, &test_rule->cr);
+                classifier_remove_assert(&cls, &test_rule->cr);
                 ovsrcu_postpone(free_rule, test_rule);
             }
             classifier_destroy(&cls);
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3239d6fcb..e1e4d37ed 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -3220,7 +3220,7 @@ fte_free_all(struct flow_tables *tables)
 
         classifier_defer(cls);
         CLS_FOR_EACH (fte, rule, cls) {
-            classifier_remove(cls, &fte->rule);
+            classifier_remove_assert(cls, &fte->rule);
             ovsrcu_postpone(fte_free, fte);
         }
         classifier_destroy(cls);