Thanks for the review! Pushed in two separate commits and the for statement restored and backported to branch-2.1 and branch-2.3.
More comments below, Jarno On Jun 11, 2014, at 2:46 PM, Ben Pfaff <[email protected]> wrote: > On Mon, Jun 09, 2014 at 03:06:22PM -0700, Jarno Rajahalme wrote: >> When reaching the end of a prefix trie, we checked one bit off the end >> to the intended data. However, since the trie node in that case has >> NULLs for both edge links, this did not result in incorrect >> functionality. >> >> Found via check-valgrind. >> >> Reported-by: Ben Pfaff <[email protected]> >> Signed-off-by: Jarno Rajahalme <[email protected]> >> --- >> v2: Clarify trie_lookup_value() by returning from the case when ofs >>> = n_bits. > > I think that the only behavioral change here is: > + if (ofs >= n_bits) { > + *checkbits = n_bits; /* Full prefix. */ > + return match_len; > + } > and updating the callers, which seems good. > > Otherwise, I see two stylistic changes: > > 1. s/plen/ofs/. OK. > > 2. Change "for" to "while". This one is a little puzzling > because I don't see a need for it. > It was a remnant from when I break’ed out of the loop instead of returning in the portion you quoted above. It is simpler with a return, so the for statement needs not change. I have committed this with the for statement intact now, thanks for pointing this out! Jarno > I'd be tempted to put the behavioral changes in a patch by themselves > (to make it really obvious what's changing), and the stylistic changes > in another, but I'll leave it up to you. > > Acked-by: Ben Pfaff <[email protected]> _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
