> 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

Reply via email to