[ovs-dev] [PATCH ovn v3 08/10] expr: Make expr_to_flows() include conj_id flows.

[Ben Pfaff]

When I wrote expr_to_flows() originally, I assumed that the caller could
simply add an appropriate conj_id=X flow for each of the conjunctive
matches.  I forgot that the conj_id=X flows also need to include
prerequisites for actions, e.g. if the OpenFlow actions manipulate TCP
fields, then the conj_id=X field must match on eth_type=0x800 and
ip_proto=6.  That's hard for the caller to generate itself, so this commit
changes expr_to_matches() to generate the conj_id=X flows also.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ovn/lib/expr.c   |   18 ++++++++++++++++++
 ovn/lib/expr.h   |    4 ++++
 tests/test-ovn.c |   12 +-----------
 3 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index fb9b1cf..a1275a3 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -2242,6 +2242,10 @@ add_conjunction(const struct expr *and, const struct simap *ports,
                 }
             }
         }
+
+        /* Add the flow that matches on conj_id. */
+        match_set_conj_id(&match, *n_conjsp);
+        expr_match_add(matches, expr_match_new(&match, 0, 0, 0));
     }
 }
 
@@ -2264,6 +2268,20 @@ add_cmp_flow(const struct expr *cmp, const struct simap *ports,
  * conjunctive match IDs beginning with 0; the caller must offset or remap them
  * into the desired range as necessary.
  *
+ * The matches inserted into 'matches' will be of three distinct kinds:
+ *
+ *     - Ordinary flows.  The caller should add these OpenFlow flows with
+ *       its desired actions.
+ *
+ *     - Conjunctive flows, distinguished by 'n > 0' in the expr_match
+ *       structure.  The caller should add these OpenFlow flows with the
+ *       conjunction(id, k/n) actions as specified in the 'conjunctions' array,
+ *       remapping the ids.
+ *
+ *     - conj_id flows, distinguished by matching on the "conj_id" field.  The
+ *       caller should remap the conj_id and add the OpenFlow flow with its
+ *       desired actions.
+ *
  * 'ports' must be a map from strings (presumably names of ports) to integers.
  * Any comparisons against string fields in 'expr' are translated into integers
  * through this map.  A comparison against a string that is not in 'ports' acts
diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
index 54cec46..3540708 100644
--- a/ovn/lib/expr.h
+++ b/ovn/lib/expr.h
@@ -355,7 +355,11 @@ struct expr *expr_normalize(struct expr *);
 bool expr_honors_invariants(const struct expr *);
 bool expr_is_simplified(const struct expr *);
 bool expr_is_normalized(const struct expr *);
+
+/* Converting expressions to OpenFlow flows. */
 
+/* An OpenFlow match generated from a Boolean expression.  See
+ * expr_to_matches() for more information. */
 struct expr_match {
     struct hmap_node hmap_node;
     struct match match;
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 67093d5..b6fdb0c 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -880,9 +880,8 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
         if (operation >= OP_FLOW) {
             struct expr_match *m;
             struct test_rule *test_rule;
-            uint32_t n_conjs;
 
-            n_conjs = expr_to_matches(modified, NULL, &matches);
+            expr_to_matches(modified, NULL, &matches);
 
             classifier_init(&cls, NULL);
             HMAP_FOR_EACH (m, hmap_node, &matches) {
@@ -890,15 +889,6 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
                 cls_rule_init(&test_rule->cr, &m->match, 0);
                 classifier_insert(&cls, &test_rule->cr, m->conjunctions, m->n);
             }
-            for (uint32_t conj_id = 1; conj_id <= n_conjs; conj_id++) {
-                struct match match;
-                match_init_catchall(&match);
-                match_set_conj_id(&match, conj_id);
-
-                test_rule = xmalloc(sizeof *test_rule);
-                cls_rule_init(&test_rule->cr, &match, 0);
-                classifier_insert(&cls, &test_rule->cr, NULL, 0);
-            }
         }
         for (int subst = 0; subst < 1 << (n_bits * n_vars); subst++) {
             bool expected = evaluate_expr(expr, subst, n_bits);
-- 
1.7.10.4