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

Message ID 1444450544-11845-2-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 10, 2015, 4:15 a.m. UTC
This makes it easier to route a "destination unreachable" message
generated because of an IP routing failure, because the destination
unreachable message must itself be routed the same way.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/controller/lflow.c | 15 +++++----
 ovn/lib/actions.c      | 88 +++++++++++++++++++++++++++++++++++++++-----------
 ovn/lib/actions.h      | 10 +++---
 ovn/lib/expr.c         | 11 ++-----
 ovn/lib/lex.c          | 21 ++++++++++++
 ovn/lib/lex.h          |  2 ++
 ovn/ovn-sb.xml         |  5 ++-
 tests/ovn.at           | 10 ++++--
 tests/test-ovn.c       |  4 +--
 9 files changed, 123 insertions(+), 43 deletions(-)

Comments

Justin Pettit Oct. 15, 2015, 8:46 p.m. UTC | #1
> 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.

> +    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".

> +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.

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>

--Justin

Patch
diff mbox

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 9246e61..2b1984a 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -264,16 +264,16 @@  lflow_run(struct controller_ctx *ctx, struct hmap *flow_table)
             continue;
         }
 
-        /* Translate logical table ID to physical table ID. */
+        /* Determine translation of logical table IDs to physical table IDs. */
         bool ingress = !strcmp(lflow->pipeline, "ingress");
-        uint8_t phys_table = lflow->table_id + (ingress
-                                                ? OFTABLE_LOG_INGRESS_PIPELINE
-                                                : OFTABLE_LOG_EGRESS_PIPELINE);
-        uint8_t next_phys_table
-            = lflow->table_id + 1 < LOG_PIPELINE_LEN ? phys_table + 1 : 0;
+        uint8_t first_table = (ingress
+                               ? OFTABLE_LOG_INGRESS_PIPELINE
+                               : OFTABLE_LOG_EGRESS_PIPELINE);
+        uint8_t phys_table = first_table + lflow->table_id;
         uint8_t output_phys_table = (ingress
                                      ? OFTABLE_REMOTE_OUTPUT
                                      : OFTABLE_LOG_TO_PHY);
+
         /* Translate OVN actions into OpenFlow actions.
          *
          * XXX Deny changes to 'outport' in egress pipeline. */
@@ -284,7 +284,8 @@  lflow_run(struct controller_ctx *ctx, struct hmap *flow_table)
 
         ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
         error = actions_parse_string(lflow->actions, &symtab, &ldp->ports,
-                                     next_phys_table, output_phys_table,
+                                     first_table, LOG_PIPELINE_LEN,
+                                     lflow->table_id, output_phys_table,
                                      &ofpacts, &prereqs);
         if (error) {
             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 4170fc5..2bc495c 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -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. */
+    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. */
 
     /* State. */
@@ -131,6 +133,48 @@  emit_resubmit(struct action_context *ctx, uint8_t table_id)
     resubmit->table_id = table_id;
 }
 
+static bool
+action_get_int(struct action_context *ctx, int *value)
+{
+    bool ok = lexer_get_int(ctx->lexer, value);
+    if (!ok) {
+        action_syntax_error(ctx, "expecting small integer");
+    }
+    return ok;
+}
+
+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;
+
+        if (!action_get_int(ctx, &table)) {
+            return;
+        }
+        if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+            action_syntax_error(ctx, "expecting `)'");
+            return;
+        }
+
+        if (table >= 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);
+    } else {
+        if (ctx->cur_table < ctx->n_tables) {
+            emit_resubmit(ctx, ctx->first_table + ctx->cur_table + 1);
+        } else {
+            action_error(ctx, "\"next\" action not allowed in last table.");
+        }
+    }
+}
+
 static void
 parse_actions(struct action_context *ctx)
 {
@@ -158,13 +202,9 @@  parse_actions(struct action_context *ctx)
             || lookahead == LEX_T_LSQUARE) {
             parse_set_action(ctx);
         } else if (lexer_match_id(ctx->lexer, "next")) {
-            if (ctx->next_table_id) {
-                emit_resubmit(ctx, ctx->next_table_id);
-            } else {
-                action_error(ctx, "\"next\" action not allowed here.");
-            }
+            parse_next_action(ctx);
         } else if (lexer_match_id(ctx->lexer, "output")) {
-            emit_resubmit(ctx, ctx->output_table_id);
+            emit_resubmit(ctx, ctx->output_table);
         } else {
             action_syntax_error(ctx, "expecting action");
         }
