[Ns-developers] NS-3 UDP / TCP refactoring

[Mathieu Lacage]

hi sebastien,

I see that no one has replied to this second email, so, I gave your repo
another look and I have to confess that, at first, I was a bit scared
so, I took a step back and tried to figure out what we are trying to
achieve, and what really matters. I hope you won't find this too painful
since I don't know much about ipv6 but, the following will hopefully
make it easier to go forward.

I) the socket API
-----------------

1) The primary means of interaction is AF_INET6 and PF_INET6, both of
which replace AF_INET and PF_INET in socket function calls. So, we can
write the following:
  socket (PF_INET, SOCK_DGRAM, 0); // udp/ipv4
  socket (PF_INET, SOCK_STREAM, 0); // tcp/ipv4
  socket (PF_INET6, SOCK_DGRAM, 0); // udp/ipv6
  socket (PF_INET6, SOCK_STREAM, 0); // tcp/ipv6
  socket (PF_INET, SOCK_RAW, proto); // proto/ipv4
  socket (PF_INET6, SOCK_RAW, proto); // proto/ipv6
  socket (PF_INET, SOCK_RAW, IPPROTO_RAW); // raw/ipv4
  socket (PF_INET6, SOCK_RAW, IPPROTO_RAW); // raw/ipv6

2) a secondary means of interaction is through the so-called ipv4
compatibility:
  socket (PF_INET6, SOCK_DGRAM, 0); // udp/ipv6
  socket (PF_INET6, SOCK_STREAM, 0); // tcp/ipv6

  But we use an ipv4 address to for connect or bind.

You obviously care about supporting both kinds of interactions so, it is
important for you that ipv6 sockets actually support ipv4 addresses.

Is the above correct ?


II) How is the socket function mapped in ns-3 terms ?
-----------------------------------------------------

In ns-3, sockets are created with subclasses of the SocketFactory base
class. Each instance of a SocketFactory creates one type of socket and,
we get a socket factory by writing:
Ptr<Socket> socket = Socket::CreateSocket (node, socketType);

How can we encode the type of socket to create in the socketType ?
We discussed that briefly in our first thread on that topic in june and
I think that the version we seemed to converge upon is to map pairs
family+type to a single string:
  - PF_INET+SOCK_DGRAM = "ns3::Ipv4UdpSocketFactory"
  - PF_INET+SOCK_STREAM = "ns3::Ipv4TcpSocketFactory"
  - PF_INET6+SOCK_DGRAM = "ns3::Ipv6UdpSocketFactory"
  - PF_INET6+SOCK_STREAM = "ns3::Ipv6TcpSocketFactory"

For compatibility, we could easily rename "ns3::Ipv4UdpSocketFactory" to
"ns3::UdpSocketFactory" as an exception but I have to confess that I
don't find this especially exciting.

What the above does not solve, though, is what happens of the protocol
argument which, unfortunately, will be needed if you want to be able to
create ICMP sockets. We could keep extending the above proposal:
  - PF_INET+SOCK_RAW+IPPROTO_ICMP = "ns3::Ipv4RawIcmpSocketFactory"
  - PF_INET6+SOCK_RAW+IPPROTO_ICMP = "ns3::Ipv6RawIcmpSocketFactory"
  - etc.

So, what all this means is that we need a large set of SocketFactory
subclasses for each of these combinations. This, of course, does not
mandate you to implement one type of socket for each combination: you
could conceivably implement this scheme by reusing the same socket type
in multiple socket factories and setting a special bit before returning
the socket to the caller. This brings me to the next part of this email,
the implementation of that scheme.

III) How do we implement all this ?
-----------------------------------

The most obvious implementation is the one you did originally:
copy/paste everything, replace ipv4 by ipv6, et voila: you are done.
Now, clearly, there is a pretty strong motivating factor to at least be
able to reuse the tcp implementation. The biggest question I have here
is whether or not it makes sense to attempt to reuse more than the tcp
implementation and I think that the answer to that question is going to
drive a lot of decisions on how you implement this. I have tried to
figure out what was your answer to that question but I am not sure. I
think that you are trying to reuse much more but the result is not
really overwhelming because you seem to end up duplicating a lot of
important functions with Ipv4 and Ipv6 variants in the Demux for
example, but in a couple of other locations.

So, I personally don't have any strong opinion about this but, I think
that it would be really helpful if you could comment on the above to
explain what you think the goal is.

Now, that I went through all these abstract considerations, here are a
couple of comments on the actual patchset:


