Brite Integration

Brite Integration

[Tom Henderson, 2012-11-16 23:27:02]

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