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

[Sébastien Vincent]

Hi,

OK, I will see it after my holidays.

Regards,
--
Sebastien

Mathieu Lacage a écrit :
> 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
>>>
>>>       
>
>
>