1) I find it really weird that you have if (ipv4) else if (ipv6)
statements both in the caller and the callee of TcpHeader. i.e., either
TcpHeader should not have if (ipv4) else (ipv6) statements or the
following should be moved down to TcpHeader::InitializeChecksum and its
signature should be changed to accept Address instead of Ipv4Address.

code taken from tcp-l4-protocol.cc:

- tcpHeader.InitializeChecksum (source, destination, PROT_NUMBER); 
+    if(Ipv4Address::IsMatchingType(source))
+    {
+      tcpHeader.InitializeChecksum (Ipv4Address::ConvertFrom(source), Ipv4Address::ConvertFrom(destination), PROT_NUMBER);
+    }
+    /* add IPv6 code here */
   }

I would tend towards changing the signature of InitializeChecksum to
take Address instead of Ipv4Address.


2) The same questions holds in code like that:

-  Ipv4EndPointDemux::EndPoints endPoints =
-    m_endPoints->Lookup (destination, tcpHeader.GetDestinationPort (),
-                         source, tcpHeader.GetSourcePort (),incomingInterface);
+  EndPointDemux::EndPoints endPoints;
+
+  if(Ipv4Address::IsMatchingType(source))
+  {
+    endPoints = m_endPoints->Lookup (Ipv4Address::ConvertFrom(destination), tcpHeader.GetDestinationPort (),
+                  Ipv4Address::ConvertFrom(source), tcpHeader.GetSourcePort (), cidr);
+  }
+  /* add IPv6 code here */
+

I would tend to say that if you rename Ipv4EndPointDemux to
EndPointDemux, its methods should take Address arguments, and not
Ipv4Address arguments so, the above should be rewritten to move the
IsMatchingType down to the implementation of the Lookup method or,
better, maybe make the implementation of EndPointDemux not know anything
about Ipv4 vs Ipv6. I don't believe that this would be really possible
for Lookup: you will need to demultiplex Lookup into LookupV4 and
LookupV6 but it might well be possible for Allocate* methods if you
remove Allocate(void) and Allocate(uint16_t port)). At this point,
though, you will have to wonder how much code you are really sharing and
figure out whether it makes much sense to use a single shared class or
if you could not simply duplicate Ipv4EndPointDemux into
Ipv6EndPointDemux.

I would tend to do the duplication in this case.

I went through the rest of your patch too quickly to give more comments,
but my gut feeling is that it could be much more productive and easy to
just duplicate everything but the TCP code and simply add some shielding
to the TCP code to make it address-independent. So, at this point, what
I would do is try to give a long hard look at the TCP code to figure out
where it is address-dependent and how we can split this out of it.

I honestly don't really know what the answer is here but I thought you
might find the above helpful.

Mathieu

On Wed, 2008-07-30 at 17:18 +0200, Sébastien Vincent wrote: 
> Sébastien Vincent wrote:
> > Hello,
> >
> > I begin to work on UDP / TCP that could manage both IPv6 and IPv4.
> >
> > My repository is at https://svnet.u-strasbg.fr/hg/ns-3-l4/
> >
> > It is based on ns-3-dev.
> >
> > The changeset 3494:79fd6df66ee6 is the preparation of UdpHeader and 
> > TcpHeader for IPv6 support. The things I have done are :
> > - change Ipv4Address (m_source and m_destination) to Address;
> > - Refactor CalculateHeaderChecksum by testing the m_source type 
> > address and apply the checksum calculation.
> >
> > For IPv6 version the only things to do are to add  in 
> > Udp/TcpHeader::CalculateHeaderChecksum () :
> > "else if(Ipv6Address::IsMatchingAddress(m_source)..." and the specific 
> > IPv6 checksum calculation.
> >
> > Now I will look more specifically in *L4Protocol what things I can do.
> >
> 
> I have setup a preparation for the IPv6 support in *L4Protocol. Tell me 
> if there are too much of "if(IsMatchingType(ipv4) ... else".
> For each part I leave a comment of where put specific IPv6 code.
> 
> I have done something for the replacement of the incomingInterface 
> parameter in *L4Protocol::Receive(). In facts for directed broadcast 
> (like in simple-point-to-point-olsr) we need to have at least the 
> network mask of the receiver IPv4 address. I think it is not the best 
> solution but we remove a class dependency (Ptr<Ipv4Interface>), maybe 
> replace this by a CidrSocketTag.
> 
> I have updated the repository (ns-3-l4), so feedbacks is welcome.
> 
> 
> > Regards,
> > -- 
> > Sebastien
> >
>