> On Aug 8, 2016, at 11:21 AM, Ben Pfaff <b...@ovn.org> wrote: > > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index a395ce9..d24db59 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -20,16 +20,261 @@ > ... > +#define OVNACTS \ > + OVNACT(OUTPUT, ovnact_null) \ > + OVNACT(NEXT, ovnact_next) \ > + OVNACT(LOAD, ovnact_load) \ > + OVNACT(MOVE, ovnact_move) \ > + OVNACT(EXCHANGE, ovnact_move) \ > + OVNACT(DEC_TTL, ovnact_null) \ > + OVNACT(CT_NEXT, ovnact_next) \ > + OVNACT(CT_COMMIT, ovnact_ct_commit) \ > + OVNACT(CT_DNAT, ovnact_ct_nat) \ > + OVNACT(CT_SNAT, ovnact_ct_nat) \ > + OVNACT(CT_LB, ovnact_ct_lb) \ > + OVNACT(ARP, ovnact_nest) \ > + OVNACT(ND_NA, ovnact_nest) \ > + OVNACT(GET_ARP, ovnact_get_mac_bind) \ > + OVNACT(PUT_ARP, ovnact_put_mac_bind) \ > + OVNACT(GET_ND, ovnact_get_mac_bind) \ > + OVNACT(PUT_ND, ovnact_put_mac_bind) \ > + OVNACT(PUT_DHCP_OPTS, ovnact_put_dhcp_opts) > ... > +/* OVNACT_NEXT. */ > +struct ovnact_next { > + struct ovnact ovnact; > + uint8_t ltable; /* Logical table ID of next table. */ > +};
Did you want to mention this is also used by OVNACT_CT_NEXT? > +/* OVNACT_ARP, OVNACT_NA. */ > +struct ovnact_nest { > + struct ovnact ovnact; > + struct ovnact *nested; > + size_t nested_len; > +}; I think that should be OVNACT_ND_NA instead of OVNACT_NA. > @@ -97,14 +342,45 @@ struct action_header { > }; > BUILD_ASSERT_DECL(sizeof(struct action_header) == 8); > > -struct action_params { > +struct ovnact_parse_params { > /* A table of "struct expr_symbol"s to support (as one would provide to > * expr_parse()). */ > const struct shash *symtab; > > - /* hmap of 'struct dhcp_opts_map' to support 'put_dhcp_opts' action */ > + /* hmap of 'struct dhcp_opts_map' to support 'put_dhcp_opts' action */ > const struct hmap *dhcp_opts; > > + /* OVN maps each logical flow table (ltable), one-to-one, onto a physical > + * OpenFlow flow table (ptable). A number of parameters describe this > + * mapping and data related to flow tables: > + * > + * - 'first_ptable' and 'n_tables' define the range of OpenFlow > tables > + * to which the logical "next" action should be able to jump. > + * Logical table 0 maps to OpenFlow table 'first_ptable', logical > + * table 1 to 'first_ptable + 1', and so on. If 'n_tables' is 0 > + * then "next" is disallowed entirely. > + * > + * - 'cur_ltable' is an offset from 'first_ptable' (e.g. 0 <= > + * cur_ltable < n_ptables) of the logical flow that contains the Should that be "n_tables" instead of "n_ptables"? > +static enum expr_constant_type > +load_type(const struct ovnact_load *load) > +{ > + return load->dst.symbol->width > 0 ? EXPR_C_INTEGER : EXPR_C_STRING; > > - struct action_header ah = { .opcode = htonl(opcode) }; > - ofpbuf_put(ofpacts, &ah, sizeof ah); > +} There's an unnecessary blank line here. > + } else if (snat) { > + /* XXX: For performance reasons, we try to prevent additional > + * recirculations. So far, ct_snat which is used in a gateway router > + * does not need a recirculation. ct_snat(IP) does need a > + * recirculation. Should we consider a method to let the actions > + * specify whether a action needs recirculation if there more use > + * cases?. */ s/a action/an action/ > + while (!lexer_match(ctx->lexer, LEX_T_RCURLY)) { > + if (!parse_action(&inner_ctx)) { > + break; > + } > + } > + > + expr_destroy(inner_ctx.prereqs); /* XXX what is the correct behavior? */ This comment seems a little worrying. Is it something that we should fix now? If not, it would be helpful to describe in greater detail what we need to fix in the future. > @@ -1169,21 +1758,21 @@ parse_actions(struct action_context *ctx) > * eventually free it. > * > * Returns NULL on success, otherwise a malloc()'d error message that the > - * caller must free. On failure, 'ofpacts' has the same contents and > + * caller must free. On failure, 'ovnacts' has the same contents and > * '*prereqsp' is set to NULL, but some tokens may have been consumed from > * 'lexer'. > */ You didn't introduce it, but I think there's an extra space here. > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > index db00d7c..25281e0 100644 > --- a/ovn/lib/expr.c > +++ b/ovn/lib/expr.c ... > +/* Checks that 'f' is 'n_bits' wide (where 'n_bits == 0' means that 'f' must > be > + * a string field) and, if 'rw' is true, that 'f' is modifiable. Returns > NULL > + * if 'f' is acceptable, otherwise an malloc()'d error message that the > caller > + * must free(). */ s/an malloc/a malloc/ Due to the churn, this was kind of a tough patch to review. I re-went though "actions.c", and it looks good. Also, it has really good tests, which is reassuring. Acked-by: Justin Pettit <jpet...@ovn.org> Thanks for doing this. It's very clean, and the new test output is a big improvement. --Justin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev