Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1917)

Issue 4812043: NSoC Code Review Request: Ipv6 Address Generator

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by atishayjain25
Modified:
6 years, 2 months ago
Reviewers:
Tom Henderson, Chris K
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 9

Patch Set 2 : Patch set updated as per review comments #

Total comments: 13

Patch Set 3 : Patch set updated as per review comments 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1015 lines, -2 lines) Patch
M src/internet/helper/ipv6-routing-helper.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
A src/internet/model/ipv6-address-generator.h View 1 2 1 chunk +110 lines, -0 lines 0 comments Download
A src/internet/model/ipv6-address-generator.cc View 1 2 1 chunk +589 lines, -0 lines 0 comments Download
A src/internet/test/ipv6-address-generator-test-suite.cc View 1 2 1 chunk +312 lines, -0 lines 0 comments Download
M src/internet/wscript View 3 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 6
Tom Henderson
There are various improvements that need to be made before ready for merge (indentation problems, ...
6 years, 7 months ago (2011-07-22 20:26:14 UTC) #1
Tom Henderson
this seems very close to merging; some minor comments on documentation and commented-out assertions that ...
6 years, 6 months ago (2011-08-12 23:12:51 UTC) #2
Tom Henderson
I think this is ready to merge, with a few small caveats, which I am ...
6 years, 5 months ago (2011-08-26 23:22:16 UTC) #3
Chris K
On 2011/07/22 20:26:14, Tom Henderson wrote: > There are various improvements that need to be ...
6 years, 3 months ago (2011-11-22 19:47:35 UTC) #4
Tom Henderson
I merged the three main new files from this patchset (the ipv6-address-generator.{cc,h} and corresponding test ...
6 years, 2 months ago (2011-11-27 04:54:18 UTC) #5
Tom Henderson
6 years, 2 months ago (2011-11-27 21:56:34 UTC) #6
On 2011/11/22 19:47:35, Chris K wrote:
> On 2011/07/22 20:26:14, Tom Henderson wrote:
> > There are various improvements that need to be made before ready for merge
> > (indentation problems, lack of doxygen, copy/paste errors from IPv4
classes).
> > 
> > When helpers are used, the Ipv4AddressGenerator is used primarily to detect
> > collisions in address assignment.  I would recommend to add the
> "AddAllocated()"
> > check to the helper-level code, such as in IPv4.
> > 
> > In reviewing this, I noticed that we have different APIs for IPv4 and IPv6
> > address assignment, which IMO in the long run will be confusing to users (we
> > should have caught this earlier).  Basically, the workflow of IPv4 address
> > configuration is like this:
> > SetBase (...);
> > Assign (interfaceContainer);
> > NewNetwork ();
> > Assign (newInterfaceContainer);
> > etc.
> > 
> > but in IPv6, it is:
> > NewNetwork (...);
> > Assign (interfaceContainer);
> > NewNetwork (...);
> > Assign (newInterfaceContainer);
> > 
> > The lack of NewNetwork (void) prevents the user from just incrementing from
> the
> > base, which will be cumbersome in large topology examples; e.g. 
> > src/topology-read/examples/topology-read-sim.cc
> > 
> > Also, there is also an AssignWithoutAddress(interfaceContainer) which
doesn't
> > really do any assignments after all.  Plus, there probably needs to be some
> > consideration for assigning link-locals automatically.
> > 
> > I think we need to revisit this ipv6-address-helper.h API and consider some
> > alignment and review the IPv6 addressing code in general.
> 
> Tom,
> 
> I was browsing this code and came across your IPv6 comments specifically
> regarding the address helper.  I have a local copy of a modified
> ipv6-address-helper class which bridges some of the differences between the
IPv4
> and IPv6, including:
> 
> - Renames the existing NewNetwork(Ipv6Address,Ipv6Prefix) to
> SetBase(Ipv6Address,Ipv6Prefix)
> - Creates void NewNetwork(void)
> 
> Where should I submit these two files, if you are interested?  I may also have
a
> test file lying around somewhere too.

Please send them to tomh@tomh.org.  I also have a locally modified helper with a
SetBase() method, which I can send to you once I get your email address.  I
would like to try to resolve this in the next day or two.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 204d58d