[ovs-dev] [PATCH v7 2/7] ovn: add is_chassis_resident match expression component

Ben Pfaff blp at ovn.org
Fri Jan 6 23:14:00 UTC 2017


On Fri, Jan 06, 2017 at 12:00:29PM -0800, Mickey Spiegel wrote:
> This patch introduces a new match expression component
> is_chassis_resident().  Unlike match expression comparisons,
> is_chassis_resident is not pushed down to OpenFlow.  It is a
> conditional that is evaluated in the controller during expr_simplify(),
> when it is replaced by a boolean expression.  The is_chassis_resident
> conditional evaluates to "true" when the specified string identifies a
> port name that is resident on this controller chassis, i.e., the
> corresponding southbound database Port_Binding has a chassis column
> that matches this chassis.  Otherwise it evaluates to "false".
> 
> This allows higher level features to specify flows that are only
> installed on some chassis rather than on all chassis with the
> corresponding datapath.
> 
> Suggested-by: Ben Pfaff <blp at ovn.org>
> Signed-off-by: Mickey Spiegel <mickeys.dev at gmail.com>

Here's an incremental I suggest folding in.  Some of the differences are
style, but I also added documentation and changed the details of the
expr_simplify_condition() interface.

I suggest calling these "functions" instead of "conditions", but I
didn't make that change, except in the documentation.

One change I made is that is_chassis_resident is considered to be a
function name only if it is followed by a left parenthesis.  In fact, I
think that this is a good marker of a function invocation in any case,
so I generalized it so that any identifier followed by a left
parenthesis has to be a function invocation otherwise a
function-specific syntax error is given.

Acked-by: Ben Pfaff <blp at ovn.org>


--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index c0655c3..3d7633e 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -97,11 +97,7 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
 
     const struct sbrec_port_binding *pb
         = lport_lookup_by_name(c_aux->lports, port_name);
-    if (pb && pb->chassis && pb->chassis == c_aux->chassis) {
-        return true;
-    }
-
-    return false;
+    return pb && pb->chassis && pb->chassis == c_aux->chassis;
 }
 
 static bool
