On Fri, Jan 20, 2017 at 9:16 AM, Ben Pfaff <b...@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 <b...@ovn.org> > --- > 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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev