[ovs-dev,01/23] ovn: Extend logical "next" action to jump to arbitrary flow tables.
diff mbox

Message ID 20151016032253.GA10217@nicira.com
State Not Applicable
Headers show

Commit Message

Ben Pfaff Oct. 16, 2015, 3:22 a.m. UTC
On Thu, Oct 15, 2015 at 01:46:44PM -0700, Justin Pettit wrote:
> 
> > On Oct 9, 2015, at 9:15 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > @@ -30,8 +30,10 @@ struct action_context {
> >     /* Input. */
> >     struct lexer *lexer;        /* Lexer for pulling more tokens. */
> >     const struct shash *symtab; /* Symbol table. */
> > -    uint8_t next_table_id;      /* OpenFlow table for 'next' to resubmit. */
> > -    uint8_t output_table_id;    /* OpenFlow table for 'output' to resubmit. */
> > +    uint8_t first_table;        /* First OpenFlow table. */
> > +    uint8_t n_tables;           /* Number of OpenFlow tables. */
> 
> I think these two descriptions could be a bit misleading, since they
> define the range of tables that a "next" action can jump to, but it
> sounds like it's the full range of supported OpenFlow tables.

OK, that's a good point, thanks.

> > +    uint8_t cur_table;          /* 0 <= cur_table < n_tables. */
> 
> This member name seems a little confusing, since all the other
> "_table" members refer to an OpenFlow port, but this one is an offset.
> What about something like "cur_table_offset"?  Another option would be
> to relabel them things like "*_of_table" (or "*_phy_table") and
> "*_log_table".

How about ptable and ltable?

> > +static void
> > +parse_next_action(struct action_context *ctx)
> > +{
> > +    if (!ctx->n_tables) {
> > +        action_error(ctx, "\"next\" action not allowed here.");
> > +    } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> > +        int table;
> 
> Similar to above, this is actually a logical table or an offset to an
> OpenFlow table.  It might be nice to clarify that for later readers.

I renamed it "ltable".

> This is a nice addition, but I think some of the older phrasing
> related to logical and physical tables was a bit clearer than exposing
> that they're offsets.
>
> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks for the review.  I guess given the ack you're more or less happy,
so here's an incremental that I folded in, and I'll apply this to master
in a minute.

Patch
diff mbox

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 2bc495c..8afddee 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -27,19 +27,24 @@ 
 
 /* Context maintained during actions_parse(). */
 struct action_context {
-    /* Input. */
+/* Input. */
+
     struct lexer *lexer;        /* Lexer for pulling more tokens. */
-    const struct shash *symtab; /* Symbol table. */
-    uint8_t first_table;        /* First OpenFlow table. */
-    uint8_t n_tables;           /* Number of OpenFlow tables. */
-    uint8_t cur_table;          /* 0 <= cur_table < n_tables. */
-    uint8_t output_table;       /* OpenFlow table for 'output' to resubmit. */
     const struct simap *ports;  /* Map from port name to number. */
+    const struct shash *symtab; /* Symbol table. */
 
-    /* State. */
+    /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
+     * OpenFlow flow table (ptable).  These members describe the mapping and
+     * data related to flow tables. */
+    uint8_t n_tables;           /* Number of flow tables. */
+    uint8_t first_ptable;       /* First OpenFlow table. */
+    uint8_t cur_ltable;         /* 0 <= cur_ltable < n_tables. */
+    uint8_t output_ptable;      /* OpenFlow table for 'output' to resubmit. */
+
+/* State. */
     char *error;                /* Error, if any, otherwise NULL. */
 
-    /* Output. */
+/* Output. */
     struct ofpbuf *ofpacts;     /* Actions. */
     struct expr *prereqs;       /* Prerequisites to apply to match. */
 };
@@ -149,9 +154,9 @@  parse_next_action(struct action_context *ctx)
     if (!ctx->n_tables) {
         action_error(ctx, "\"next\" action not allowed here.");
     } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
-        int table;
+        int ltable;
 
-        if (!action_get_int(ctx, &table)) {
+        if (!action_get_int(ctx, &ltable)) {
             return;
         }
         if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
@@ -159,16 +164,16 @@  parse_next_action(struct action_context *ctx)
             return;
         }
 
-        if (table >= ctx->n_tables) {
+        if (ltable >= ctx->n_tables) {
             action_error(ctx, "\"next\" argument must be in range 0 to %d.",
                          ctx->n_tables - 1);
             return;
         }
 
-        emit_resubmit(ctx, ctx->first_table + table);
+        emit_resubmit(ctx, ctx->first_ptable + ltable);
     } else {
-        if (ctx->cur_table < ctx->n_tables) {
-            emit_resubmit(ctx, ctx->first_table + ctx->cur_table + 1);
+        if (ctx->cur_ltable < ctx->n_tables) {
+            emit_resubmit(ctx, ctx->first_ptable + ctx->cur_ltable + 1);
         } else {
             action_error(ctx, "\"next\" action not allowed in last table.");
         }
@@ -204,7 +209,7 @@  parse_actions(struct action_context *ctx)
         } else if (lexer_match_id(ctx->lexer, "next")) {
             parse_next_action(ctx);
         } else if (lexer_match_id(ctx->lexer, "output")) {
-            emit_resubmit(ctx, ctx->output_table);
+            emit_resubmit(ctx, ctx->output_ptable);
         } else {
             action_syntax_error(ctx, "expecting action");
         }
@@ -228,17 +233,23 @@  parse_actions(struct action_context *ctx)
  * (as one would provide to expr_to_matches()).  Strings used in the actions
  * that are not in 'ports' are translated to zero.
  *
- * 'first_table' and 'n_tables' define the range of OpenFlow tables that the
- * logical "next" action should be able to jump to.  Logical table 0 maps to
- * OpenFlow table 'first_table', logical table 1 to 'first_table + 1', and so
- * on.  If 'n_tables' is 0 then "next" is disallowed entirely.
+ * OVN maps each logical flow table (ltable), one-to-one, onto a physical
+ * OpenFlow flow table (ptable).  A number of parameters describe this mapping
+ * and data related to flow tables:
+ *
+ *     - 'first_ptable' and 'n_tables' define the range of OpenFlow tables to
+ *       which the logical "next" action should be able to jump.  Logical table
+ *       0 maps to OpenFlow table 'first_ptable', logical table 1 to
+ *       'first_ptable + 1', and so on.  If 'n_tables' is 0 then "next" is
+ *       disallowed entirely.
  *
- * 'cur_table' is an offset from 'first_table' (e.g. 0 <= cur_table < n_tables)
- * of the logical flow that contains the actions.  If cur_table + 1 < n_tables,
- * then this defines the default table that "next" will jump to.
+ *     - 'cur_ltable' is an offset from 'first_ptable' (e.g. 0 <= cur_ltable <
+ *       n_ptables) of the logical flow that contains the actions.  If
+ *       cur_ltable + 1 < n_tables, then this defines the default table that
+ *       "next" will jump to.
  *
- * 'output_table' should be the OpenFlow table to which the "output" action
- * will resubmit
+ *     - 'output_ptable' should be the OpenFlow table to which the logical
+ *       "output" action will resubmit
  *
  * Some actions add extra requirements (prerequisites) to the flow's match.  If
  * so, this function sets '*prereqsp' to the actions' prerequisites; otherwise,
@@ -253,8 +264,8 @@  parse_actions(struct action_context *ctx)
 char * OVS_WARN_UNUSED_RESULT
 actions_parse(struct lexer *lexer, const struct shash *symtab,
               const struct simap *ports,
-              uint8_t first_table, uint8_t n_tables, uint8_t cur_table,
-              uint8_t output_table, struct ofpbuf *ofpacts,
+              uint8_t first_ptable, uint8_t n_tables, uint8_t cur_ltable,
+              uint8_t output_ptable, struct ofpbuf *ofpacts,
               struct expr **prereqsp)
 {
     size_t ofpacts_start = ofpacts->size;
@@ -263,10 +274,10 @@  actions_parse(struct lexer *lexer, const struct shash *symtab,
     ctx.lexer = lexer;
     ctx.symtab = symtab;
     ctx.ports = ports;
-    ctx.first_table = first_table;
+    ctx.first_ptable = first_ptable;
     ctx.n_tables = n_tables;
-    ctx.cur_table = cur_table;
-    ctx.output_table = output_table;
+    ctx.cur_ltable = cur_ltable;
+    ctx.output_ptable = output_ptable;
     ctx.error = NULL;
     ctx.ofpacts = ofpacts;
     ctx.prereqs = NULL;
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index ae897d6..a84c05b 100644
--- a/ovn/lib/actions.h
+++ b/ovn/lib/actions.h
@@ -27,15 +27,15 @@  struct shash;
 struct simap;
 
 char *actions_parse(struct lexer *, const struct shash *symtab,
-                    const struct simap *ports, uint8_t first_table,
-                    uint8_t n_tables, uint8_t cur_table,
-                    uint8_t output_table, struct ofpbuf *ofpacts,
+                    const struct simap *ports, uint8_t first_ptable,
+                    uint8_t n_tables, uint8_t cur_ltable,
+                    uint8_t output_ptable, struct ofpbuf *ofpacts,
                     struct expr **prereqsp)
     OVS_WARN_UNUSED_RESULT;
 char *actions_parse_string(const char *s, const struct shash *symtab,
-                           const struct simap *ports, uint8_t first_table,
-                           uint8_t n_tables, uint8_t cur_table,
-                           uint8_t output_table, struct ofpbuf *ofpacts,
+                           const struct simap *ports, uint8_t first_ptable,
+                           uint8_t n_tables, uint8_t cur_ltable,
+                           uint8_t output_ptable, struct ofpbuf *ofpacts,
                            struct expr **prereqsp)
     OVS_WARN_UNUSED_RESULT;