"Bug 2546: Add PrintRoutingTable(All)Change function to only print routing table changes."

"Bug 2546: Add PrintRoutingTable(All)Change function to only print routing table changes."

[Tom Henderson, 2017-01-21 21:45:54]

Overall I like this addition but have some small comments to resolve before
merging.

https://codereview.appspot.com/317900043/diff/20001/src/aodv/examples/aodv-1.cc
File src/aodv/examples/aodv-1.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/aodv/examples/aodv-1....
src/aodv/examples/aodv-1.cc:1: /* -*- Mode:C++; c-file-style:"gnu";
indent-tabs-mode:nil; -*- */
I prefer to keep a single 'aodv.cc' and either 1) change the existing periodic
routing table print method to the change-driven version that you add, or 2) add
an option to command line such as 'printPeriodic' (true/false) that toggles the
behavior from PrintAt to PrintChange.

https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
File src/aodv/model/aodv-routing-protocol.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
src/aodv/model/aodv-routing-protocol.cc:344: NS_LOG_FUNCTION_NOARGS ();
change to 'NS_LOG_FUNCTION (this)'; NS_LOG_FUNCTION_NOARGS (void) is supposed to
be used with static methods where there is no 'this' pointer.

https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
src/aodv/model/aodv-routing-protocol.cc:880: RouteChange();
This comment applies to also DSDV and perhaps others.  The calls to
RouteChange() are sprinked throughout this file, and it is possible that we
forget to call this in the future (or that a call to e.g. AddRoute() doesn't
result in an actual change.  Consider how Olsr has a RouteChangedCallback, which
allows you to localize the RouteChange() call to one place.  What about instead
introducing similar RouteChangedCallback to the Aodv and Dsdv tables?  

I realize that AODV routing table is not an ns3::Object but it could either be
made into one, or a callback could still be included at the public API of the
table.

https://codereview.appspot.com/317900043/diff/20001/src/click/model/ipv4-clic...
File src/click/model/ipv4-click-routing.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/click/model/ipv4-clic...
src/click/model/ipv4-click-routing.cc:566: {
I tend to prefer at least an NS_LOG_WARN ("method unimplemented!"); or even an
NS_FATAL_ERROR or NS_ABORT if this is called.  Leaving method bodies blank leads
to soft errors.

https://codereview.appspot.com/317900043/diff/20001/src/dsdv/examples/dsdv-ma...
File src/dsdv/examples/dsdv-manet-1.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/dsdv/examples/dsdv-ma...
src/dsdv/examples/dsdv-manet-1.cc:1: /* -*- Mode:C++; c-file-style:"gnu";
indent-tabs-mode:nil; -*- */
similar comment as to aodv-1.cc; let's keep one version of the example, perhaps
defaulting to the PrintChange mode of operation.

https://codereview.appspot.com/317900043/diff/20001/src/dsdv/model/dsdv-routi...
File src/dsdv/model/dsdv-routing-protocol.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/dsdv/model/dsdv-routi...
src/dsdv/model/dsdv-routing-protocol.cc:255: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION (this);  (please review the patch globally for this change)

https://codereview.appspot.com/317900043/diff/20001/src/internet/model/ipv4-g...
File src/internet/model/ipv4-global-routing.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/internet/model/ipv4-g...
src/internet/model/ipv4-global-routing.cc:84: RouteChange();
Global routing is a special case; this is not a dynamic routing protocol but one
that is run at time zero, so I don't think it is appropriate to call
RouteChange() every time some change is made during the initial construction.

Can you perhaps try to call or trigger a call from the end of
GlobalRouteManagerImpl::InitializeRoutes ()?

https://codereview.appspot.com/317900043/diff/20001/src/nix-vector-routing/mo...
File src/nix-vector-routing/model/ipv4-nix-vector-routing.cc (right):

https://codereview.appspot.com/317900043/diff/20001/src/nix-vector-routing/mo...
src/nix-vector-routing/model/ipv4-nix-vector-routing.cc:124: RouteChange();
similar comment here as to Ipv4GlobalRouting; suggest to make sure that
RouteChange() gets called only once upon an actual routing table change.

[mariemarth7, 2017-07-23 10:44:53]

On 2017/01/21 21:45:54, Tom Henderson wrote:
> Overall I like this addition but have some small comments to resolve before
> merging.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/aodv/examples/aodv-1.cc
> File src/aodv/examples/aodv-1.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/aodv/examples/aodv-1....
> src/aodv/examples/aodv-1.cc:1: /* -*- Mode:C++; c-file-style:"gnu";
> indent-tabs-mode:nil; -*- */
> I prefer to keep a single 'aodv.cc' and either 1) change the existing periodic
> routing table print method to the change-driven version that you add, or 2)
add
> an option to command line such as 'printPeriodic' (true/false) that toggles
the
> behavior from PrintAt to PrintChange.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
> File src/aodv/model/aodv-routing-protocol.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
> src/aodv/model/aodv-routing-protocol.cc:344: NS_LOG_FUNCTION_NOARGS ();
> change to 'NS_LOG_FUNCTION (this)'; NS_LOG_FUNCTION_NOARGS (void) is supposed
to
> be used with static methods where there is no 'this' pointer.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/aodv/model/aodv-routi...
> src/aodv/model/aodv-routing-protocol.cc:880: RouteChange();
> This comment applies to also DSDV and perhaps others.  The calls to
> RouteChange() are sprinked throughout this file, and it is possible that we
> forget to call this in the future (or that a call to e.g. AddRoute() doesn't
> result in an actual change.  Consider how Olsr has a RouteChangedCallback,
which
> allows you to localize the RouteChange() call to one place.  What about
instead
> introducing similar RouteChangedCallback to the Aodv and Dsdv tables?  
> 
> I realize that AODV routing table is not an ns3::Object but it could either be
> made into one, or a callback could still be included at the public API of the
> table.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/click/model/ipv4-clic...
> File src/click/model/ipv4-click-routing.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/click/model/ipv4-clic...
> src/click/model/ipv4-click-routing.cc:566: {
> I tend to prefer at least an NS_LOG_WARN ("method unimplemented!"); or even an
> NS_FATAL_ERROR or NS_ABORT if this is called.  Leaving method bodies blank
leads
> to soft errors.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/dsdv/examples/dsdv-ma...
> File src/dsdv/examples/dsdv-manet-1.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/dsdv/examples/dsdv-ma...
> src/dsdv/examples/dsdv-manet-1.cc:1: /* -*- Mode:C++; c-file-style:"gnu";
> indent-tabs-mode:nil; -*- */
> similar comment as to aodv-1.cc; let's keep one version of the example,
perhaps
> defaulting to the PrintChange mode of operation.
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/dsdv/model/dsdv-routi...
> File src/dsdv/model/dsdv-routing-protocol.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/dsdv/model/dsdv-routi...
> src/dsdv/model/dsdv-routing-protocol.cc:255: NS_LOG_FUNCTION_NOARGS ();
> NS_LOG_FUNCTION (this);  (please review the patch globally for this change)
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/internet/model/ipv4-g...
> File src/internet/model/ipv4-global-routing.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/internet/model/ipv4-g...
> src/internet/model/ipv4-global-routing.cc:84: RouteChange();
> Global routing is a special case; this is not a dynamic routing protocol but
one
> that is run at time zero, so I don't think it is appropriate to call
> RouteChange() every time some change is made during the initial construction.
> 
> Can you perhaps try to call or trigger a call from the end of
> GlobalRouteManagerImpl::InitializeRoutes ()?
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/nix-vector-routing/mo...
> File src/nix-vector-routing/model/ipv4-nix-vector-routing.cc (right):
> 
>
https://codereview.appspot.com/317900043/diff/20001/src/nix-vector-routing/mo...
> src/nix-vector-routing/model/ipv4-nix-vector-routing.cc:124: RouteChange();
> similar comment here as to Ipv4GlobalRouting; suggest to make sure that
> RouteChange() gets called only once upon an actual routing table change.