[ovs-dev] [vlans v2 1/5] ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL.

[Pravin Shelar]

On Fri, Sep 23, 2011 at 5:17 PM, Ben Pfaff <blp at nicira.com> wrote:
> compose_actions(), which is part of the OFPP_NORMAL implementation, had
> multiple flaws that this commit corrects.
>
> First, it did not commit changes made to the flow by actions preceding
> the output to OFPP_NORMAL.  This means that, for example, if an OpenFlow
> action to modify an L2 or L3 header preceded the output to OFPP_NORMAL,
> then OFPP_NORMAL would output its packets without those changes.
>
> Second, it did not update the action_xlate_ctx's notion of the VLAN that
> was currently set, in the cases where it modified the current VLAN.  This
> means that actions following the output to OFPP_NORMAL could output to
> unexpected VLANs.
>
> Third, when it switched to VLAN 0, it unconditionally also stripped the
> whole 802.1Q header, so that if the packet originally was priority tagged,
> the output packet was not priority tagged.  This is reasonable behavior,
> but it is a change in behavior from what previous releases of OVS did, so
> this commit reverts it.
>
> Based on a patch from and a conversation with Pravin Shelar
> <pshelar at nicira.com>.
> ---
>  ofproto/ofproto-dpif.c |   56 ++++++++++++++++++++++++++++-------------------
>  1 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 47a9d29..ec5e3be 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2888,6 +2888,26 @@ static void do_xlate_actions(const union ofp_action *in, size_t n_in,
>  static void xlate_normal(struct action_xlate_ctx *);
>
>  static void
> +commit_vlan_tci(struct action_xlate_ctx *ctx, ovs_be16 vlan_tci)
> +{
> +    struct flow *base = &ctx->base_flow;
> +    struct ofpbuf *odp_actions = ctx->odp_actions;
> +
> +    if (base->vlan_tci != vlan_tci) {
> +        if (!(vlan_tci & htons(VLAN_CFI))) {
> +            nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> +        } else {
> +            if (base->vlan_tci != htons(0)) {
> +                nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> +            }
> +            nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
> +                            vlan_tci & ~htons(VLAN_CFI));
> +        }
> +        base->vlan_tci = vlan_tci;
> +    }
> +}
> +
> +static void
>  commit_odp_actions(struct action_xlate_ctx *ctx)
>  {
>     const struct flow *flow = &ctx->flow;
> @@ -2914,18 +2934,7 @@ commit_odp_actions(struct action_xlate_ctx *ctx)
>         base->nw_tos = flow->nw_tos;
>     }
>
> -    if (base->vlan_tci != flow->vlan_tci) {
> -        if (!(flow->vlan_tci & htons(VLAN_CFI))) {
> -            nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> -        } else {
> -            if (base->vlan_tci != htons(0)) {
> -                nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> -            }
> -            nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
> -                            flow->vlan_tci & ~htons(VLAN_CFI));
> -        }
> -        base->vlan_tci = flow->vlan_tci;
> -    }
> +    commit_vlan_tci(ctx, flow->vlan_tci);
>
>     if (base->tp_src != flow->tp_src) {
>         nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_SET_TP_SRC, flow->tp_src);
> @@ -3741,8 +3750,13 @@ compose_actions(struct action_xlate_ctx *ctx, uint16_t vlan,
>     dst_set_init(&set);
>     compose_dsts(ctx, vlan, in_bundle, out_bundle, &set);
>     compose_mirror_dsts(ctx, vlan, in_bundle, &set);
> +    if (!set.n) {
> +        dst_set_free(&set);
> +        return;
> +    }
>
>     /* Output all the packets we can without having to change the VLAN. */
> +    commit_odp_actions(ctx);
>     initial_vlan = vlan_tci_to_vid(ctx->flow.vlan_tci);
>     if (initial_vlan == 0) {
>         initial_vlan = OFP_VLAN_NONE;
> @@ -3762,19 +3776,15 @@ compose_actions(struct action_xlate_ctx *ctx, uint16_t vlan,
>             continue;
>         }
>         if (dst->vlan != cur_vlan) {
> -            if (dst->vlan == OFP_VLAN_NONE) {
> -                nl_msg_put_flag(ctx->odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> -            } else {
> -                ovs_be16 tci;
> +            ovs_be16 tci;
>
> -                if (cur_vlan != OFP_VLAN_NONE) {
> -                     nl_msg_put_flag(ctx->odp_actions, OVS_ACTION_ATTR_POP_VLAN);
> -                }
> -                tci = htons(dst->vlan & VLAN_VID_MASK);
> -                tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
> -                nl_msg_put_be16(ctx->odp_actions,
> -                                OVS_ACTION_ATTR_PUSH_VLAN, tci);
> +            tci = htons(dst->vlan == OFP_VLAN_NONE ? 0 : dst->vlan);
> +            tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
> +            if (tci) {
> +                tci |= htons(VLAN_CFI);
>             }
> +            commit_vlan_tci(ctx, tci);
> +
>             cur_vlan = dst->vlan;
>         }
>         nl_msg_put_u32(ctx->odp_actions,
> --

Looks good.

Thanks,
Pravin.