On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
> Always storing the maximum mf_value size wastes about 120 bytes for
> each stack entry.  This patch changes the stack from an mf_value array
> to a string of value-length pairs.
> 
> The lenght is stored after the value so that the stack pop may first

s/lenght/length/

> read the length and then the appropriate number of bytes.  Iterating
> the stack in the reverse order is not possible, so some code needs to
> reverse the stack first to then traverse the stack.  This could be
> avoided by storing the length of the value both before and after it.
> 
> Signed-off-by: Jarno Rajahalme <[email protected]>

I don't think it's worth reversing the stack to serialize for
continuations.  This is part of the "private" part of the continuation
that users are not supposed to look at, so we can put it in any order we
like.  Also, for the sake of printing it for debugging purposes, we can
also print it in any order we like; starting from top-of-stack is not a
problem, although it might be nice to put (top) and (bottom) indicators
in the output.

I'd be more comfortable if nx_stack_pop() had assertions to check for
underflow.

In ofputil_decode_packet_in_private(), it's probably worth checking the
format of the stack we pull from the payload, since a badly formatted
stack can segfault us (if we leave out assertions) or assert-fail us (if
we include them).

It'd be nice to document the stack format somewhere in a comment.

Thanks,

Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to