[Ns-developers] Checksum patch

[Mathieu Lacage]

hi sebastien,

Thanks for this new patch. Comments inline below.

On Wed, 2008-06-25 at 15:51 +0200, Sébastien Vincent wrote:

> I propose a patch that do this stuff. The code is located at
> https://svnet.u-strasbg.fr/hg/ns-3-checksum (it is based on last
> ns-3-dev).

Are you sure that this works ? Did you test it ?
+  Config::SetDefault("ns3::UdpHeader::CalcChecksum", BooleanValue("true"));


@@ -130,8 +136,52 @@ TcpHeader::InitializeChecksum (Ipv4Addre
                                    Ipv4Address destination,
                                    uint8_t protocol)
 {
-  m_checksum = 0;
-//XXX requires peeking into IP to get length of the TCP segment
+  Buffer buf(12);

             ^^^^^
Please, use this syntax instead:

Buffer buf = Buffer (12);

It is exactly the same from the point of view of the compiler and is
much more readable (to me, at least).

Instead of using a default value:

+      uint16_t CalculateIpChecksum(uint16_t size, uint32_t initialChecksum = 0);

can you instead provide two overloaded methods ?

@@ -208,7 +266,7 @@ uint32_t TcpHeader::Deserialize (Buffer:
   m_flags = field & 0x3F;
   m_length = field>>12;
   m_windowSize = start.ReadNtohU16 ();
-  m_checksum = start.ReadNtohU16 ();
+  m_checksum = start.ReadU16 ();

     ^^^^^^^^^^^^^^^^
Are you sure that this ReadU16 is correct ? I see you have a matching
WriteU16 above in Serialize but, did you test this code on a non-x86
box ?

in TcpL4Protocol::Receive, the code would be probably easier to write if
we provided a Packet::PeekHeader function. I will add this to ns-3-dev
after the 3.1 release.

@@ -25,7 +27,7 @@ namespace ns3 {
 
 NS_OBJECT_ENSURE_REGISTERED (UdpHeader);
 
-bool UdpHeader::m_calcChecksum = false;
+/* bool UdpHeader::m_calcChecksum = false; */
 
Instead of commenting out code, delete it if it is unused.

Rather than use:
bool UdpHeader::IsChecksumOk(const uint8_t* payload) const;
bool TcpHeader::IsChecksumOk(const uint8_t* payload) const;

please, do this:
bool UdpHeader::IsChecksumOk(void) const;
bool TcpHeader::IsChecksumOk(void) const;
and make both methods return a bool m_goodChecksum and set
m_goodChecksum from TcpHeader::Deserialize and UdpHeader::Deserialize.

I do not understand the changes to Ipv4Header::Deserialize: the code did
not work before ? If so, can you point me to a testcase ?


Finally, I think that we really do not want to enable checksum
calculation in tcp by default, even if the standard requires it. So, can
you also add a flag there which is disabled by default ? 

The implementation of that flag should be inspired by the Ipv4L3Protocol
checksum flag, that is: add an attribute flag to TcpL4Protocol and
UdpL4Protocol and use it to call TcpHeader::EnableChecksum and
UdpHeader::EnableChecksum

regards,
Mathieu