@@ -239,11 +235,6 @@ consider_logical_flow(const struct lport_index *lports,
     struct hmap matches;
     struct expr *expr;
 
-    struct condition_aux cond_aux = {
-        .lports = lports,
-        .chassis = chassis
-    };
-
     expr = expr_parse_string(lflow->match, &symtab, addr_sets, &error);
     if (!error) {
         if (prereqs) {
@@ -262,6 +253,7 @@ consider_logical_flow(const struct lport_index *lports,
         return;
     }
 
+    struct condition_aux cond_aux = { lports, chassis };
     expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
     expr = expr_normalize(expr);
     uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 95430e6..d57fac2 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1002,9 +1002,6 @@ expr_addr_sets_destroy(struct shash *addr_sets)
 static struct expr *
 parse_chassis_resident(struct expr_context *ctx)
 {
-    if (!lexer_force_match(ctx->lexer, LEX_T_LPAREN)) {
-        return NULL;
-    }
     if (ctx->lexer->token.type != LEX_T_STRING) {
         lexer_syntax_error(ctx->lexer, "expecting string");
         return NULL;
@@ -1044,9 +1041,14 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic)
         enum expr_relop r;
         struct expr_constant_set c;
 
-        if (lexer_match_id(ctx->lexer, "is_chassis_resident")) {
-            *atomic = true;
-            return parse_chassis_resident(ctx);
+        if (lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
+            if (lexer_match_id(ctx->lexer, "is_chassis_resident")) {
+                lexer_get(ctx->lexer); /* Skip "(". */
+                *atomic = true;
+                return parse_chassis_resident(ctx);
+            }
+            lexer_error(ctx->lexer, "parsing function name");
+            return NULL;
         }
 
         if (!parse_field(ctx, &f)) {
@@ -1865,26 +1867,20 @@ expr_simplify_condition(struct expr *expr,
                                                     const char *port_name),
                         const void *c_aux)
 {
-    bool cond_initial_result;
+    bool result;
+
     switch (expr->cond.type) {
     case EXPR_COND_CHASSIS_RESIDENT:
-        if (!is_chassis_resident || !c_aux) {
-            /* For test cases where chassis is not considered. */
-            cond_initial_result = true;
-        } else {
-            cond_initial_result =
-                                 is_chassis_resident(c_aux, expr->cond.string);
-        }
+        result = is_chassis_resident(c_aux, expr->cond.string);
         break;
 
     default:
         OVS_NOT_REACHED();
     }
-    if (expr->cond.not) {
-        cond_initial_result = !cond_initial_result;
-    }
+
+    result ^= expr->cond.not;
     expr_destroy(expr);
-    return expr_create_boolean(cond_initial_result);
+    return expr_create_boolean(result);
 }
 
 /* Takes ownership of 'expr' and returns an equivalent expression whose
@@ -3153,6 +3149,15 @@ expr_resolve_field(const struct expr_field *f)
     return (struct mf_subfield) { symbol->field, ofs, n_bits };
 }
 
+static bool
+microflow_is_chassis_resident_cb(const void *c_aux OVS_UNUSED,
+                                 const char *port_name OVS_UNUSED)
+{
+    /* Assume tests calling expr_parse_microflow are not chassis specific, so
+     * is_chassis_resident need not be supplied and should return true. */
+    return true;
+}
+
 static struct expr *
 expr_parse_microflow__(struct lexer *lexer,
                        const struct shash *symtab,
@@ -3173,9 +3178,7 @@ expr_parse_microflow__(struct lexer *lexer,
     struct ds annotated = DS_EMPTY_INITIALIZER;
     expr_format(e, &annotated);
 
-    /* Assume tests calling expr_parse_microflow are not chassis specific, so
-     * is_chassis_resident need not be supplied and should return true. */
-    e = expr_simplify(e, NULL, NULL);
+    e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL);
     e = expr_normalize(e);
 
     struct match m = MATCH_CATCHALL_INITIALIZER;
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 2f35079..f78f040 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -534,6 +534,20 @@
         packet.
       </p>
 
+      <p>
+        Match expressions also support a kind of function syntax.  The
+        following functions are supported:
+      </p>
+
+      <dl>
+        <dt><code>is_chassis_resident(<var>lport</var>)</code></dt>
+        <dd>
+          Evaluates to true on a chassis on which logical port <var>lport</var>
+          (a quoted string) resides, and to false elsewhere.  This function was
+          introduced in OVN 2.7.
+        </dd>
+      </dl>
+
       <p><em>Symbols</em></p>
 
       <p>
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 7106bed..149471c 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -586,7 +586,7 @@ read_address_sets(void)
 }
 
 static bool
-ovntrace_is_chassis_resident(const void *ports_ OVS_UNUSED,
+ovntrace_is_chassis_resident(const void *aux OVS_UNUSED,
                              const char *port_name)
 {
     if (port_name[0] == '\0') {
@@ -681,7 +681,7 @@ read_flows(void)
             continue;
         }
         if (match) {
-            match = expr_simplify(match, ovntrace_is_chassis_resident, &ports);
+            match = expr_simplify(match, ovntrace_is_chassis_resident, NULL);
         }
 
         struct ovntrace_flow *flow = xzalloc(sizeof *flow);
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index daec9ac..e64f12f 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -803,6 +803,13 @@ free_rule(struct test_rule *test_rule)
     free(test_rule);
 }
 
+static bool
+tree_shape_is_chassis_resident_cb(const void *c_aux OVS_UNUSED,
+                                  const char *port_name OVS_UNUSED)
+{
+    return true;
+}
+
 static int
 test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
                              struct expr *terminals[], int n_terminals,
@@ -849,7 +856,9 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
                 exit(EXIT_FAILURE);
             }
         } else if (operation >= OP_SIMPLIFY) {
-            modified  = expr_simplify(expr_clone(expr), NULL, NULL);
+            modified = expr_simplify(expr_clone(expr),
+                                     tree_shape_is_chassis_resident_cb,
+                                     NULL);
             ovs_assert(expr_honors_invariants(modified));
 
             if (operation >= OP_NORMALIZE) {



More information about the dev mailing list