I see that the next patch fixed this bug, Jarno
> On Jan 20, 2016, at 3:49 PM, Jarno Rajahalme <ja...@ovn.org> wrote: > > Nice simplification! With one bug fix below: > > Acked-by: Jarno Rajahalme <ja...@ovn.org <mailto: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
