diff mbox

[ovs-dev,10/14] ovn: Use callback function instead of simap for logical port number map.

Message ID 1449623297-31060-11-git-send-email-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff Dec. 9, 2015, 1:08 a.m. UTC 
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 <blp@ovn.org>
---
 ovn/controller/lflow.c | 18 ++++++++++--
 ovn/lib/actions.c      |  3 +-
 ovn/lib/actions.h      | 10 ++++---
 ovn/lib/expr.c         | 74 +++++++++++++++++++++++++++++++-------------------
 ovn/lib/expr.h         | 10 +++++--
 tests/test-ovn.c       | 19 +++++++++++--
 6 files changed, 94 insertions(+), 40 deletions(-)

Comments

Justin Pettit Dec. 16, 2015, 1:59 a.m. UTC | #1 
> On Dec 8, 2015, at 5:08 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> @@ -31,10 +32,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 'name'.  If found, stores its port number in
> +     * '*portp' and returns true; otherwise, returns false. */
> +    bool (*lookup_port)(const void *aux, const char *name,
> +                        unsigned int *portp);

lookup_port_cb() refers to the second argument as "port_name" instead of "name".  It might be nice to use the same argument names.  If you change the name to "port_name", there are quite a few other protoypes defined in "expr.c" and "expr.h" that use "name", too.

> @@ -2443,19 +2452,21 @@ add_cmp_flow(const struct expr *cmp, const struct simap *ports,
>  * (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 *name,
> +                                    unsigned int *portp),
> +                const void *aux, struct hmap *matches)
> {

I think the comment describing this function needs to be updated, since "ports" is no longer an argument.

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Jan. 28, 2016, 4:51 a.m. UTC | #2 
On Tue, Dec 15, 2015 at 05:59:53PM -0800, Justin Pettit wrote:
> 
> > On Dec 8, 2015, at 5:08 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > @@ -31,10 +32,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 'name'.  If found, stores its port number in
> > +     * '*portp' and returns true; otherwise, returns false. */
> > +    bool (*lookup_port)(const void *aux, const char *name,
> > +                        unsigned int *portp);
> 
> lookup_port_cb() refers to the second argument as "port_name" instead
> of "name".  It might be nice to use the same argument names.  If you
> change the name to "port_name", there are quite a few other protoypes
> defined in "expr.c" and "expr.h" that use "name", too.

Thanks.  'port_name' is better.  I changed all the prototypes to
consistently use that name.

> > @@ -2443,19 +2452,21 @@ add_cmp_flow(const struct expr *cmp, const struct simap *ports,
> >  * (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 *name,
> > +                                    unsigned int *portp),
> > +                const void *aux, struct hmap *matches)
> > {
> 
> I think the comment describing this function needs to be updated,
> since "ports" is no longer an argument.

Thanks, I updated the comment.

> Acked-by: Justin Pettit <jpettit@ovn.org>
diff mbox

Patch

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 2f91bca..dcabe2f 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -255,6 +255,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
@@ -299,7 +311,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,
@@ -340,7 +353,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. */
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index be303f9..e7dea8d 100644
--- a/ovn/lib/actions.c
+++ b/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);
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index 2c3644a..6ca15c4 100644
--- a/ovn/lib/actions.h
+++ b/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"
 
@@ -31,10 +32,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 'name'.  If found, stores its port number in
+     * '*portp' and returns true; otherwise, returns false. */
+    bool (*lookup_port)(const void *aux, const char *name,
+                        unsigned int *portp);
+    const void *aux;
 
     /* A map from a port name to its connection tracking zone. */
     const struct simap *ct_zones;
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index f30500e..0d7ba32 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -2286,17 +2286,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 *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;
         }
 
@@ -2307,7 +2308,7 @@  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);
     }
@@ -2315,7 +2316,10 @@  constrain_match(const struct expr *expr, const struct simap *ports,
 }
 
 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 *name,
+                                    unsigned int *portp),
+                const void *aux,
                 struct match *m, uint8_t clause, uint8_t n_clauses,
                 uint32_t conj_id, struct hmap *matches)
 {
@@ -2326,7 +2330,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 {
@@ -2341,8 +2345,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 *name,
+                                    unsigned int *portp),
+                const void *aux, uint32_t *n_conjsp, struct hmap *matches)
 {
     struct match match;
     int n_clauses = 0;
@@ -2354,7 +2360,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;
@@ -2372,7 +2378,8 @@  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 {
@@ -2380,7 +2387,7 @@  add_conjunction(const struct expr *and, const struct simap *ports,
         (*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
@@ -2400,11 +2407,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 *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);
@@ -2443,19 +2452,21 @@  add_cmp_flow(const struct expr *cmp, const struct simap *ports,
  * (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 *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:
@@ -2463,16 +2474,16 @@  expr_to_matches(const struct expr *expr, const struct simap *ports,
             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);
                 }
             }
         }
@@ -2698,8 +2709,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 *name,
+                                     unsigned int *portp),
+                 const void *aux, struct ofpbuf *ofpacts)
 {
     struct expr *prereqs = NULL;
 
@@ -2808,7 +2821,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);
@@ -2838,7 +2854,9 @@  exit:
  * 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 *name,
+                                          unsigned int *portp),
+                      const void *aux,
                       struct ofpbuf *ofpacts, struct expr **prereqsp)
 {
     struct expr_context ctx;
@@ -2847,7 +2865,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;
diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
index d755b55..7d17489 100644
--- a/ovn/lib/expr.h
+++ b/ovn/lib/expr.h
@@ -370,7 +370,10 @@  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 *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 *);
@@ -378,7 +381,10 @@  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 *name,
+                                                unsigned int *portp),
+                            const void *aux, struct ofpbuf *ofpacts,
                             struct expr **prereqsp);
 
 #endif /* ovn/expr.h */
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 0a51369..3e43cd8 100644
--- a/tests/test-ovn.c
+++ b/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,