ovn: Use callback function instead of simap for logical port number map.

An simap is convenient but it isn't very flexible.  If the client wants to
keep extra data with each node then it has to build a second parallel data
structure.  A callback function is kind of a pain for the clients from the
point of view of having to write it and deal with auxiliary data, etc., but
it allows the storage to be more flexible.

An upcoming commit will make further use of this capability.

Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Justin Pettit <[email protected]>

ovn/controller/lflow.c

@@ -273,6 +273,18 @@ lflow_init(void)
     symtab_init();
 }
 
+static bool
+lookup_port_cb(const void *ldp_, const char *port_name, unsigned int *portp)
+{
+    const struct logical_datapath *ldp = ldp_;
+    const struct simap_node *node = simap_find(&ldp->ports, port_name);
+    if (!node) {
+        return false;
+    }
+    *portp = node->data;
+    return true;
+}
+
 /* Translates logical flows in the Logical_Flow table in the OVN_SB database
  * into OpenFlow flows.  See ovn-architecture(7) for more information. */
 void
@@ -348,7 +360,8 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
         ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
         struct action_params ap = {
             .symtab = &symtab,
-            .ports = &ldp->ports,
+            .lookup_port = lookup_port_cb,
+            .aux = ldp,
             .ct_zones = ct_zones,
 
             .n_tables = LOG_PIPELINE_LEN,
@@ -389,7 +402,8 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
 
         expr = expr_simplify(expr);
         expr = expr_normalize(expr);
-        uint32_t n_conjs = expr_to_matches(expr, &ldp->ports, &matches);
+        uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, ldp,
+                                           &matches);
         expr_destroy(expr);
 
         /* Prepare the OpenFlow matches for adding to the flow table. */

ovn/lib/actions.c

@@ -108,7 +108,8 @@ parse_set_action(struct action_context *ctx)
     struct expr *prereqs;
     char *error;
 
