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

Issue 6499120: Brite Integration

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

Description

Brite Integration

Patch Set 1 #

Total comments: 22

Patch Set 2 : Brite Update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+363 lines, -253 lines) Patch
M src/brite/doc/brite.rst View 1 3 chunks +48 lines, -6 lines 0 comments Download
M src/brite/examples/brite-MPI-example.cc View 1 6 chunks +13 lines, -15 lines 0 comments Download
M src/brite/examples/brite-generic-example.cc View 1 4 chunks +6 lines, -7 lines 0 comments Download
R src/brite/examples/conf_files/newseed_file View 1 1 chunk +0 lines, -6 lines 0 comments Download
R src/brite/examples/conf_files/seed_file View 1 1 chunk +0 lines, -6 lines 0 comments Download
M src/brite/examples/wscript View 1 1 chunk +3 lines, -1 line 0 comments Download
M src/brite/helper/brite-topology-helper.h View 1 9 chunks +89 lines, -45 lines 0 comments Download
M src/brite/helper/brite-topology-helper.cc View 1 7 chunks +170 lines, -108 lines 0 comments Download
M src/brite/test/brite-test-topology.cc View 1 7 chunks +26 lines, -17 lines 0 comments Download
M src/brite/wscript View 1 2 chunks +6 lines, -42 lines 0 comments Download
M test.py View 1 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 1
Tom Henderson
9 years, 7 months ago (2012-11-16 23:27:02 UTC) #1
My main concerns are that the IP address assignment code seems to be incorrect. 
Somehow, the example code works as-is, but we need to follow the right
conventions for IP addressing.  Also, the Ipv6 addressing API should align with
the Ipv4 addressing API (use of a helper).  

The secondary concerns are:
- no IPv6 test or example (but this is probably due to lacking global/nix
routing support?)
- the documentation needs to go deeper, in my opinion
- test doesn't compile
- NS_LOG statements are missing from the helper

and there are a bunch of other cleanups I'd like to see (inline in the code).

