[ovs-dev] [PATCH v2 4/7] actions: Omit table number when possible for formatting "next" action.

[Mickey Spiegel]

On Fri, Jan 20, 2017 at 2:48 PM, Ben Pfaff <blp at ovn.org> wrote:

> Until now, formatting the "next" action has always required including
> the table number, because the action struct didn't include enough context
> so that the formatter could decide whether the table number was the next
> table or some other table.  This is more or less OK, but an upcoming commit
> will add a "pipeline" field to the "next" action, which means that the same
> policy there would require that the pipeline always be printed.  That's a
> little obnoxious because 99+% of the time, the pipeline to be printed is
> the same pipeline that the flow is in and printing it would be distracting.
> So it's better to store some context to help with formatting.  This commit
> begins adopting that policy for the existing table number field.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
>

Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>

One comment inline.


> ---
>  include/ovn/actions.h |  8 ++++++++
>  ovn/lib/actions.c     | 43 +++++++++++++++++++++----------------------
>  tests/ovn.at          |  8 ++++----
>  3 files changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 92c8888..38764fe 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -143,7 +143,15 @@ struct ovnact_null {
>  /* OVNACT_NEXT. */
>  struct ovnact_next {
>      struct ovnact ovnact;
> +
> +    /* Arguments. */
>      uint8_t ltable;             /* Logical table ID of next table. */
> +
> +    /* Information about the flow that the action is in.  This does not
> affect
> +     * behavior, since the implementation of "next" doesn't depend on the
> +     * source table or pipeline.  It does affect how ovnacts_format()
> prints
> +     * the action. */
> +    uint8_t src_ltable;            /* Logical table ID of source table. */
>  };
>
>  /* OVNACT_LOAD. */
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 2162dad..bcc690f 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -259,36 +259,35 @@ parse_NEXT(struct action_context *ctx)
>  {
>      if (!ctx->pp->n_tables) {
>          lexer_error(ctx->lexer, "\"next\" action not allowed here.");
> -    } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> -        int ltable;
> -
> -        if (!lexer_force_int(ctx->lexer, &ltable) ||
> -            !lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
> -            return;
> -        }
> +        return;
> +    }
>
> -        if (ltable >= ctx->pp->n_tables) {
> -            lexer_error(ctx->lexer,
> -                        "\"next\" argument must be in range 0 to %d.",
> -                         ctx->pp->n_tables - 1);
> -            return;
> -        }
> +    int table = ctx->pp->cur_ltable + 1;
> +    if (lexer_match(ctx->lexer, LEX_T_LPAREN)
> +        && (!lexer_force_int(ctx->lexer, &table) ||
> +            !lexer_force_match(ctx->lexer, LEX_T_RPAREN))) {
> +        return;
> +    }
>
> -        ovnact_put_NEXT(ctx->ovnacts)->ltable = ltable;
> -    } else {
> -        if (ctx->pp->cur_ltable < ctx->pp->n_tables) {
> -            ovnact_put_NEXT(ctx->ovnacts)->ltable = ctx->pp->cur_ltable
> + 1;
> -        } else {
> -            lexer_error(ctx->lexer,
> -                        "\"next\" action not allowed in last table.");
> -        }
> +    if (table >= ctx->pp->n_tables) {
> +        lexer_error(ctx->lexer,
> +                    "\"next\" action cannot advance beyond table %d.",
> +                    ctx->pp->n_tables - 1);
>

Should there be a "return;" here?

Mickey


>      }
> +
> +    struct ovnact_next *next = ovnact_put_NEXT(ctx->ovnacts);
> +    next->ltable = table;
> +    next->src_ltable = ctx->pp->cur_ltable;
>  }
>
>  static void
>  format_NEXT(const struct ovnact_next *next, struct ds *s)
>  {
> -    ds_put_format(s, "next(%d);", next->ltable);
> +    if (next->ltable != next->src_ltable + 1) {
> +        ds_put_format(s, "next(%d);", next->ltable);
> +    } else {
> +        ds_put_cstr(s, "next;");
> +    }
>  }
>
>  static void
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 67d73c5..f71a4af 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -643,9 +643,9 @@ output;
>
>  # next
>  next;
> -    formats as next(11);
>      encodes as resubmit(,27)
>  next(11);
> +    formats as next;
>      encodes as resubmit(,27)
>  next(0);
>      encodes as resubmit(,16)
> @@ -657,7 +657,7 @@ next();
>  next(10;
>      Syntax error at `;' expecting `)'.
>  next(16);
> -    "next" argument must be in range 0 to 15.
> +    "next" action cannot advance beyond table 15.
>
>  # Loading a constant value.
>  tcp.dst=80;
> @@ -678,7 +678,7 @@ ip.ttl=4;
>      encodes as set_field:4->nw_ttl
>      has prereqs eth.type == 0x800 || eth.type == 0x86dd
>  outport="eth0"; next; outport="LOCAL"; next;
> -    formats as outport = "eth0"; next(11); outport = "LOCAL"; next(11);
> +    formats as outport = "eth0"; next; outport = "LOCAL"; next;
>      encodes as set_field:0x5->reg15,resubmit(
> ,27),set_field:0xfffe->reg15,resubmit(,27)
>
>  inport[1] = 1;
> @@ -868,7 +868,7 @@ ct_snat();
>      Syntax error at `)' expecting IPv4 address.
>
>  # clone
> -clone { ip4.dst = 255.255.255.255; output; }; next(11);
> +clone { ip4.dst = 255.255.255.255; output; }; next;
>      encodes as clone(set_field:255.255.255.255->ip_dst,resubmit(,64)),
> resubmit(,27)
>      has prereqs eth.type == 0x800
>
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>