[ovs-dev] [xlate 2/4] ofproto-dpif: Move odp_actions from subfacet to facet.

[Justin Pettit]

On May 14, 2013, at 7:29 PM, Ethan Jackson <ethan at nicira.com> wrote:

> -/* Creates and returns a new facet owned by 'rule', given a 'flow'.
> +/* Creates and returns a new facet based on 'miss'.
>  *
>  * The caller must already have determined that no facet with an identical
> - * 'flow' exists in 'ofproto' and that 'flow' is the best match for 'rule' in
> - * the ofproto's classifier table.
> + * 'miss->flow' exists in 'miss->ofproto'.
>  *
> - * 'hash' must be the return value of flow_hash(flow, 0).
> + * 'hash' must be the return value of flow_hash(miss->flow, 0).
>  *
>  * The facet will initially have no subfacets.  The caller should create (at
>  * least) one subfacet with subfacet_create(). */
> static struct facet *
> -facet_create(struct rule_dpif *rule, const struct flow *flow, uint32_t hash)
> +facet_create(const struct flow_miss *miss, uint32_t hash)
> {
> -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> +    struct ofproto_dpif *ofproto = miss->ofproto;
> +    struct action_xlate_ctx ctx;
>     struct facet *facet;
> 
>     facet = xzalloc(sizeof *facet);
>     facet->used = time_msec();
> +    facet->flow = miss->flow;
> +    facet->initial_vals = miss->initial_vals;
> +    facet->rule = rule_dpif_lookup(ofproto, &facet->flow);

This isn't a full review, but I just wanted to comment on this.  The only caller to facet_create() is handle_flow_miss(), and it ran rule_dpif_lookup() just a few lines prior to calling facet_create().  I think it would be better to leave the "rule" argument in and just continue to use that value.

Thanks again for this series!  It makes the work I'm doing much easier and cleaner.

--Justin