[Ns-developers] review request for ns-3 IPv4 refactoring

[Tom Henderson]

Mathieu,
Thanks for the quick feedback.  Some responses below.

>-----Original Message-----
>From: Mathieu Lacage [mailto:mathieu.lacage at sophia.inria.fr]
>Sent: Tuesday, January 27, 2009 01:18 AM
>To: 'Tom Henderson'
>Cc: 'ns-developers'
>Subject: Re: [Ns-developers] review request for ns-3 IPv4 refactoring
>
>hi tom,
>
>For reviews, I usually find it easier if you include a list of files
>added and modified. 

I will try to generate such a list and post it later.

>Anyway, review of the API in src/node/ipv4* only
>below:
>
>1) src/node/ipv4-interface-address.h:
>
>Needs serious documentation on what it is: what is a host, link, and
>local ip address ? Define what primary vs secondary means. I am not
>familiar with the linux implementation so, either you need to include a
>link to where I can find information about all these concepts or
>copy/paste them in the header.

I did not have time to write that doxygen documentation, but here is some for now:
http://www.policyrouting.org/iproute2.doc.html#ss9.2

>
>2) src/node/ipv4.h:
>
>Given its semantics, Ipv4::AddRoutingProtocol should be renamed to
>Ipv4::SetRoutingProtocol.

That would be fine.  I had it as "SetRouting" a while back.

>
>I would tend towards renaming Ipv4::GetInterfaceForAddress to
>Ipv4::FindInterfaceForAddress. The same goes for GetInterfaceForDevice
>which I would rename to FindInterfaceForDevice.

These name choices go back to Craig's initial repository to fix bug 85 (ns-3.2 timeframe) so I have not thought about them much recently and would have to go back to the archives to check, but I think that Craig suggested this because "Find..." was not a conventional method name through the rest of our codebase ("Get..." is prevalent).

>
>It would be nice to document more fully what JoinMulticastGroup and
>LeaveMulticastGroup do. Do these generate IGMP messages ?

I did not change these functions or the doxygen.  Typically JoinMulticastGroup would generate IGMP, but the internet stack does not yet support IGMP, so they just enable delivery of the multicast groups selected.

>
>I am not sure I really understand the difference between SetAddress and
>AddAddress. Maybe these should be renamed to SetPrimaryAddress and
>AddSecondaryAddress ? 
>
>Similarly, maybe 'Ipv4Address GetAddress (uint32_t interface)' should be
>renamed GetPrimaryAddress. 

I see that you immediately detected the API overlap here.  More on this in a few sentences.

>
>Overall, this header gives me a feeling of utter confusion because it is
>not clear from the methods what is associated to each device: if it is a
>list of Ipv4InterfaceAddress, then, this is what you should export from
>this header. Right now, some methods manipulate Ipv4InterfaceAddress,
>others don't: while I think I sort of understand why, I think that this
>is really just confusing and that it would make more sense to bite the
>bullet and deal with Ipv4InterfaceAddress objects only in this class
>API. 

Yes, you are chiding me for not being bold enough to remove those methods.

Ipv4::SetAddress
Ipv4::GetAddress
Ipv4::SetNetworkMask
Ipv4::GetNetworkMask

I started down that path and abandoned it for now mainly just for backward compatibility reasons and because it seems that the common use case will be having one address.  But you are right; if the API assumes that addressing information is grouped into classes such as has been proposed for Ipv4InterfaceAddress, then methods like the above don't fit as well.

>If you feel that a convenience API is needed to minimize the impact
>on the existing IPv4 stack and users, I would suggest to move this
>convenience API and methods somewhere else, maybe as standalone
>functions (not methods) defined in the ipv4.h header, or, if you really
>feel that they should belong to the Ipv4 class, as non-virtual methods
>which are clearly separated from the other virtual methods defined in
>the class.

I think that at the very least, non-virtual class methods would be an improvement.  

