[Ns-developers] Implementation of an IPv6 stack for NS-3

[Mathieu Lacage]

hi,

On Fri, 2008-06-13 at 09:30 +0200, Sébastien Vincent wrote:

> We are glad to announce the first release of our functional IPv6 stack 
> for ns-3. You can retrieve this implementation from our mercurial 
> repository:

This is all pretty cool so, I had a quick first look at the code below.

> hg clone https://svnet.u-strasbg.fr/hg/ns-3-ipv6/

1) first the obvious, annoying, and painful stuff which I hate
commenting about but which I have to: the coding style
(http://www.nsnam.org/codingstyle.html) recommends that you use the gnu
style. Most notably:
  - no tabs ! use 2-space indents.
  - if statements (and all loop statements)
if (test)
  {
    // code
  }
else
  {
    // code
  }
and not:
if (test)
{
\tab // code
}
else
{
\tab // code
}

I have also seen some automatic tab reindentation in
src/internet-stack/tcp-header.cc and that is not very helpful. Please,
make sure you avoid doing this again.


2) It is not in the coding style but all of our code does it:

namespace ns3 {
// start defining stuff _on the first column_. No indent for namespaces.
} // namespace ns3


3) in ipv6-header.h:
   enum Ipv6NextHeader
   {
//...
   } Ipv6NextHeader;

I am fairly certain that you did not intend to define a member variable
named Ipv6NextHeader. The following would probably be more appropriate:

   enum NextHeader_e
   {
//...
   };

(No need to repeat the Ipv6 prefix because it is implicit here: you
define a member enum of the class Ipv6Header.

4) src/node/icmp-socket.h has serious indentation breakage.

5) src/node/ipv6-address.h
  - I see inline methods: are these really needed ? i.e., did you see
them in performance profiles ?
  - memcmp requires #include <string.h>

6) src/node/ipv6-address.cc

using std::hex;

This is not so ugly and there is no clear-cut obvious answer but I
really think that it would be nicer (i.e., more readable for newcomers
who miss the top of the file and do not know what "hex" could be) to
actually use the fully-qualified name in the code rather than import hex
into your namespace.


> - We try to avoid radical changes in existing classes from ns-3, but 
> here are the major (and required) changes:

Although there is no single almighty rule about this, I think that there
is little point in trying to make ipv6 look like ipv4 if it is really
different. i.e., doing so would just increase the implementation pain
for you, increase the API pain for users and not bring much as positive
things. So, if there are parts of the API where you have worked hard to
make ipv6 fit in the shoes of ipv4, I think that you should consider
giving yourself some slack and try to export instead the best possible
API from the perspective of ipv6.

> 
> # *-net-device
> -> adding methods to get the multicast Ethernet representation for IPv6 
> ("33:33:00:00:00:00")

ok

> # csma-net-device
> -> adding an attribute for setting the MTU (required to test some ICMPv6 
> error)

ok. That attribute should be a uint16_t, not a uint32_t. i.e., 
MakeUintegerChecker<uint16_t> ()
not:
MakeUintegerChecker<uint32_t> ()

> -> adding an EnumValue "IPV6" for attribute EncapsulationMode

I see that the above is really just the same as IP_ARP. Can't we find a
better name for IP_ARP rather than add an IPV6 alias ?

> -> in ::Receive methods, forward up ethernet multicast representation of 
> IPv6 multicast destination (solicited multicast, all-nodes, all-routers 
> and all-hosts)

ok.

> 
> # udp-header and tcp-header
> -> some changes for checksum calculation (required in IPv6) for both 
> IPv6 and IPv4

What is the difference between ipv6-checksum.cc and ipv4-checksum.cc ?
The former looks like a copy of the latter: why did you introduce it ?.

As a side-note, although I don't care a damn here, I should point out
that copying code like this and replacing the original copyright with a
new one could be considered as "copyright infrigement" by the original
copyright holders. So, you probably should be more careful in the
future :)

Looking at this issue more carefully, a nice way to go forward would be
to:
  - move the function Ipv4Header::ChecksumCalculate to the
Buffer::Iterator class as a non-static member method name
CalculateIpChecksum.
  - make the Ipv4Header code use this new method
  - make the udp and tcp header code use this new method in IsChecksumOk
  - re-implement InitializeChecksum in UdpHeader and TcpHeader by
creating a raw temporary instance of a Buffer, call Buffer::Begin (),
and serialize the source, dest, and payload fields, call
Iterator::CalculateIpChecksum
  - remove the UdpHeader::m_ipv6 variable (and the UdpHeader::SetIpv6
method, obviously). The same for TcpHeader.
  - kill ipv4-checksum.* and ipv6-checksum.*

All of the above would be ipv4-material. The only ipv6-specific issue
would be that you eventually need to duplicate InitializeChecksum for
ipv6. It would be nice to get an ipv4-only patch for the above first.

> - We add icmp-socket.cc,h in the node module to provide an abstract ICMP 
> socket (this can be reused to implement ICMPv4 protocol later in ns-3)

ok.

> 
> - We duplicate and adapt TCP and UDP code to provide our IPv6 version of 
> these protocols , we know that it is not the best thing to do. We will 
> discuss about it on the mailling-list later.

Yes, clearly, that is one of the big sticking points. I think that, in
this case, it would make a lot of sense to avoid doing what linux and
BSD did, that is, avoid duplicating _all_ the code. Making the Udp and
Tcp layers independent from the underlying ip layer could be done
without too much pain if you were to define a packet tag which contains
the source and destination ipv4 addresses (and another tag for the
source and destination ipv6 addresses) and remove the Ipv4Address from
all method signatures in the Tcp and Udp layers. That would allow you to
move the ip-specific code to a few very specific functions which would
take a packet, lookup which of the two tags is present, and which would
handle the two cases, ipv4 and ipv6.

Another issue I wonder about is what the socket API looks like. I have
to confess that I know next to nothing about ipv6 so, I would like to
know how OSes actually export ipv6 sockets ? How can I specify (in a
real OS) that I want an ipv6 tcp socket ? Can I specify that I want a
tcp socket to listen both on ipv4 and ipv6 on the same port ?

The last issue I have very briefly looked at is how you handle ipv6
multihoming. Although the current ns-3 ipv4 stack implementation (and,
unfortunately, API too, as discovered recently in a couple of bug
reports) falls short of supporting multihoming, the initial plan was to
support multihoming in ipv4 by allowing the user to define multiple ipv4
interfaces for one netdevice and by setting a _single_ ipv4 address on
each ipv4 interface. Is there any reason why the above would not work
for ipv6 ? (I see that you have instead chosen to manage multiple ipv6
addresses per "ipv6 interface").

All of the above, of course, raises the issue of how we are going to
merge all this cool stuff in ns-3. I think that the most productive
strategy would be to:
  - the net-device.h changes seem pretty straightforward (they would
depend on src/node/ipv6-address.h but that seems also pretty
straightforward)
  - the csma changes also look pretty straightforward
  - handle the tcp and udp checksum changes
  - try to see how the udp and tcp layers can be made independent of
ipv4

Once all of the above is done, merging in ns-3-dev should be really
easy/trivial.

Mathieu