[ovs-dev] [PATCH 36/41] ofproto-dpif-rid: Don't carry actset_output explicitly in metadata.

[Jarno Rajahalme]

This is nice reduction of code duplication as well.

Acked-by: Jarno Rajahalme <jarno at ovn.org>

> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> Instead reconstruct it using the action set, since we already have the
> logic to do that.
> 
> This seems a little nicer because we don't have to "trust" the metadata
> as much.
> 
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> ofproto/ofproto-dpif-rid.h   |  3 ---
> ofproto/ofproto-dpif-xlate.c | 40 ++++++++++++++++++----------------------
> 2 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index ba968ba..654c95c 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -101,7 +101,6 @@ struct recirc_metadata {
>     ovs_be64 metadata;            /* OpenFlow Metadata. */
>     uint64_t regs[FLOW_N_XREGS];  /* Registers. */
>     ofp_port_t in_port;           /* Incoming port. */
> -    ofp_port_t actset_output;     /* Output port in action set. */
> };
> 
> static inline void
> @@ -113,7 +112,6 @@ recirc_metadata_from_flow(struct recirc_metadata *md,
>     md->metadata = flow->metadata;
>     memcpy(md->regs, flow->regs, sizeof md->regs);
>     md->in_port = flow->in_port.ofp_port;
> -    md->actset_output = flow->actset_output;
> }
> 
> static inline void
> @@ -128,7 +126,6 @@ recirc_metadata_to_flow(const struct recirc_metadata *md,
>     flow->metadata = md->metadata;
>     memcpy(flow->regs, md->regs, sizeof flow->regs);
>     flow->in_port.ofp_port = md->in_port;
> -    flow->actset_output = md->actset_output;
> }
> 
> /* State that flow translation can save, to restore when recirculation
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 1140652..e3b46ab 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4074,12 +4074,9 @@ may_receive(const struct xport *xport, struct xlate_ctx *ctx)
> }
> 
> static void
> -xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
> +xlate_write_actions__(struct xlate_ctx *ctx,
> +                      const struct ofpact *ofpacts, size_t ofpacts_len)
> {
> -    const struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
> -    size_t on_len = ofpact_nest_get_action_len(on);
> -    const struct ofpact *inner;
> -
>     /* Maintain actset_output depending on the contents of the action set:
>      *
>      *   - OFPP_UNSET, if there is no "output" action.
> @@ -4090,10 +4087,11 @@ xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
>      *   - OFPP_UNSET, if there is a "group" action.
>      */
>     if (!ctx->action_set_has_group) {
> -        OFPACT_FOR_EACH (inner, on->actions, on_len) {
> -            if (inner->type == OFPACT_OUTPUT) {
> -                ctx->xin->flow.actset_output = ofpact_get_OUTPUT(inner)->port;
> -            } else if (inner->type == OFPACT_GROUP) {
> +        const struct ofpact *a;
> +        OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> +            if (a->type == OFPACT_OUTPUT) {
> +                ctx->xin->flow.actset_output = ofpact_get_OUTPUT(a)->port;
> +            } else if (a->type == OFPACT_GROUP) {
>                 ctx->xin->flow.actset_output = OFPP_UNSET;
>                 ctx->action_set_has_group = true;
>                 break;
> @@ -4101,7 +4099,13 @@ xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
>         }
>     }
> 
> -    ofpbuf_put(&ctx->action_set, on->actions, on_len);
> +    ofpbuf_put(&ctx->action_set, ofpacts, ofpacts_len);
> +}
> +
> +static void
> +xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact_nest *a)
> +{
> +    xlate_write_actions__(ctx, a->actions, ofpact_nest_get_action_len(a));
> }
> 
> static void
> @@ -4723,7 +4727,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>             break;
> 
>         case OFPACT_WRITE_ACTIONS:
> -            xlate_write_actions(ctx, a);
> +            xlate_write_actions(ctx, ofpact_get_WRITE_ACTIONS(a));
>             break;
> 
>         case OFPACT_WRITE_METADATA:
> @@ -5175,20 +5179,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> 
>         /* Restore action set, if any. */
>         if (state->action_set_len) {
> -            const struct ofpact *a;
> -
>             xlate_report_actions(&ctx, "- Restoring action set",
>                                  state->action_set, state->action_set_len);
> 
> -            ofpbuf_put(&ctx.action_set,
> -                       state->action_set, state->action_set_len);
> -
> -            OFPACT_FOR_EACH (a, state->action_set, state->action_set_len) {
> -                if (a->type == OFPACT_GROUP) {
> -                    ctx.action_set_has_group = true;
> -                    break;
> -                }
> -            }
> +            flow->actset_output = OFPP_UNSET;
> +            xlate_write_actions__(&ctx, state->action_set,
> +                                  state->action_set_len);
>         }
> 
>         /* Restore recirculation actions.  If there are no actions, processing
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev