[ovs-dev] [PATCH 2/4] dpif-netdev: Add SCTP support

[Ben Pfaff]

On Tue, Oct 02, 2012 at 12:44:53AM +1300, Joe Stringer wrote:
> On 28 September 2012 04:53, Ben Pfaff <blp at 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.