> On Dec 21, 2016, at 2:36 PM, Ben Pfaff <[email protected]> wrote: > > 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/ >
Fixed, thanks! >> 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. You mean literally “(top)” in the beginning and “(bottom)” at the end, right? It seems we missed a newline as well.. > > I'd be more comfortable if nx_stack_pop() had assertions to check for > underflow. > I don’t think OVS should assert fail if controller issues one pop too many? Do you mean that current users of nx_stack_pop() do not check for NULL return? I had a look and think that setting “*bytes” to zero when returning NULL should be enough for all users. > 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). > What do you mean with badly formatted stack? Zero-sized property? IMO even that would be properly pushed and popped from the stack, storing only the length (of zero) in the stack. > It'd be nice to document the stack format somewhere in a comment. Added a comment to nx-match.c. I just sent a v2, thanks for the review! Jarno > > Thanks, > > Ben. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
