Thanks for the review, Ben. This looks like good reference for the next version.
I'm putting this on the shelf for the moment, due to other priorities. I hope to return early November. On 2 October 2012 06:15, Ben Pfaff <b...@nicira.com> wrote: > On Tue, Oct 02, 2012 at 12:44:53AM +1300, Joe Stringer wrote: >> On 28 September 2012 04:53, Ben Pfaff <b...@nicira.com> wrote: >> > "sparse" reports a potential byte-swapping error: >> >> I would imagine this is because the CRC32C implementation I introduced >> in patch #1 purports to calculate checksums in HBO. Perhaps that >> should be modified to use ovs_be32? > > If the CRC32C implementation returns a network-byte-order value, then > yes it should return it as an ovs_be32. > >> > It looks to me like the SCTP checksum is getting computed across the >> > entire packet, including the Ethernet header. I doubt that that can >> > be correct. Can you take a look? >> >> I wasn't 100% confident that I understood how the function was >> interacting with the ofpbuf struct, so it's likely I've simply got it >> wrong. With a fresh look it seems like I should actually be using *sh >> to calculate the checksum. I'll make the modification for round two. > > "*sh"? > > I'd guess that the correct place to start the checksum is the l4 > pointer, but I haven't read the SCTP spec. > >> Do we currently have code that triggers this? I take it that this type >> of recalculation is required when we modify the packet, eg set-field >> action. What I'd like to know is how I might test that the behaviour >> is correct. > > There's no test code right now that modifies the checksum of an SCTP > packet. I'd suggest using set-field actions of the SCTP source and > dest ports to trigger it. > > You could use a tool like Wireshark to verify that the modified packet > was correct. > > Once you have manually constructed a test case that you know is > correct, we should add that to the unit tests so that it stays > correct. There are existing tests that run packets through the > switch; you should be able to follow that model. > >> One idea perhaps would be to place OVS between two hosts and make it >> change the destination port on flows in one direction, then using >> lksctp, see whether the hosts continue to communicate correctly. This >> has been my general approach to testing this patchset. The downside is >> that it's a lot of overhead to set up, unlike the autotests. > > That's a good thing to try once, as part of constructing an automatic > test case. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
