NS-3 IPv6 support in TCP/UDP (2)

[Tommaso Pecorella, 2011-11-02 00:55:37]

Here are my comments. As I said, great job.

By the way, since we're at it and changing stuff... I noticed that Ipv6addresses
can be tested for various "Is[...]" functions:
IsLocalhost
IsMulticast
IsAllNodesMulticast
IsAllRoutersMulticast
IsAllHostsMulticast
IsLinkLocal
IsSolicitedMulticast
IsAny

we do miss at least one:
IsLinkLocalMulticast
see also RFC4291, section 2.7. Basically ff02:: is a link-local multicast
address, but right now we can't check for this. It should important for routing
tho.

I know it's not the right policy to mix patches, but...

Also, I didn't check if with the patch it's already possible for an UDP or TCP
client to receive ICMPs at application level. I'll do as soon as possible
(probably during weekend).

http://codereview.appspot.com/5311077/diff/1/src/applications/model/udp-echo-...
File src/applications/model/udp-echo-server.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/applications/model/udp-echo-...
src/applications/model/udp-echo-server.cc:115: NS_FATAL_ERROR ("Error: joining
multicast on a non-UDP socket");
This was puzzling even in the original file.
We're opening an UDP socket for sure, so the reason the previous call might fail
is not that we're trying to use a non-UDP socket.
I don't even know if it could fail at all, since we're using a GetAny address.
TBH I'd change this to a simpler "Error: failed to joining multicast group", as
the previous error explanation is misleading.

http://codereview.appspot.com/5311077/diff/1/src/csma-layout/model/csma-star-...
File src/csma-layout/model/csma-star-helper.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/csma-layout/model/csma-star-...
src/csma-layout/model/csma-star-helper.cc:135:
CsmaStarHelper::AssignIpv6Addresses (uint64_t network)
Quite complex. Are you sure you can't use the "plain" Ipv6AddressHelper here ?
It would simplify the code greatly (and it would be much closer to the v4 one)

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv4-l4-pro...
File src/internet/model/icmpv4-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv4-l4-pro...
src/internet/model/icmpv4-l4-protocol.cc:252: {
It would mean an ICMPv4 wrapped into an IPv6 packet. Kinda unlikely.
I'd raise an error here, as this should never be reached.

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv6-l4-pro...
File src/internet/model/icmpv6-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv6-l4-pro...
src/internet/model/icmpv6-l4-protocol.cc:178: enum IpL4Protocol::RxStatus
Icmpv6L4Protocol::Receive (Ptr<Packet> packet, Ipv4Header const &header, 
Ptr<Ipv4Interface> interface)
Same as for the v4 one, this should raise an error.

http://codereview.appspot.com/5311077/diff/1/src/internet/model/tcp-l4-protoc...
File src/internet/model/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/tcp-l4-protoc...
src/internet/model/tcp-l4-protocol.cc:138: }
Why the DownTarget is in one case Ipv4 and in the other case Ipv6L3Protocol ?
Shouldn't it be consistent ?
Worth a comment at least (if it's correct).

[kdrenard2, 2011-11-18 16:44:08]

Could not add a second patch to this issue due to a change in my google account.
 This has been added as a new full patch set with issue number 5417048.

http://codereview.appspot.com/5417048

[sorry for the confusion]

[kdrenard2, 2011-11-18 16:45:12]

Responses to your comments.  Had to create a new issue to update patch set
(google account issue).

Thanks so much for your time and review!

http://codereview.appspot.com/5311077/diff/1/src/applications/model/udp-echo-...
File src/applications/model/udp-echo-server.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/applications/model/udp-echo-...
src/applications/model/udp-echo-server.cc:115: NS_FATAL_ERROR ("Error: joining
multicast on a non-UDP socket");
I changed this error message to "Error joining multicast group".  I agree that
this _should_ never happen in either the IPv4 or IPv6 case.  I guess the test is
concerned with the success of the dynamic cast more than the type of socket.

http://codereview.appspot.com/5311077/diff/1/src/csma-layout/model/csma-star-...
File src/csma-layout/model/csma-star-helper.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/csma-layout/model/csma-star-...
src/csma-layout/model/csma-star-helper.cc:135:
CsmaStarHelper::AssignIpv6Addresses (uint64_t network)
This approach for generating IPv6 addresses is used in a few other examples
(point-to-point-[grid | star | dumbbell].  It is ugly and complex.  I see this
as a place holder for a better method to come with Atishay's
Ipv6AddressGenerator patch (http://codereview.appspot.com/4812043/).

I suggest that a follow-up patch clean this up when both this patch and the
Ipv6Generator patch are integrated.  It should be pretty quick and easy as these
are only in tests/examples.

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv4-l4-pro...
File src/internet/model/icmpv4-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv4-l4-pro...
src/internet/model/icmpv4-l4-protocol.cc:252: {
How about returning IpL4Protocol::RX_ENDPOINT_UNREACH ?

This should never happen and should never be possible, but I would hate to have
a fatal NS3 error given a malformed packet (my security pbackground is kicking
in here :-)

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv6-l4-pro...
File src/internet/model/icmpv6-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/icmpv6-l4-pro...
src/internet/model/icmpv6-l4-protocol.cc:178: enum IpL4Protocol::RxStatus
Icmpv6L4Protocol::Receive (Ptr<Packet> packet, Ipv4Header const &header, 
Ptr<Ipv4Interface> interface)
For completeness, this is the comment from the icmpv4 code:


How about returning IpL4Protocol::RX_ENDPOINT_UNREACH ?

This should never happen and should never be possible, but I would hate to have
a fatal NS3 error given a malformed packet (my security pbackground is kicking
in here :-)

http://codereview.appspot.com/5311077/diff/1/src/internet/model/tcp-l4-protoc...
File src/internet/model/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/5311077/diff/1/src/internet/model/tcp-l4-protoc...
src/internet/model/tcp-l4-protocol.cc:138: }
Due to the fact that the IPv4 and IPv6 Send() functions have different
prototypes, we need to keep these separate.  We keep track of 2 different down
target callbacks [m_downTarget and m_downTarget6].  Depending on whether we are
connected as v4 or v6, we call the appropriate L3 function.