I did a bad job on this patch. I've rewritten it and will repost.
On Fri, Jan 20, 2017 at 11:51:56AM -0800, Mickey Spiegel wrote: > 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
