<div class="gmail_quote">On Wed, Apr 21, 2010 at 12:16 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com">blp@nicira.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Tue, Apr 20, 2010 at 05:22:01PM -0400, Jesse Gross wrote:<br>
> The translation of fragmentation-needed messages from outside the<br>
> tunnel to inside didn't quite make the transition from the old<br>
> GRE implementation to the new one intact. This fixes a number of<br>
> minor bugs in the implmentation. The primary issues are with computing<br>
> the tunnel header length and comparing the input vs. output values<br>
> for tunnel parameters such as the key.<br>
<br>
</div>How thoroughly do you recommend that I should review this? I looked it<br>
over fairly quickly and didn't see anything that looked bad, but I could<br>
spend some more time on it, if you think I should, and increase my<br>
understanding.<br>
<br></blockquote><div><br></div><div>More than anything this is just a cleanup patch. I tested this functionality before I pushed the big changeset, saw that it was broken but didn't have disastrous consequences and decided to wait. None of the logic here is new - it's all brought over from my path MTU implementation for Linux GRE devices. I believe all the PMTUD stuff works now with this patch so hopefully there won't be too much more in this area.</div>
<div><br></div><div>Obviously you are welcome to review it if you want but if you don't see anything obviously stupid it's probably unnecessary.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
I did notice one place where code was written like this:<br>
if ((flags & A) && (flags & B))<br>
Usually I would write this as:<br>
if ((flags & (A|B)) == (A|B))<br>
Maybe you did it that way because the "flags" and "A" and "B" are all<br>
pretty long and would cause the line to wrap in an ugly way. I suspect<br>
that GCC generates the same code either way, so it probably makes sense<br>
to leave it alone, but I did think it was at least worth pointing out.<br></blockquote><div><br></div><div>Yeah, it's really hard to read if you write it the other way. None of this stuff is performance critical anyways.</div>
</div><br>