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

Justin Pettit jpettit at nicira.com
Thu Oct 15 20:46:44 UTC 2015


> On Oct 9, 2015, at 9:15 PM, Ben Pfaff <blp at 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 at nicira.com>

--Justin





More information about the dev mailing list