[ovs-dev] [wdp] classifier: Fix classifier bugs.

[Ben Pfaff]

The classifier tests have been failing on the 'wdp' branch for a long time
now, ever since commit 8980c78 "ofproto: Start work to enable datapaths
with built-in wildcard support."  I finally got around to finding the
cause, which is that the classifier was hashing and comparing the
'priority' member of exact-match flows.  That member is basically
meaningless for such flows, so this commit fixes the problem by changing
the functions that hash and compare flows to ignore it.  They also now
ignore the 'wildcards' field.  These functions' callers already assumed
these semantics, so there is no change to the classifier itself.  The
classifier tests, on the other hand, were not treating the priority of
exact-match flows in the same way as the classifier itself, so this commit
changes the tests' behavior to match.
---
 lib/flow.h              |   22 ++++++++++++++++++++--
 tests/test-classifier.c |   12 +++++++++---
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index 82d6ae0..2ccb2f2 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -77,22 +77,40 @@ static inline int flow_compare(const flow_t *, const flow_t *);
 static inline bool flow_equal(const flow_t *, const flow_t *);
 static inline size_t flow_hash(const flow_t *, uint32_t basis);
 
+/* Compares members of 'a' and 'b' except for 'wildcards' and 'priority' and
+ * returns a strcmp()-like return value. */
 static inline int
 flow_compare(const flow_t *a, const flow_t *b)
 {
-    return memcmp(a, b, FLOW_SIG_SIZE);
+    /* Assert that 'wildcards' and 'priority' are leading 32-bit fields. */
+    BUILD_ASSERT_DECL(offsetof(struct flow, wildcards) == 0);
+    BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->wildcards) == 4);
+    BUILD_ASSERT_DECL(offsetof(struct flow, priority) == 4);
+    BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->priority) == 4);
+
+    return memcmp((char *) a + 8, (char *) b + 8, FLOW_SIG_SIZE - 8);
 }
 
+/* Returns true if all members of 'a' and 'b' are equal except for 'wildcards'
+ * and 'priority', false otherwise. */
 static inline bool
 flow_equal(const flow_t *a, const flow_t *b)
 {
     return !flow_compare(a, b);
 }
 
+/* Returns a hash value for 'flow' that does not include 'wildcards' or
+ * 'priority', folding 'basis' into the hash value. */
 static inline size_t
 flow_hash(const flow_t *flow, uint32_t basis)
 {
-    return hash_bytes(flow, FLOW_SIG_SIZE, basis);
+    /* Assert that 'wildcards' and 'priority' are leading 32-bit fields. */
+    BUILD_ASSERT_DECL(offsetof(struct flow, wildcards) == 0);
+    BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->wildcards) == 4);
+    BUILD_ASSERT_DECL(offsetof(struct flow, priority) == 4);
+    BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->priority) == 4);
+
+    return hash_bytes((char *) flow + 8, FLOW_SIG_SIZE - 8, basis);
 }
 
 /* Information on wildcards for a flow, as a supplement to flow_t. */
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index a671a6a..a2f6817 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -95,15 +95,21 @@ tcls_is_empty(const struct tcls *tcls)
     return tcls->n_rules == 0;
 }
 
+static unsigned int
+effective_priority(const flow_t *flow)
+{
+    return flow->wildcards ? flow->priority : MAX(flow->priority, UINT16_MAX);
+}
+
 static struct test_rule *
 tcls_insert(struct tcls *tcls, const struct test_rule *rule)
 {
+    unsigned int priority = effective_priority(&rule->cls_rule.flow);
     size_t i;
 
-    assert(rule->cls_rule.flow.wildcards || rule->cls_rule.flow.priority == UINT_MAX);
     for (i = 0; i < tcls->n_rules; i++) {
         const struct cls_rule *pos = &tcls->rules[i]->cls_rule;
-        if (pos->flow.priority == rule->cls_rule.flow.priority
+        if (pos->flow.priority == priority
             && pos->flow.wildcards == rule->cls_rule.flow.wildcards
             && flow_equal(&pos->flow, &rule->cls_rule.flow)) {
             /* Exact match.
@@ -111,7 +117,7 @@ tcls_insert(struct tcls *tcls, const struct test_rule *rule)
             free(tcls->rules[i]);
             tcls->rules[i] = xmemdup(rule, sizeof *rule);
             return tcls->rules[i];
-        } else if (pos->flow.priority < rule->cls_rule.flow.priority) {
+        } else if (pos->flow.priority < priority) {
             break;
         }
     }
-- 
1.7.1