On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote: > > > 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?
That's what I was thinking, yes. > It seems we missed a newline as well.. I didn't test it, so I could easily have missed that. > > 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. It appears to me that if stack->size is greater than 0 but less than the number of bytes indicated by its last byte, then it will corrupt the ofpbuf size and that this will later cause some kind of failure that will be harder to debug than an assertion failure. > > 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. I mean that if the property contains, for example, a single byte with value 0xff, then it's badly formatted because we can pop off a length (255) but then popping off that number of bytes will underflow. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