>
>My suggestion here to clarify the purpose and intend of this class would
>be something along these lines:
>
>uint32_t GetNInterfaces (void) const;
>std::list<Ipv4InterfaceAddress> GetAddresses (uint32_t interface) const;
>
>(feel free to replace the std::list with std::vector or with an extra
>input argument for the address index).
>
>And, then:
>
>namespace Ipv4Utils {
> Ipv4Address GetPrimaryAddress (Ptr<Ipv4> ipv4, uint32_t interface);
> ... whatever you think is helpful.

I will think about this some more.  GetPrimaryAddress is probably not the best name since there can be more than one primary address.

I think the main issue to resolve now is whether we want to keep the existing methods that presume only one IP address and mask can be on an interface, for backward compatibility reasons.

>}
>
>3) src/node/ipv4-routing-protocol.h
>
>
>Ipv4RoutingProtocol::AddRoutingProtocol does not belong to this base
>class: its declaration should be moved to the subclasses which support
>this feature. The need for
>Ipv4RoutingProtocol::SupportsAddRoutingProtocol should have been a
>warning sign that you were moving in your base class subclass-level API.
>If you really must, you could define an intermediate subclass of
>Ipv4RoutingProtocol which defines this method:
>
>class ListIpv4RoutingProtocol : Ipv4RoutingProtocol {
>public:
>  virtual ... AddRoutingProtocol (...) = 0;
>};
>
>and, then, derive from this class instead of Ipv4RoutingProtocol when
>needed. But this looks like overengineering to me.

Yes, it didn't seem ideal but I followed the lead of what we've done in other places such as SupportsSendFrom ().  

The general problem here is that an agent (at runtime) needs to insert a routing protocol into a node.  It has a Ptr<Ipv4>.  Take OLSR for example.  OLSR could be inserted as the only routing protocol, or it could be added to an existing routing protocol (the proposed Ipv4ListRouting protocol) in the manner as is presently done in ns-3.  It can find out whether Ipv4 has a routing protocol, but in that case, it needs to know whether it should insert itself as another protocol, or assert due to trying to overwrite some other protocol.  

One way that I originally had this working was to DynamicCast the Ipv4->GetRoutingProtocol to a ListRouting protocol (i.e., what you call overengineering above).

Another possible solution would be to add some API to OLSR to instruct it how it should add itself at runtime into the stack, rather than let OLSR figure it out with such mechanisms. 

>
>I won't comment on the detailed signature of RouteInput and RouteOutput
>because I don't understand the details of routing anymore (especially
>when you have to handle all the unicast/multicast variants).
>
>
>On Tue, 2009-01-27 at 00:15 -0800, Tom Henderson wrote:
>> I've been working on some refactoring of the IPv4 layer for a few 
>> months, and would like to propose it for merging in ns-3.4.  There is 
>> still some stuff to be done, but I'm fairly satisfied with how it has 
>> come out and I believe it will be ready for merge by next week, barring 
>> major comments.
>
>I don't especially enjoy to be the bearer of bad news (despite what some
>might believe :) but if the goal was to merge this in 3.4, final reviews
>for this crucial API/implementation should have started happening in
>early january, not on the day of the end of "new feature
>merging" (http://www.nsnam.org/wiki/index.php/Ns-3.4#The_ns-3.4_Release_Schedule).
>
>I would be happy to invest some time to help finalize this tree if you
>want me to but we would need to settle on a definite API first.
>

Your point is well taken; ideally, that would have been the case but it took me a lot of time (in various spurts of activity) to work through these changes.  

I think there is a lot of interest in using ns-3 for routing work (judging by the ns-3-users posts) and it is important to make sure that people are comfortable with the API and framework. 

So, I am not going to push for ns-3.4 inclusion too strenuously, but I am still open to merging it if there is rapid consensus.  I would like to ask that people look at it and discuss it a bit and maybe we can revisit what to do with it by early next week.  

Tom