[ovs-dev] [PATCH v2 3/4] New action "ct_clear".

[Mickey Spiegel]

On Thu, Jan 5, 2017 at 8:58 PM, Ben Pfaff <blp at ovn.org> wrote:

> This is being introduced specifically to allow a user of the "clone" action
> to clear the connection tracking state, but it's implemented as a separate
> action as a matter of clean design and in case another use case arises
> later.
>
> Reported-by: Mickey Spiegel <mickeys.dev at gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> January/326981.html
> Fixes: 7ae62a676d3a ("ofp-actions: Add clone action.")
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>
>

The patch applies now, but I am getting a test failure. See inline for that
plus one other nit.


> ---
>  NEWS                              |  2 +-
>  include/openvswitch/ofp-actions.h |  3 +-
>  lib/ofp-actions.c                 | 43 +++++++++++++++++++++++++++-
>  ofproto/ofproto-dpif-xlate.c      | 17 ++++++++---
>  tests/ofp-actions.at              |  3 ++
>  tests/ofproto-dpif.at             | 60 ++++++++++++++++++++++++++++++
> +++++++++
>  utilities/ovs-ofctl.8.in          |  6 ++++
>  7 files changed, 127 insertions(+), 7 deletions(-)
>
>
<snip>


> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ceeaac6..347dc95 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2874,12 +2874,15 @@ xlate_commit_actions(struct xlate_ctx *ctx)
>  }
>
>  static void
> -clear_conntrack(struct flow *flow)
> +clear_conntrack(struct xlate_ctx *ctx)
>  {
> +    ctx->conntracked = false;
> +
> +    struct flow *flow = &ctx->xin->flow;
>      flow->ct_state = 0;
>      flow->ct_zone = 0;
>      flow->ct_mark = 0;
> -    memset(&flow->ct_label, 0, sizeof flow->ct_label);
> +    flow->ct_label = OVS_U128_ZERO;
>  }
>
>  static bool
> @@ -2979,7 +2982,7 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
>          memset(flow->regs, 0, sizeof flow->regs);
>          flow->actset_output = OFPP_UNSET;
>          ctx->conntracked = false;
>

Since clear_conntrack now sets ctx->conntracked = false, the line above is
unnecessary.


> -        clear_conntrack(flow);
> +        clear_conntrack(ctx);
>
>          /* When the patch port points to a different bridge, then the
> mirrors
>           * for that bridge clearly apply independently to the packet, so
> we
> @@ -4510,6 +4513,7 @@ freeze_unroll_actions(const struct ofpact *a, const
> struct ofpact *end,
>          case OFPACT_CLONE:
>          case OFPACT_DEBUG_RECIRC:
>          case OFPACT_CT:
> +        case OFPACT_CT_CLEAR:
>          case OFPACT_NAT:
>              /* These may not generate PACKET INs. */
>              break;
> @@ -4765,6 +4769,7 @@ recirc_for_mpls(const struct ofpact *a, struct
> xlate_ctx *ctx)
>      case OFPACT_CLONE:
>      case OFPACT_UNROLL_XLATE:
>      case OFPACT_CT:
> +    case OFPACT_CT_CLEAR:
>      case OFPACT_NAT:
>      case OFPACT_DEBUG_RECIRC:
>      case OFPACT_METER:
> @@ -5130,6 +5135,10 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>              compose_conntrack_action(ctx, ofpact_get_CT(a));
>              break;
>
> +        case OFPACT_CT_CLEAR:
> +            clear_conntrack(ctx);
> +            break;
> +
>          case OFPACT_NAT:
>              /* This will be processed by compose_conntrack_action(). */
>              ctx->ct_nat_action = ofpact_get_NAT(a);
> @@ -5516,7 +5525,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
>          xlate_report(&ctx, "- Resuming from table %"PRIu8, ctx.table_id);
>
>          if (!state->conntracked) {
> -            clear_conntrack(flow);
> +            clear_conntrack(&ctx);
>          }
>
>          /* Restore pipeline metadata. May change flow's in_port and other
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index 3881f9f..33d4bea 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -247,6 +247,9 @@ fe800000 00000000 020c 29ff fe88 0001 dnl
>  fe800000 00000000 020c 29ff fe88 a18b dnl
>  00ff1000 00000000
>
> +# actions=ct_clear
> +ffff 0010 00002320 002a 000000000000
> +
>

I am getting a test failure due to this change:

## ---------------------- ##
## Detailed failed tests. ##
## ---------------------- ##

#                             -*- compilation -*-
137. ofp-actions.at:3: testing OpenFlow 1.0 action translation ...
./ofp-actions.at:281: ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions
OpenFlow10 < input.txt
--- expout      2017-01-05 23:43:29.892861427 -0800
+++ /home/mickey/ovs5/ovs/tests/testsuite.dir/at-groups/137/stdout
 2017-01-05 23:43:29.904861427 -0800
@@ -132,7 +132,7 @@

 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))

-actions=ct_clear
+actions=clone(drop)

I changed 002a to 002b and it passed.


>  # actions=output(port=1,max_len=100)
>  ffff 0010 00002320 0027 0001 00000064
>

<snip>

Mickey