On Fri, Jan 20, 2017 at 04:27:04PM -0800, Mickey Spiegel wrote:
> 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>
Thanks for the review.
> One comment inline.
>
> > + 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?
It's better with the return, so I added it. The lack of a return was
harmless though since the error was still being recorded and the
appended action would be ignored.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
