Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
Thanks for the clean-up, will be easier to maintain this code going forward.
Appreciate the C lessons as well :-)
Jarno
> On Jul 29, 2015, at 11:42 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> Before this commit, mirroring state was not preserved across recirculation,
> which could result in a packet being mirrored to the same destination both
> before and after recirculation. This commit fixes the problem and adds a
> test to avoid regression.
>
> Found by inspection.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif-rid.c | 2 ++
> ofproto/ofproto-dpif-rid.h | 2 ++
> ofproto/ofproto-dpif-xlate.c | 4 ++++
> tests/ofproto-dpif.at | 28 ++++++++++++++++++++++++++++
> 4 files changed, 36 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index e2867b7..b8207c4 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -137,6 +137,7 @@ recirc_metadata_hash(const struct recirc_state *state)
> hash = hash_words64((const uint64_t *) state->stack->data,
> state->stack->size / sizeof(uint64_t), hash);
> }
> + hash = hash_int(state->mirrors, hash);
> hash = hash_int(state->action_set_len, hash);
> if (state->ofpacts_len) {
> hash = hash_words64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> @@ -156,6 +157,7 @@ recirc_metadata_equal(const struct recirc_state *a,
> && (((!a->stack || !a->stack->size) &&
> (!b->stack || !b->stack->size))
> || (a->stack && b->stack && ofpbuf_equal(a->stack, b->stack)))
> + && a->mirrors == b->mirrors
> && a->action_set_len == b->action_set_len
> && ofpacts_equal(a->ofpacts, a->ofpacts_len,
> b->ofpacts, b->ofpacts_len));
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index 9caa662..11ae486 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -23,6 +23,7 @@
> #include "cmap.h"
> #include "list.h"
> #include "ofp-actions.h"
> +#include "ofproto-dpif-mirror.h"
> #include "ovs-thread.h"
>
> struct ofproto_dpif;
> @@ -135,6 +136,7 @@ struct recirc_state {
> struct ofproto_dpif *ofproto; /* Post-recirculation bridge. */
> struct recirc_metadata metadata; /* Flow metadata. */
> struct ofpbuf *stack; /* Stack if any. */
> + mirror_mask_t mirrors; /* Mirrors already output. */
>
> /* Actions to be translated on recirculation. */
> uint32_t action_set_len; /* How much of 'ofpacts' consists of an
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index dc16a39..e2596d9 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3522,6 +3522,7 @@ compose_recirculate_action(struct xlate_ctx *ctx)
> .ofproto = ctx->xbridge->ofproto,
> .metadata = md,
> .stack = &ctx->stack,
> + .mirrors = ctx->mirrors,
> .action_set_len = ctx->recirc_action_offset,
> .ofpacts_len = ctx->action_set.size,
> .ofpacts = ctx->action_set.data,
> @@ -4868,6 +4869,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> ofpbuf_put(&ctx.stack, state->stack->data, state->stack->size);
> }
>
> + /* Restore mirror state. */
> + ctx.mirrors = state->mirrors;
> +
> /* Restore action set, if any. */
> if (state->action_set_len) {
> const struct ofpact *a;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 9bfb794..58c426b 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -4144,6 +4144,34 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow"
> "$actual"], [0], [expout])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# This test verifies that mirror state is preserved across recirculation.
> +#
> +# Otherwise, post-recirculation the ingress and the output to port 4
> +# would cause the packet to be mirrored to port 3 a second time.
> +AT_SETUP([ofproto-dpif - mirroring with recirculation])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], 1, 2, 3, 4)
> +ovs-vsctl \
> + set Bridge br0 mirrors=@m --\
> + --id=@p3 get Port p3 --\
> + --id=@m create Mirror name=mymirror select_all=true output_port=@p3
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=2,debug_recirc,4
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 3,2,recirc(0x1)
> +])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow,recirc_id(1)"
> -generate], [0], [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 4
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> # Two testcases below are for the ofproto/trace command
> # The first one tests all correct syntax:
> # ofproto/trace [dp_name] odp_flow [-generate|packet]
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