http://codereview.appspot.com/6499120/diff/1/src/brite/doc/brite.rst
File src/brite/doc/brite.rst (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/doc/brite.rst#newcode34
src/brite/doc/brite.rst:34: for the user to either attach custom topologies or
install ns-3 applications directly.
Here is where there could be more detail about distinguishing between an
AS-level and a RTR-level topology.

http://codereview.appspot.com/6499120/diff/1/src/brite/doc/brite.rst#newcode41
src/brite/doc/brite.rst:41: 
How about a hyperlink to the BRITE user's manual?
http://www.cs.bu.edu/brite/publications/usermanual.pdf

http://codereview.appspot.com/6499120/diff/1/src/brite/doc/brite.rst#newcode102
src/brite/doc/brite.rst:102: $ ./waf --run brite-generic-example --vis
can you add some documentation about the difference between the generic and the
MPI variants?

Also, the example uses a specific conf file, but a lot of other conf files are
provided.  Can you document here what each different conf file is providing?

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/brite-generic...
File src/brite/examples/brite-generic-example.cc (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:90: bth.AssignIpv4Addresses
(address);
see my other comment in the topology-helper code; if we instead passed 'stack'
to BuildBriteTopology, loopback devices would end up on interface index 0.

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/conf_files/se...
File src/brite/examples/conf_files/seed_file (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/conf_files/se...
src/brite/examples/conf_files/seed_file:2: CONNECT 26883 38699 1089
are these seed files needed anymore?

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/wscript
File src/brite/examples/wscript (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/examples/wscript#newcode4
src/brite/examples/wscript:4: obj =
bld.create_ns3_program('brite-generic-example', ['brite', 'internet',
'point-to-point', 'nix-vector-routing', 'applications', 'visualizer'])
delete 'visualizer' here, as it is not needed and will prevent the program from
running if python is disabled.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.cc (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:18: #include "ns3/log.h"
ns-3 logging is missing from this; at least support NS_LOG_FUNCTION() and also
NS_LOG_DEBUG for the logic-intensive operations of this helper.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:51: Ptr<UniformRandomVariable> uv =
CreateObject<UniformRandomVariable> ();
We are going to want to move this to the constructor and make it a member
variable.  Then, we want to support BriteTopologyHelper::AssignStreams (int64_t)
so that users can fix the randomness of the topology while varying other aspects
of the topology.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:69: remove("briteSeedFile.txt");
Should this be removed?  Might users want to archive this with the output of
their experiment?  Suggest to keep it.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:322: address.NewNetwork();
This seems to assign the same network to all devices in an AS, which is wrong. 
I'm not sure how the example code can work with this, but routing seems to be
able to handle this.

Address assignment and network number incrementing has to be done by iterating
on edges, not net devices.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:339: Ipv6AddressGenerator::NextNetwork
(prefix);
similar comment.  This should align with the Ipv4 API, using IPv4AddressHelper. 
I think that at the time that this was originally written (pre ns-3.14), the
Ipv6AddressHelper didn't exist, but we should update this now.

Also, there seems to be no testing of the IPv6 code, even in an example.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:387:
BriteTopologyHelper::BuildBriteTopology ()
We might want to consider adding the InternetStack helper here as a parameter. 
This is perhaps subtle, but as it is written now, the devices are added to the
node before the internet stack is added.  This means that the Ipv4 loopback
device doesn't get to be on interface id 0 but is instead the last interface in
general.  

If the internet stack helper were available, then it could add the stack before
calling ConstructTopology().

I don't think any code breaks doing it the current way but I was just observing,
when studying the resulting example, that the loopback interfaces were getting
the highest interface ID rather than 0.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.h (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:94: NodeContainer& GetLeafNodesForAS
(uint32_t asNum);
return by reference is not allowed; can you return by value (does the
NodeContainer have value semantics)?

Also, shouldn't there be a way to access *all* nodes in the topology?  Perhaps a
GetNodesForAS () could be added.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:138: PointToPointHelper&
GetPointToPointHelperForTopology ();
same comment-- do not return by reference, but by value, but check that you can
return by value for PointToPointHelper object.

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:144: * \returns the pointToPoint helper
used to create the links within the brite topology
copy-paste error

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:152: * \returns the pointToPoint helper
used to create the links within the brite topology
copy-paste error

http://codereview.appspot.com/6499120/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:219: uint32_t numAs;
all of these private variables should be prefixed with "m_".  This makes the
implementation code easier to read.

http://codereview.appspot.com/6499120/diff/1/src/brite/test/brite-test-topolo...
File src/brite/test/brite-test-topology.cc (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/test/brite-test-topolo...
src/brite/test/brite-test-topology.cc:188: AddTestCase (new
BriteTopologyFunctionTestCase);
consider adding an IPv6 equivalent

hmm... no global/nix routing though (perhaps it is time to work on that...)

http://codereview.appspot.com/6499120/diff/1/src/brite/wscript
File src/brite/wscript (right):

http://codereview.appspot.com/6499120/diff/1/src/brite/wscript#newcode67
src/brite/wscript:67: def build(bld):
build() is defined twice.  Delete from line 67-107.

http://codereview.appspot.com/6499120/diff/1/src/brite/wscript#newcode69
src/brite/wscript:69: module = bld.create_ns3_module('brite', ['network',
'core'])
brite depends on 'internet' also, and point-to-point

http://codereview.appspot.com/6499120/diff/1/src/brite/wscript#newcode112
src/brite/wscript:112: ]
You need to add test library here:

+  module_test = bld.create_ns3_module_test_library('brite')
+  module_test.source = [
+        ]

http://codereview.appspot.com/6499120/diff/1/src/brite/wscript#newcode124
src/brite/wscript:124: headers.source.append ('helper/brite-topology-helper.h')
You need to add test code here:
+      module_test.source.append('test/brite-test-topology.cc')

but this test doesn't presently compile
Sign in to reply to this message.

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