ns-3 ipv4 changes

[faker.moatamri, 2009-05-15 07:55:25]

Hello,
Here is some comments on the patch to be pushed to the server, the patch review
link being:
http://codereview.appspot.com/65047/show
Best regards
Faker Moatamri

http://codereview.appspot.com/65047/diff/1/13
File src/helper/ipv4-static-routing-helper.cc (right):

http://codereview.appspot.com/65047/diff/1/13#newcode40
Line 40: Ptr<Ipv4RoutingProtocol> ipv4rp = ipv4->GetRoutingProtocol ();
ipv4rp can't be null?

http://codereview.appspot.com/65047/diff/1/20
File src/internet-stack/icmpv4-l4-protocol.cc (right):

http://codereview.appspot.com/65047/diff/1/20#newcode66
Line 66: if (ipv4->GetRoutingProtocol ())
Should you put if (ipv4->GetRoutingProtocol () != 0) instead?

http://codereview.appspot.com/65047/diff/1/22
File src/internet-stack/internet-stack.cc (right):

http://codereview.appspot.com/65047/diff/1/22#newcode97
Line 97: // Add Ipv4ListRouting by default, with an Ipv4StaticRouting protocol
We must be careful when merging with my patch because I removed this
file:internet-stack.h/.cc. This should probably be moved to the
NotifyNewAggregate implementation in ipv4-l3-protocol.cc

http://codereview.appspot.com/65047/diff/1/24
File src/internet-stack/ipv4-global-routing.cc (right):

http://codereview.appspot.com/65047/diff/1/24#newcode138
Line 138: Ptr<Ipv4> ipv4 = m_node->GetObject<Ipv4> ();
Looks like a copy and paste code, can we do it in a cleaner way?

http://codereview.appspot.com/65047/diff/1/24#newcode142
Line 142: // XXX handle multi-address case
Should we put a TODO in here and create a bug in the bug tracker to do it in the
future (of course if it is not altready on the bug tracker)

http://codereview.appspot.com/65047/diff/1/28
File src/internet-stack/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/65047/diff/1/28#newcode579
Line 579: if (!route)
Should you put route == 0?

http://codereview.appspot.com/65047/diff/1/35
File src/internet-stack/ipv4-static-routing-impl.h (right):

http://codereview.appspot.com/65047/diff/1/35#newcode60
Line 60: * @brief Add a host route to the static routing table.
should be \brief, \param, etc...

http://codereview.appspot.com/65047/diff/1/40
File src/internet-stack/tcp-socket-impl.cc (right):

http://codereview.appspot.com/65047/diff/1/40#newcode847
Line 847: if (ipv4->GetRoutingProtocol ())
may be add != 0

http://codereview.appspot.com/65047/diff/1/43
File src/internet-stack/udp-socket-impl.cc (right):

http://codereview.appspot.com/65047/diff/1/43#newcode395
Line 395: else if (ipv4->GetRoutingProtocol ())
add != 0

http://codereview.appspot.com/65047/diff/1/66
File src/routing/olsr/olsr-routing-protocol.cc (right):

http://codereview.appspot.com/65047/diff/1/66#newcode2559
Line 2559: if (Lookup (header.GetDestination (), entry1))
!= 0

http://codereview.appspot.com/65047/diff/1/66#newcode2620
Line 2620: #endif
//NS3_LOG_ENABLE

http://codereview.appspot.com/65047/diff/1/66#newcode2640
Line 2640: //    Ipv4RoutingTableEntry route =
Ipv4RoutingTableEntry::CreateHostRouteTo
remove commented statements

[Tom Henderson, 2009-05-18 04:52:16]

On 2009/05/15 07:55:25, faker.moatamri wrote:
> Hello,
> Here is some comments on the patch to be pushed to the server, the patch
review

Faker, I accepted all of your comments, except the one about logging a bug in
the tracker for some of the XXX comments regarding multi-address not being
handled.  I will plan to file that bug at a later date.

Thanks,
Tom

[Mathieu Lacage, 2009-05-19 15:18:34]

+1 for merging.

http://codereview.appspot.com/65047/diff/1/27
File src/internet-stack/ipv4-interface.h (right):

http://codereview.appspot.com/65047/diff/1/27#newcode100
Line 100: * XXX deprecated?  This is duplicate API to GetDevice ()->GetMtu ()
ok, let's kill it.

http://codereview.appspot.com/65047/diff/1/30
File src/internet-stack/ipv4-list-routing-impl.cc (right):

http://codereview.appspot.com/65047/diff/1/30#newcode255
Line 255: srp = dynamic_cast<Ipv4StaticRouting*>
(PeekPointer((*rprotoIter).second));
DynamicCast<Ipv4StaticRouting> is your friend.

http://codereview.appspot.com/65047/diff/1/60
File src/node/ipv4.h (right):

http://codereview.appspot.com/65047/diff/1/60#newcode128
Line 128: virtual int32_t GetInterfaceForAddress (Ipv4Address address) const =
0;
I liked the previous name better: Find makes it clear that this is a O(n)
operation, not O(1). Not a big deal either way.

http://codereview.appspot.com/65047/diff/1/64
File src/routing/global-routing/global-route-manager-impl.cc (right):

http://codereview.appspot.com/65047/diff/1/64#newcode1438
Line 1438: #if 0
dead code, remove it ?

http://codereview.appspot.com/65047/diff/1/66
File src/routing/olsr/olsr-routing-protocol.cc (right):

http://codereview.appspot.com/65047/diff/1/66#newcode2588
Line 2588: << " interface=" << entry2.interface);      NS_LOG_DEBUG ("Found
route to " << rtentry->GetDestination () << " via nh " << rtentry->GetGateway ()
<< " with source addr " << rtentry->GetSource () << " and output dev " <<
rtentry->GetOutputDevice());
missing cariage return

http://codereview.appspot.com/65047/diff/1/66#newcode2596
Line 2596: #if 0
If code is dead, can you kill it ?

http://codereview.appspot.com/65047/diff/1/66#newcode2613
Line 2613: << " --> NOT FOUND; ** Dumping routing table...");      for
(std::map<Ipv4Address, RoutingTableEntry>::const_iterator iter = m_table.begin
();
missing cariage return

http://codereview.appspot.com/65047/diff/1/66#newcode2627
Line 2627: const Ipv4Header &header, Ptr<const NetDevice> idev,                 
          UnicastForwardCallback ucb, MulticastForwardCallback mcb,
indenting looks all broken.

http://codereview.appspot.com/65047/diff/1/66#newcode2675
Line 2675: << " --> NOT FOUND; ** Dumping routing table...");      for
(std::map<Ipv4Address, RoutingTableEntry>::const_iterator iter = m_table.begin
();
missing caarriage return ?

http://codereview.appspot.com/65047/diff/1/66#newcode2688
Line 2688: #if 0
If this code is dead, can you kill it ?