> On Aug 8, 2016, at 11:21 AM, Ben Pfaff <[email protected]> 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 <[email protected]>
Thanks for doing this. It's very clean, and the new test output is a big
improvement.
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
