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