@@ -188,10 +228,16 @@  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.
  *
- * 'next_table_id' should be the OpenFlow table to which the "next" action will
- * resubmit, or 0 to disable "next".
+ * '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.
+ *
+ * '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.
  *
- * 'output_table_id' should be the OpenFlow table to which the "output" action
+ * 'output_table' should be the OpenFlow table to which the "output" action
  * will resubmit
  *
  * Some actions add extra requirements (prerequisites) to the flow's match.  If
@@ -206,8 +252,9 @@  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 next_table_id,
-              uint8_t output_table_id, struct ofpbuf *ofpacts,
+              const struct simap *ports,
+              uint8_t first_table, uint8_t n_tables, uint8_t cur_table,
+              uint8_t output_table, struct ofpbuf *ofpacts,
               struct expr **prereqsp)
 {
     size_t ofpacts_start = ofpacts->size;
@@ -216,8 +263,10 @@  actions_parse(struct lexer *lexer, const struct shash *symtab,
     ctx.lexer = lexer;
     ctx.symtab = symtab;
     ctx.ports = ports;
-    ctx.next_table_id = next_table_id;
-    ctx.output_table_id = output_table_id;
+    ctx.first_table = first_table;
+    ctx.n_tables = n_tables;
+    ctx.cur_table = cur_table;
+    ctx.output_table = output_table;
     ctx.error = NULL;
     ctx.ofpacts = ofpacts;
     ctx.prereqs = NULL;
@@ -238,8 +287,9 @@  actions_parse(struct lexer *lexer, const struct shash *symtab,
 /* Like actions_parse(), but the actions are taken from 's'. */
 char * OVS_WARN_UNUSED_RESULT
 actions_parse_string(const char *s, const struct shash *symtab,
-                     const struct simap *ports, uint8_t next_table_id,
-                     uint8_t output_table_id, struct ofpbuf *ofpacts,
+                     const struct simap *ports, uint8_t first_table,
+                     uint8_t n_tables, uint8_t cur_table,
+                     uint8_t output_table, struct ofpbuf *ofpacts,
                      struct expr **prereqsp)
 {
     struct lexer lexer;
@@ -247,8 +297,8 @@  actions_parse_string(const char *s, const struct shash *symtab,
 
     lexer_init(&lexer, s);
     lexer_get(&lexer);
-    error = actions_parse(&lexer, symtab, ports, next_table_id,
-                          output_table_id, ofpacts, prereqsp);
+    error = actions_parse(&lexer, symtab, ports, first_table, n_tables,
+                          cur_table, output_table, ofpacts, prereqsp);
     lexer_destroy(&lexer);
 
     return error;
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index 74cd185..ae897d6 100644
--- a/ovn/lib/actions.h
+++ b/ovn/lib/actions.h
@@ -27,13 +27,15 @@  struct shash;
 struct simap;
 
 char *actions_parse(struct lexer *, const struct shash *symtab,
-                    const struct simap *ports, uint8_t next_table_id,
-                    uint8_t output_table_id, struct ofpbuf *ofpacts,
+                    const struct simap *ports, uint8_t first_table,
+                    uint8_t n_tables, uint8_t cur_table,
+                    uint8_t output_table, 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 next_table_id,
-                           uint8_t output_table_id, struct ofpbuf *ofpacts,
+                           const struct simap *ports, uint8_t first_table,
+                           uint8_t n_tables, uint8_t cur_table,
+                           uint8_t output_table, struct ofpbuf *ofpacts,
                            struct expr **prereqsp)
     OVS_WARN_UNUSED_RESULT;
 
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 8270b82..6fbe3e2 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -668,16 +668,11 @@  exit:
 static bool
 expr_get_int(struct expr_context *ctx, int *value)
 {
-    if (ctx->lexer->token.type == LEX_T_INTEGER
-        && ctx->lexer->token.format == LEX_F_DECIMAL
-        && ntohll(ctx->lexer->token.value.integer) <= INT_MAX) {
-        *value = ntohll(ctx->lexer->token.value.integer);
-        lexer_get(ctx->lexer);
-        return true;
-    } else {
+    bool ok = lexer_get_int(ctx->lexer, value);
+    if (!ok) {
         expr_syntax_error(ctx, "expecting small integer.");
-        return false;
     }
+    return ok;
 }
 
 static bool
diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
index 2ffcfb9..308d216 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -750,3 +750,24 @@  lexer_match_id(struct lexer *lexer, const char *id)
         return false;
     }
 }
+
+bool
+lexer_is_int(const struct lexer *lexer)
+{
+    return (lexer->token.type == LEX_T_INTEGER
+            && lexer->token.format == LEX_F_DECIMAL
+            && ntohll(lexer->token.value.integer) <= INT_MAX);
+}
+
+bool
+lexer_get_int(struct lexer *lexer, int *value)
+{
+    if (lexer_is_int(lexer)) {
+        *value = ntohll(lexer->token.value.integer);
+        lexer_get(lexer);
+        return true;
+    } else {
+        *value = 0;
+        return false;
+    }
+}
diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
index b5828a2..7ad6f55 100644
--- a/ovn/lib/lex.h
+++ b/ovn/lib/lex.h
@@ -109,5 +109,7 @@  enum lex_type lexer_get(struct lexer *);
 enum lex_type lexer_lookahead(const struct lexer *);
 bool lexer_match(struct lexer *, enum lex_type);
 bool lexer_match_id(struct lexer *, const char *id);
+bool lexer_is_int(const struct lexer *);
+bool lexer_get_int(struct lexer *, int *value);
 
 #endif /* ovn/lex.h */
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 4609c2d..8c457d4 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -767,8 +767,11 @@ 
 	</dd>
 
         <dt><code>next;</code></dt>
+        <dt><code>next(<var>table</var>);</code></dt>
         <dd>
-          Executes the next logical datapath table as a subroutine.
+          Executes another logical datapath table as a subroutine.  By default,
+          the table after the current one is executed.  Specify
+          <var>table</var> to jump to a specific table in the same pipeline.
         </dd>
 
         <dt><code><var>field</var> = <var>constant</var>;</code></dt>
diff --git a/tests/ovn.at b/tests/ovn.at
index 1eb6d0b..330f723 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -438,9 +438,11 @@  dnl Text before => is input, text after => is expected output.
 AT_DATA([test-cases.txt], [[
 # Positive tests.
 drop; => actions=drop, prereqs=1
-next; => actions=resubmit(,11), prereqs=1
+next; => actions=resubmit(,27), prereqs=1
+next(0); => actions=resubmit(,16), prereqs=1
+next(15); => actions=resubmit(,31), prereqs=1
 output; => actions=resubmit(,64), prereqs=1
-outport="eth0"; next; outport="LOCAL"; next; => actions=set_field:0x5->reg7,resubmit(,11),set_field:0xfffe->reg7,resubmit(,11), prereqs=1
+outport="eth0"; next; outport="LOCAL"; next; => actions=set_field:0x5->reg7,resubmit(,27),set_field:0xfffe->reg7,resubmit(,27), prereqs=1
 tcp.dst=80; => actions=set_field:80->tcp_dst, prereqs=ip.proto == 0x6 && (eth.type == 0x800 || eth.type == 0x86dd)
 eth.dst[40] = 1; => actions=set_field:01:00:00:00:00:00/01:00:00:00:00:00->eth_dst, prereqs=1
 vlan.pcp = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=vlan.tci[12]
@@ -471,6 +473,10 @@  next; drop; => Syntax error at `drop' expecting action.
 # Missing ";":
 next => Syntax error at end of input expecting ';'.
 
+next(); => Syntax error at `)' expecting small integer.
+next(10; => Syntax error at `;' expecting `)'.
+next(16); => "next" argument must be in range 0 to 15.
+
 inport[1] = 1; => Cannot select subfield of string field inport.
 ip.proto[1] = 1; => Cannot select subfield of nominal field ip.proto.
 eth.dst[40] == 1; => Syntax error at `==' expecting `='.
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 774ebdf..0e9d2d2 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1225,8 +1225,8 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
         char *error;
 
         ofpbuf_init(&ofpacts, 0);
-        error = actions_parse_string(ds_cstr(&input), &symtab, &ports, 11, 64,
-                                     &ofpacts, &prereqs);
+        error = actions_parse_string(ds_cstr(&input), &symtab, &ports,
+                                     16, 16, 10, 64, &ofpacts, &prereqs);
         if (!error) {
             struct ds output;