IPv6 support in TCP/UDP

[Tom Henderson, 2011-10-12 19:28:26]

This looks very close so it probably is worth trying to merge it soon so that we
get some experience with it before release deadlines.

From a user perspective, it maintains existing APIs well, and seems to save most
code duplication in internet/model/.

I think that the merging (soon) of the IPv6 address generator code from Atishay
can be leveraged here as well.

I had only a few questions related to the need to introduce Bind6() method and
what is the fate of Ipv4L4Protocol and Ipv6L4Protocol classes?

There are a few cases where an attribute value is changed (e.g. UdpClient,
UdpTraceClient, the 'RemoteAddress' attribute).  These should be documented in
CHANGES.html.

My final question to Ken is whether there are any concerns or lingering
questions about this that you have that might cause any concern about merging
this soon?

Thanks,
Tom

http://codereview.appspot.com/5088044/diff/1/src/csma-layout/examples/csma-st...
File src/csma-layout/examples/csma-star.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/csma-layout/examples/csma-st...
src/csma-layout/examples/csma-star.cc:65: uint64_t ipv6AddressBase =
0x2001000000000000;
Note, merge of Atishay's code should obviate the need for you to come up with
solutions like the above.

http://codereview.appspot.com/4812043/

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ip-l4-protocol.h
File src/internet/model/ip-l4-protocol.h (right):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ip-l4-protoco...
src/internet/model/ip-l4-protocol.h:44: * This is an abstract base class for
layer four protocols which use IPv4 as
the above comment is out of date (IPv4)

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...
File src/internet/model/ipv4-raw-socket-impl.h (right):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...
src/internet/model/ipv4-raw-socket-impl.h:30: virtual int Bind6 ();
why is Bind6 needed?

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv6-l4-proto...
File src/internet/model/ipv6-l4-protocol.h (left):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv6-l4-proto...
src/internet/model/ipv6-l4-protocol.h:77: * \param incomingInterface the
Ipv6Interface on which the packet arrived
are Ipv6L4Protocol and Ipv4L4Protocol used anymore, or can they be deleted?

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-socket-ba...
File src/internet/model/tcp-socket-base.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-socket-ba...
src/internet/model/tcp-socket-base.cc:1045: // Does this really do anything?
seemingly not; suggest to test if it can be deleted.

[kdrenard, 2011-10-14 14:39:36]

Thanks for the comments and review! I will do some quick testing to see if some
of these things can be cleaned up. As far as Ipv4L4Protocol and Ipv6L4Protocol
go, they can be deprecated. I will try removing those files to see if anything
breaks. 

Will hopefully post a new patch set shortly. 

Thanks again! 

-Ken 

----- Original Message -----
From: "tomh org" <tomh.org@gmail.com> 
To: kdrenard@comcast.net 
Cc: ns-3-reviews@googlegroups.com, reply@codereview.appspotmail.com 
Sent: Wednesday, October 12, 2011 3:28:26 PM 
Subject: IPv6 support in TCP/UDP (issue 5088044) 

This looks very close so it probably is worth trying to merge it soon so 
that we get some experience with it before release deadlines. 

From a user perspective, it maintains existing APIs well, and seems to 
save most code duplication in internet/model/. 

I think that the merging (soon) of the IPv6 address generator code from 
Atishay can be leveraged here as well. 

I had only a few questions related to the need to introduce Bind6() 
method and what is the fate of Ipv4L4Protocol and Ipv6L4Protocol 
classes? 

There are a few cases where an attribute value is changed (e.g. 
UdpClient, UdpTraceClient, the 'RemoteAddress' attribute). These should 
be documented in CHANGES.html. 

My final question to Ken is whether there are any concerns or lingering 
questions about this that you have that might cause any concern about 
merging this soon? 

Thanks, 
Tom 


http://codereview.appspot.com/5088044/diff/1/src/csma-layout/examples/csma-st...

File src/csma-layout/examples/csma-star.cc (right): 

http://codereview.appspot.com/5088044/diff/1/src/csma-layout/examples/csma-st...

src/csma-layout/examples/csma-star.cc:65: uint64_t ipv6AddressBase = 
0x2001000000000000; 
Note, merge of Atishay's code should obviate the need for you to come up 
with solutions like the above. 

http://codereview.appspot.com/4812043/ 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ip-l4-protocol.h

File src/internet/model/ip-l4-protocol.h (right): 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ip-l4-protoco...

src/internet/model/ip-l4-protocol.h:44: * This is an abstract base class 
for layer four protocols which use IPv4 as 
the above comment is out of date (IPv4) 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...

File src/internet/model/ipv4-raw-socket-impl.h (right): 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...

src/internet/model/ipv4-raw-socket-impl.h:30: virtual int Bind6 (); 
why is Bind6 needed? 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv6-l4-proto...

File src/internet/model/ipv6-l4-protocol.h (left): 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv6-l4-proto...

src/internet/model/ipv6-l4-protocol.h:77: * \param incomingInterface the 
Ipv6Interface on which the packet arrived 
are Ipv6L4Protocol and Ipv4L4Protocol used anymore, or can they be 
deleted? 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-socket-ba...

