On Fri, Jan 20, 2017 at 9:16 AM, Ben Pfaff <[email protected]> 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 <[email protected]>
> ---
> include/ovn/actions.h | 8 ++++++++
> ovn/lib/actions.c | 29 +++++++++++++++++++----------
> tests/ovn.at | 8 ++++----
> 3 files changed, 31 insertions(+), 14 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..5548234 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -259,7 +259,11 @@ 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)) {
> + return;
> + }
> +
> + int table = ctx->pp->cur_ltable + 1;
> + if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> int ltable;
>
> if (!lexer_force_int(ctx->lexer, <able) ||
> @@ -273,22 +277,27 @@ parse_NEXT(struct action_context *ctx)
> ctx->pp->n_tables - 1);
> return;
> }
>
You never set "table = ltable;", which belongs here. As a result,
"next(11);" always encodes as "next;", i.e. the number is ignored.
> + }
>
> - 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);
> }
> +
> + 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.
>
Using this diff, there are two different responses
depending on whether the command is "next;" or
"next(11);".
The former gives the "cannot advance beyond" message.
The latter gives the "in range 0 to 15" message.
Unless the error messages are changed, this change
should be reverted.
Mickey
> # 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
