Looks good.
Ethan
On Thu, Oct 13, 2011 at 11:10, Ben Pfaff <b...@nicira.com> wrote:
> This is a nontrivial cross-port of commit 823518f1f "ofproto-dpif: Fix VLAN
> and other field handling in OFPP_NORMAL" from "master" to "branch-1.2".
>
> 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.
> ---
> I have not yet tested this, beyond the unit tests. I will ask our
> QA department to run their tests on it before I push it to branch-1.2.
>
> ofproto/ofproto-dpif.c | 47 ++++++++++++++++++++++++++++++-----------------
> 1 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2d0b83c..d5e276c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2801,6 +2801,23 @@ 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, ODP_ACTION_ATTR_STRIP_VLAN);
> + } else {
> + nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI,
> + 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;
> @@ -2827,15 +2844,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, ODP_ACTION_ATTR_STRIP_VLAN);
> - } else {
> - nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI,
> - 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, ODP_ACTION_ATTR_SET_TP_SRC,
> flow->tp_src);
> @@ -3561,8 +3570,12 @@ 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) {
> + 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;
> @@ -3582,15 +3595,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,
> ODP_ACTION_ATTR_STRIP_VLAN);
> - } else {
> - ovs_be16 tci;
> - tci = htons(dst->vlan & VLAN_VID_MASK);
> - tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
> - nl_msg_put_be16(ctx->odp_actions,
> - ODP_ACTION_ATTR_SET_DL_TCI, tci);
> + ovs_be16 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,
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
