Nice simplification! With one bug fix below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> In my opinion, this makes better sense for the stack, because it's not
> a packet or a collection of bytes, it's an array of struct mf_subvalue.
> (I left it as an ofpbuf for accumulating stack entries during
> translation, because the automatic reallocation and especially the stub
> support there is helpful.)
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> ofproto/ofproto-dpif-rid.c | 17 ++++++++---------
> ofproto/ofproto-dpif-rid.h | 3 ++-
> ofproto/ofproto-dpif-xlate.c | 6 ++++--
> 3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index cb00301..f4dc21f 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -142,9 +142,9 @@ recirc_metadata_hash(const struct recirc_state *state)
> hash = hash_bytes64((const uint64_t *) &state->metadata.metadata,
> sizeof state->metadata - sizeof
> state->metadata.tunnel,
> hash);
> - if (state->stack && state->stack->size != 0) {
> - hash = hash_bytes64((const uint64_t *) state->stack->data,
> - state->stack->size, hash);
> + if (state->stack && state->n_stack) {
> + hash = hash_bytes64((const uint64_t *) state->stack,
> + state->n_stack * sizeof *state->stack, hash);
> }
> hash = hash_int(state->mirrors, hash);
> hash = hash_int(state->action_set_len, hash);
> @@ -164,9 +164,8 @@ recirc_metadata_equal(const struct recirc_state *a,
> && flow_tnl_equal(a->metadata.tunnel, b->metadata.tunnel)
> && !memcmp(&a->metadata.metadata, &b->metadata.metadata,
> sizeof a->metadata - sizeof a->metadata.tunnel)
> - && (((!a->stack || !a->stack->size) &&
> - (!b->stack || !b->stack->size))
> - || (a->stack && b->stack && ofpbuf_equal(a->stack,
> b->stack)))
> + && a->n_stack == b->n_stack
> + && !memcmp(a->stack, b->stack, a->n_stack * sizeof *a->stack)
> && a->mirrors == b->mirrors
> && a->conntracked == b->conntracked
> && a->action_set_len == b->action_set_len
> @@ -211,8 +210,8 @@ recirc_state_clone(struct recirc_state *new, const struct
> recirc_state *old,
> flow_tnl_copy__(tunnel, old->metadata.tunnel);
> new->metadata.tunnel = tunnel;
>
> - if (new->stack) {
> - new->stack = new->stack->size ? ofpbuf_clone(new->stack) : NULL;
> + if (new->n_stack) {
> + new->stack = xmemdup(new->stack, new->n_stack * sizeof *new->stack);
I needed to do this instead to avoid a core dump in test 860 (MPLS):
+ new->stack = new->n_stack
+ ? xmemdup(new->stack, new->n_stack * sizeof *new->stack) : NULL;
The reason is that the input stack pointer may be non-NULL (pointing to a
stubbed ofpbuf’s data), while n_stack is 0, so we need to rewrite the stack
pointer with NULL in that case so that calling free() on it is safe.
> }
> if (new->ofpacts) {
> new->ofpacts = (new->ofpacts_len
> @@ -224,7 +223,7 @@ recirc_state_clone(struct recirc_state *new, const struct
> recirc_state *old,
> static void
> recirc_state_free(struct recirc_state *state)
> {
> - ofpbuf_delete(state->stack);
> + free(state->stack);
> free(state->ofpacts);
> }
>
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index 4bbc7bf..101bd6e 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -140,7 +140,8 @@ struct recirc_state {
> /* Pipeline context for post-recirculation processing. */
> struct ofproto_dpif *ofproto; /* Post-recirculation bridge. */
> struct recirc_metadata metadata; /* Flow metadata. */
> - struct ofpbuf *stack; /* Stack if any. */
> + union mf_subvalue *stack; /* Stack if any. */
> + size_t n_stack;
> mirror_mask_t mirrors; /* Mirrors already output. */
> bool conntracked; /* Conntrack occurred prior to recirc. */
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 2ee647b..6d67e91 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3626,7 +3626,8 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> uint8_t table)
> .table_id = table,
> .ofproto = ctx->xbridge->ofproto,
> .metadata = md,
> - .stack = &ctx->stack,
> + .stack = ctx->stack.data,
> + .n_stack = ctx->stack.size / sizeof(union mf_subvalue),
> .mirrors = ctx->mirrors,
> .conntracked = ctx->conntracked,
> .action_set_len = ctx->recirc_action_offset,
> @@ -5163,7 +5164,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
>
> /* Restore stack, if any. */
> if (state->stack) {
> - ofpbuf_put(&ctx.stack, state->stack->data, state->stack->size);
> + ofpbuf_put(&ctx.stack, state->stack,
> + state->n_stack * sizeof *state->stack);
> }
>
> /* Restore mirror state. */
> --
> 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