-    error = expr_parse_assignment(ctx->lexer, ctx->ap->symtab, ctx->ap->ports,
+    error = expr_parse_assignment(ctx->lexer, ctx->ap->symtab,
+                                  ctx->ap->lookup_port, ctx->ap->aux,
                                   ctx->ofpacts, &prereqs);
     if (error) {
         action_error(ctx, "%s", error);

ovn/lib/actions.h

@@ -17,6 +17,7 @@
 #ifndef OVN_ACTIONS_H
 #define OVN_ACTIONS_H 1
 
+#include <stdbool.h>
 #include <stdint.h>
 #include "compiler.h"
 #include "util.h"
@@ -47,10 +48,11 @@ struct action_params {
      * expr_parse()). */
     const struct shash *symtab;
 
-     /* 'ports' must be a map from strings (presumably names of ports) to
-      * integers (as one would provide to expr_to_matches()).  Strings used in
-      * the actions that are not in 'ports' are translated to zero. */
-    const struct simap *ports;
+    /* Looks up logical port 'port_name'.  If found, stores its port number in
+     * '*portp' and returns true; otherwise, returns false. */
+    bool (*lookup_port)(const void *aux, const char *port_name,
+                        unsigned int *portp);
+    const void *aux;
 
     /* A map from a port name to its connection tracking zone. */
     const struct simap *ct_zones;

ovn/lib/expr.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015 Nicira, Inc.
+ * Copyright (c) 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -2287,17 +2287,18 @@ expr_match_add(struct hmap *matches, struct expr_match *match)
 }
 
 static bool
-constrain_match(const struct expr *expr, const struct simap *ports,
-                struct match *m)
+constrain_match(const struct expr *expr,
+                bool (*lookup_port)(const void *aux, const char *port_name,
+                                    unsigned int *portp),
+                const void *aux, struct match *m)
 {
     ovs_assert(expr->type == EXPR_T_CMP);
     if (expr->cmp.symbol->width) {
         mf_mask_subfield(expr->cmp.symbol->field, &expr->cmp.value,
                          &expr->cmp.mask, m);
     } else {
-        const struct simap_node *node;
-        node = ports ? simap_find(ports, expr->cmp.string) : NULL;
-        if (!node) {
+        unsigned int port;
+        if (!lookup_port(aux, expr->cmp.string, &port)) {
             return false;
         }
 
@@ -2308,15 +2309,18 @@ constrain_match(const struct expr *expr, const struct simap *ports,
 
         union mf_subvalue x;
         memset(&x, 0, sizeof x);
-        x.integer = htonll(node->data);
+        x.integer = htonll(port);
 
         mf_write_subfield(&sf, &x, m);
     }
     return true;
 }
 
 static bool
-add_disjunction(const struct expr *or, const struct simap *ports,
+add_disjunction(const struct expr *or,
+                bool (*lookup_port)(const void *aux, const char *port_name,
+                                    unsigned int *portp),
+                const void *aux,
                 struct match *m, uint8_t clause, uint8_t n_clauses,
                 uint32_t conj_id, struct hmap *matches)
 {
@@ -2327,7 +2331,7 @@ add_disjunction(const struct expr *or, const struct simap *ports,
     LIST_FOR_EACH (sub, node, &or->andor) {
         struct expr_match *match = expr_match_new(m, clause, n_clauses,
                                                   conj_id);
-        if (constrain_match(sub, ports, &match->match)) {
+        if (constrain_match(sub, lookup_port, aux, &match->match)) {
             expr_match_add(matches, match);
             n++;
         } else {
@@ -2342,8 +2346,10 @@ add_disjunction(const struct expr *or, const struct simap *ports,
 }
 
 static void
-add_conjunction(const struct expr *and, const struct simap *ports,
-                uint32_t *n_conjsp, struct hmap *matches)
+add_conjunction(const struct expr *and,
+                bool (*lookup_port)(const void *aux, const char *port_name,
+                                    unsigned int *portp),
+                const void *aux, uint32_t *n_conjsp, struct hmap *matches)
 {
     struct match match;
     int n_clauses = 0;
@@ -2355,7 +2361,7 @@ add_conjunction(const struct expr *and, const struct simap *ports,
     LIST_FOR_EACH (sub, node, &and->andor) {
         switch (sub->type) {
         case EXPR_T_CMP:
-            if (!constrain_match(sub, ports, &match)) {
+            if (!constrain_match(sub, lookup_port, aux, &match)) {
                 return;
             }
             break;
@@ -2373,15 +2379,16 @@ add_conjunction(const struct expr *and, const struct simap *ports,
     } else if (n_clauses == 1) {
         LIST_FOR_EACH (sub, node, &and->andor) {
             if (sub->type == EXPR_T_OR) {
-                add_disjunction(sub, ports, &match, 0, 0, 0, matches);
+                add_disjunction(sub, lookup_port, aux, &match, 0, 0, 0,
+                                matches);
             }
         }
     } else {
         int clause = 0;
         (*n_conjsp)++;
         LIST_FOR_EACH (sub, node, &and->andor) {
             if (sub->type == EXPR_T_OR) {
-                if (!add_disjunction(sub, ports, &match, clause++,
+                if (!add_disjunction(sub, lookup_port, aux, &match, clause++,
                                      n_clauses, *n_conjsp, matches)) {
                     /* This clause can't ever match, so we might as well skip
                      * adding the other clauses--the overall disjunctive flow
@@ -2401,11 +2408,13 @@ add_conjunction(const struct expr *and, const struct simap *ports,
 }
 
 static void
-add_cmp_flow(const struct expr *cmp, const struct simap *ports,
-             struct hmap *matches)
+add_cmp_flow(const struct expr *cmp,
+             bool (*lookup_port)(const void *aux, const char *port_name,
+                                 unsigned int *portp),
+             const void *aux, struct hmap *matches)
 {
     struct expr_match *m = expr_match_new(NULL, 0, 0, 0);
-    if (constrain_match(cmp, ports, &m->match)) {
+    if (constrain_match(cmp, lookup_port, aux, &m->match)) {
         expr_match_add(matches, m);
     } else {
         free(m);
@@ -2433,47 +2442,52 @@ add_cmp_flow(const struct expr *cmp, const struct simap *ports,
  *       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
- * like a Boolean "false"; that is, it will always fail to match.  For a simple
- * expression, this means that the overall expression always fails to match,
- * but an expression with a disjunction on the string field might still match
- * on other port names.
+ * 'lookup_port' must be a function to map from a port name to a port number.
+ * When successful, 'lookup_port' stores the port number into '*portp' and
+ * returns true; when there is no port by the given name, it returns false.
+ * 'aux' is passed to 'lookup_port' as auxiliary data.  Any comparisons against
+ * string fields in 'expr' are translated into integers through this function.
+ * A comparison against a string that is not in 'ports' acts like a Boolean
+ * "false"; that is, it will always fail to match.  For a simple expression,
+ * this means that the overall expression always fails to match, but an
+ * expression with a disjunction on the string field might still match on other
+ * port names.
  *
  * (This treatment of string fields might be too simplistic in general, but it
  * seems reasonable for now when string fields are used only for ports.) */
 uint32_t
-expr_to_matches(const struct expr *expr, const struct simap *ports,
-                struct hmap *matches)
+expr_to_matches(const struct expr *expr,
+                bool (*lookup_port)(const void *aux, const char *port_name,
+                                    unsigned int *portp),
+                const void *aux, struct hmap *matches)
 {
     uint32_t n_conjs = 0;
 
     hmap_init(matches);
     switch (expr->type) {
     case EXPR_T_CMP:
-        add_cmp_flow(expr, ports, matches);
+        add_cmp_flow(expr, lookup_port, aux, matches);
         break;
 
     case EXPR_T_AND:
-        add_conjunction(expr, ports, &n_conjs, matches);
+        add_conjunction(expr, lookup_port, aux, &n_conjs, matches);
         break;
 
     case EXPR_T_OR:
         if (expr_is_cmp(expr)) {
             struct expr *sub;
 
             LIST_FOR_EACH (sub, node, &expr->andor) {
-                add_cmp_flow(sub, ports, matches);
+                add_cmp_flow(sub, lookup_port, aux, matches);
             }
         } else {
             struct expr *sub;
 
             LIST_FOR_EACH (sub, node, &expr->andor) {
                 if (sub->type == EXPR_T_AND) {
-                    add_conjunction(sub, ports, &n_conjs, matches);
+                    add_conjunction(sub, lookup_port, aux, &n_conjs, matches);
                 } else {
-                    add_cmp_flow(sub, ports, matches);
+                    add_cmp_flow(sub, lookup_port, aux, matches);
                 }
             }
         }
@@ -2696,8 +2710,10 @@ init_stack_action(const struct expr_field *f, struct ofpact_stack *stack)
 }
 
 static struct expr *
-parse_assignment(struct expr_context *ctx, const struct simap *ports,
-                 struct ofpbuf *ofpacts)
+parse_assignment(struct expr_context *ctx,
+                 bool (*lookup_port)(const void *aux, const char *port_name,
+                                     unsigned int *portp),
+                 const void *aux, struct ofpbuf *ofpacts)
 {
     struct expr *prereqs = NULL;
 
@@ -2806,7 +2822,10 @@ parse_assignment(struct expr_context *ctx, const struct simap *ports,
                    &c->mask.u8[sizeof c->mask - sf->field->n_bytes],
                    sf->field->n_bytes);
         } else {
-            uint32_t port = simap_get(ports, c->string);
+            uint32_t port;
+            if (!lookup_port(aux, c->string, &port)) {
+                port = 0;
+            }
             bitwise_put(port, &sf->value,
                         sf->field->n_bytes, 0, sf->field->n_bits);
             bitwise_one(&sf->mask, sf->field->n_bytes, 0, sf->field->n_bits);
@@ -2836,7 +2855,10 @@ parse_assignment(struct expr_context *ctx, const struct simap *ports,
  * those for actions_parse(). */
 char *
 expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
-                      const struct simap *ports,
+                      bool (*lookup_port)(const void *aux,
+                                          const char *port_name,
+                                          unsigned int *portp),
+                      const void *aux,
                       struct ofpbuf *ofpacts, struct expr **prereqsp)
 {
     struct expr_context ctx;
@@ -2845,7 +2867,7 @@ expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
     ctx.error = NULL;
     ctx.not = false;
 
-    struct expr *prereqs = parse_assignment(&ctx, ports, ofpacts);
+    struct expr *prereqs = parse_assignment(&ctx, lookup_port, aux, ofpacts);
     if (ctx.error) {
         expr_destroy(prereqs);
         prereqs = NULL;

ovn/lib/expr.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015 Nicira, Inc.
+ * Copyright (c) 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -370,15 +370,22 @@ struct expr_match {
     size_t n, allocated;
 };
 
-uint32_t expr_to_matches(const struct expr *, const struct simap *ports,
+uint32_t expr_to_matches(const struct expr *,
+                         bool (*lookup_port)(const void *aux,
+                                             const char *port_name,
+                                             unsigned int *portp),
+                         const void *aux,
                          struct hmap *matches);
 void expr_matches_destroy(struct hmap *matches);
 void expr_matches_print(const struct hmap *matches, FILE *);
 
 /* Action parsing helper. */
 
 char *expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
-                            const struct simap *ports, struct ofpbuf *ofpacts,
+                            bool (*lookup_port)(const void *aux,
+                                                const char *port_name,
+                                                unsigned int *portp),
+                            const void *aux, struct ofpbuf *ofpacts,
                             struct expr **prereqsp);
 
 #endif /* ovn/expr.h */

tests/test-ovn.c

@@ -238,6 +238,18 @@ create_symtab(struct shash *symtab)
     expr_symtab_add_string(symtab, "big_string", MFF_XREG0, NULL);
 }
 
+static bool
+lookup_port_cb(const void *ports_, const char *port_name, unsigned int *portp)
+{
+    const struct simap *ports = ports_;
+    const struct simap_node *node = simap_find(ports, port_name);
+    if (!node) {
+        return false;
+    }
+    *portp = node->data;
+    return true;
+}
+
 static void
 test_parse_expr__(int steps)
 {
@@ -274,7 +286,7 @@ test_parse_expr__(int steps)
             if (steps > 3) {
                 struct hmap matches;
 
-                expr_to_matches(expr, &ports, &matches);
+                expr_to_matches(expr, lookup_port_cb, &ports, &matches);
                 expr_matches_print(&matches, stdout);
                 expr_matches_destroy(&matches);
             } else {
@@ -934,7 +946,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
             struct expr_match *m;
             struct test_rule *test_rule;
 
-            expr_to_matches(modified, &string_map, &matches);
+            expr_to_matches(modified, lookup_port_cb, &string_map, &matches);
 
             classifier_init(&cls, NULL);
             HMAP_FOR_EACH (m, hmap_node, &matches) {
@@ -1229,7 +1241,8 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
         struct action_params ap = {
             .symtab = &symtab,
-            .ports = &ports,
+            .lookup_port = lookup_port_cb,
+            .aux = &ports,
             .ct_zones = &ct_zones,
 
             .n_tables = 16,