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

Mathieu Lacage mathieu.lacage at sophia.inria.fr
Tue Jan 27 01:18:30 PST 2009


hi tom,

For reviews, I usually find it easier if you include a list of files
added and modified. 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.

2) src/node/ipv4.h:

Given its semantics, Ipv4::AddRoutingProtocol should be renamed to
Ipv4::SetRoutingProtocol.

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

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

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. 

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. 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.

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.
}

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.

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.

Mathieu



More information about the Ns-developers mailing list