On Thu, Oct 15, 2015 at 01:46:44PM -0700, Justin Pettit wrote:
>
> > On Oct 9, 2015, at 9:15 PM, Ben Pfaff <b...@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 <jpet...@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.
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, <able)) {
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;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
