[Ns-developers] release update [feedback needed]

[Mathieu Lacage]

A related question about this code: why do we keep around all these base
class + implementation classes in src/node and src/internet-stack for
ListRouting ? I understand why you did this for StaticRouting but,
ListRouting does not look immediately useful and appears to merely
increase the pain of introducing modifications there. Would you object
if I removed the Ipv4ListRouting base class and removed the Impl postfix
of the impl class ?

Mathieu

On Tue, 2009-06-09 at 22:38 -0700, Tom Henderson wrote:
> Mathieu Lacage wrote:
> > On Tue, 2009-06-09 at 16:03 +0000, Tom Henderson wrote:
> > 
> >> I like the general idea.  It could be expanded to align with how
> > 
> > Shall I infer from the above that you approve of the patch as-is and
> > that I can push it ? Or that you plan to review it carefully later ?
> 
> this patch cleans up a sore spot in the implementation.  More work needs 
> to be done for static multicast routes, but regression tests pass, and I 
> think it is an improvement.
> 
> The main API is in the Ipv4RoutingProtocol base class:
> +  /**
> +   * \param i the index of the interface we are being notified about
> +   * \param isUp is this interface coming up or down ?
> +   *
> +   * Protocols are expected to implement this method to be notified of 
> the state change of
> +   * an interface in a node. When a new protocol is added to a node, it 
> is notified
> +   * of all existing interfaces.
> +   */
> +  virtual void NotifyInterface (uint32_t i, bool isUp) = 0;
> 
> This is similar to what is found in RTM_NEWLINK and RTM_DELLINK for 
> rtnetlink in Linux (man rtnetlink), except that the boolean flag allows 
> both to be in a single method NotifyInterface.  Since the routing 
> protocol consuming this message will presumably be able to acquire a 
> pointer to class Ipv4 to get whatever other information is needed, the 
> interface index is probably sufficient information to pass up.
> 
> The questions/concerns I have:
> 
> 1) implicit assumptions are made about the order in which these 
> operations occur:
> - set an IP interface to up
> - add an IP address to an interface
> - set an IPv4RoutingProtocol to the Ipv4 object
> - add an additional routing protocol to the Ipv4ListRouting protocol
> 
> There are certain combinations of operations that will cause this to 
> lose information; e.g.
> - add the Ipv4ListRouting to the Ipv4, and subsequently add protocols 
> like OLSR (the protocols added later will not get the notifications)
> - add an IP address to an interface (such as a second address) after the 
> first address was added and interface was SetUp()
> 
> so, it would help to add code to deal with users possibly rearranging 
> these operations.  For instance, I think that probably these additions 
> are needed:
>    - NotifyAddAddress (uint32_t i, Ipv4InterfaceAddress addr)
>    - NotifyRemoveAddress (uint32_t i, Ipv4InterfaceAddress addr)
> 
> or, for symmetry with NotifyInterface(), it could be:
>    - NotifyAddress (uint32_t i, Ipv4InterfaceAddress addr, bool IsAdded);
> 
> and probably Ipv4ListRoutingImpl::AddRoutingProtocol () needs to notify 
> the new routing protocol added of the interfaces, in case it is added 
> after being hooked to Ipv4.
> 
> 2) implicit assumption about the user not repeating these methods or
> calling them out of order-- e.g. can NotifyInterface (i, true); be 
> called multiple times (resulting in multiple redundant routes)?  Should, 
> for instance, the current state of an interface be saved so that these 
> calls can be detected and avoided?
> 
> 3) I had thought about putting in an rtnetlink-like framework for what 
> you posted, but that may be overkill for the moment.  However, suppose 
> someone wanted to add an rtnetlink notification framework to ns-3 in the 
> future.  What kind of notification framework would you recommend? 
> Multicast messaging internally?  Some kind of callback framework (could 
> you have multiple users register callbacks somewhere for the same event, 
> and where would they register (a Netlink object?)?
> 
> Thanks for the patch,
> Tom