> On Jan 4, 2017, at 11:03 PM, Ben Pfaff <[email protected]> wrote: > > 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. >
OK, now i got it. This is just to guard against (future) bugs in OVS itself. >>> 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. I did not change the encoding of the stack as properties, so each value in the stack is still encoded as a separate property, where the (aligned) value length is used as the property length. This causes the decoded stack values to be aligned to 8 bytes, which should not make any functional difference, as the stack is still logically an array of full-size mf_subvalues, only the internal storage format has been changed. Do you think this is a problem? If so, we could also align the internal representation to 8 bytes? ofpprop_pull() does the length checking for the properties and the current code in ofputil_decode_packet_in_private() assert fails on any error, which is not good, as a controller bug would crash OVS? Jarno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
