On Fri, Jan 20, 2017 at 2:48 PM, 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> >
Acked-by: Mickey Spiegel <mickeys....@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, <able) || > - !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 > 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