On Wed, Jan 20, 2016 at 03:49:26PM -0800, Jarno Rajahalme wrote:
> Nice simplification! With one bug fix below:
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
Thanks for the review.
> > 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>
...
> > @@ -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.
Good point.
I folded in the following incremental:
diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index f4dc21f..c61b007 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -210,14 +210,12 @@ 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->n_stack) {
- new->stack = xmemdup(new->stack, new->n_stack * sizeof *new->stack);
- }
- if (new->ofpacts) {
- new->ofpacts = (new->ofpacts_len
- ? xmemdup(new->ofpacts, new->ofpacts_len)
- : NULL);
- }
+ new->stack = (new->n_stack
+ ? xmemdup(new->stack, new->n_stack * sizeof *new->stack)
+ : NULL);
+ new->ofpacts = (new->ofpacts_len
+ ? xmemdup(new->ofpacts, new->ofpacts_len)
+ : NULL);
}
static void
With that fixed, I applied this to master. Thanks again.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