File src/internet/model/tcp-socket-base.cc (right): 

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-socket-ba...

src/internet/model/tcp-socket-base.cc:1045: // Does this really do 
anything? 
seemingly not; suggest to test if it can be deleted. 

http://codereview.appspot.com/5088044/ 

[John Abraham, 2011-10-24 17:09:42]

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-header.cc
File src/internet/model/tcp-header.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/tcp-header.cc...
src/internet/model/tcp-header.cc:155: buf.AddAtStart ((2 * Address::MAX_SIZE) +
8);
Can you add a comment on how you arrived at (2 * Max) + 8 ?
For efficiency,we could have a variable uint32_t sz = (2 * Address::MAX_SIZE) +
8 and use sz for the two statements.

[John Abraham, 2011-10-25 20:01:07]

I think overall , it is looking good. Just added a few minor comments. There
were the occasional "(" instead of " (".

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...
File src/internet/model/ipv4-raw-socket-impl.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/internet/model/ipv4-raw-sock...
src/internet/model/ipv4-raw-socket-impl.cc:117: Ipv4RawSocketImpl::Bind6 (void)
Should this function be in ipv6-raw-socket-impl.cc?

http://codereview.appspot.com/5088044/diff/1/src/netanim/examples/star-animat...
File src/netanim/examples/star-animation.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/netanim/examples/star-animat...
src/netanim/examples/star-animation.cc:61: uint64_t ipv6AddressBase =
0x2001000000000000;
is there a better way to enter ipv6 addresses?

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-dumbbell.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-dumbbell.cc:160: #define
UINT64_TO_IPV6_PREFIX(_network, _ret_address) \
i feel this ought to be moved to a more generic module , such as ipv6 address
helpers

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-grid.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-grid.cc:141: } \
this macro is too generic to be placed in this file. possibly move to ipv6
address  helpers?

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
File src/point-to-point-layout/model/point-to-point-star.cc (right):

http://codereview.appspot.com/5088044/diff/1/src/point-to-point-layout/model/...
src/point-to-point-layout/model/point-to-point-star.cc:131:
UINT64_TO_IPV6_PREFIX(network, v6addr);
a space before "(" preferably

http://codereview.appspot.com/5088044/diff/6001/src/network/utils/ipv6-addres...
File src/network/utils/ipv6-address.cc (right):

http://codereview.appspot.com/5088044/diff/6001/src/network/utils/ipv6-addres...
src/network/utils/ipv6-address.cc:316: Serialize(buf);
Serialize (buf) instead of Serialize(buf)

[kdrenard, 2011-10-28 12:32:50]

On 2011/10/25 20:01:07, johnjoe7 wrote:
> I think overall , it is looking good. Just added a few minor comments. There
> were the occasional "(" instead of " (".
> 
[...]

Thanks for the review and comments!

1.  I have done some cleanup with respect to spaces before "()".  Thanks for
catching this.

2.  As far as some of the IPv6 address ugliness in the example programs, I would
like to revisit these areas when the Ipv6Address changes are integrated and
clean them up.

3.  I have taken out "ipv6-l4-protocol.*" and "ipv4-l4-protocol.*" which
uncovered the dependency in the nsc-tcp stuff.  I have switched ncs-tcp over to
the new ip-l4-protocol and given it a quick test, but have not done any IPv6
work with nsc-tcp [just place holders to implement virtual methods of
ip-l4-protocol].

Thanks again for your help!

-Ken

[Tommaso Pecorella, 2011-10-31 14:36:58]

Great job. I still have to check it fully, but it seems great.

Just two things.
1) can you post a "global" patch ? So we don't have to check in / review all the
3 ones differently.
2) in icmpv6-l4-protocol.cc there is a "m_downTarget" callback. Great (plus I
totally need it for other purposes), but... it's never used in the code !

Could you mimic the behavior of icmpv4 ? Well, it's more a mild request,
otherwise some apps might not work.

Cheers,

Tommaso

PS: as soon as I'll have a global patch I'll review the whole code, but don't
wait my +1 if John and Tom gave their ok. I completely trust them.

[kdrenard, 2011-11-01 14:44:45]

Thanks, Tommaso!

I have combined these 3 patches and the icmpv6-l4-protocol update as Issue
5311077.

http://codereview.appspot.com/5311077

-Ken

[Tommaso Pecorella, 2011-11-01 19:47:59]

On 2011/11/01 14:44:45, kdrenard wrote:
> Thanks, Tommaso!
> 
> I have combined these 3 patches and the icmpv6-l4-protocol update as Issue
> 5311077.
> 
> http://codereview.appspot.com/5311077
> 
> -Ken

Thank you very much, I'll check it all as soon as possible.

By the way, in the previous comment I pointed out the wrong thing. The ICMP
"feature" that was present in v4 and not in v6 is the one allowing this:

      CallbackValue cbValue = MakeCallback(&MyClass::HandleReadIcmp, this);
      m_socketClient->SetAttribute ("IcmpCallback", cbValue);

actually the code is quite counterintuitive, so I fear we'll have to address
this in another patch. It would be nice to address it in one go tho. Otherwise
it could lead to "strange" things for some cases.

Tommaso