[Ns-developers] Checksum patch

[Mathieu Lacage]

to follow up on this issue, sebastien and I did a couple more review
iterations on irc and he came up with a final version still stored in
https://svnet.u-strasbg.fr/hg/ns-3-checksum

I would like to merge that final patchset in ns-3-dev as soon as ns-3.1
is released. If there are no comments, I will proceed with that when 3.1
happens.

Mathieu


On Wed, 2008-06-25 at 10:38 -0700, Mathieu Lacage wrote:
> 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
>