Sorry, I'm retarded, corrections to my bug report...

TOPOLOGY:

     WIRED CSMA             WIRELESS CHANNEL
   ______________
   O            O  ))          (( O ))           (( O
   N3           N0                N1                N2  


Running the UdpEcho application, the packets make it to the
destination N3 (172.16.0.1), but for some reason, N3 cannot echo the packet
back to N2. Looking at the pcap file, it seems that N3 is asking (ARP) for the MAC
address of 192.168.0.1 (the gateway N0 on Wifi interface) and it should be
asking for 172.16.0.2 (the gateway N0 on CSMA interface).

(In reply to comment #1)
> Sorry, I'm retarded, corrections to my bug report...
> 
> TOPOLOGY:
> 
>      WIRED CSMA             WIRELESS CHANNEL
>    ______________
>    O            O  ))          (( O ))           (( O
>    N3           N0                N1                N2  
> 
> 
> Running the UdpEcho application, the packets make it to the
> destination N3 (172.16.0.1), but for some reason, N3 cannot echo the packet
> back to N2. Looking at the pcap file, it seems that N3 is asking (ARP) for the
> MAC
> address of 192.168.0.1 (the gateway N0 on Wifi interface) and it should be
> asking for 172.16.0.2 (the gateway N0 on CSMA interface).
> 

I've been looking at this off and on for the past week or so.  Here is what I've found.

1) here are the routes that N3 OLSR builds:

dest: 192.168.0.1 next: 192.168.0.1 iface: 172.16.0.1 dist: 1
dest: 172.16.0.2 next: 172.16.0.2 iface: 172.16.0.1 dist: 1
dest: 192.168.0.2 next: 192.168.0.1 iface: 172.16.0.1 dist: 2
dest: 192.168.0.3 next: 192.168.0.1 iface: 172.16.0.1 dist: 3

What I think is missing is the following entry 
dest: 192.168.0.1 next: 172.16.0.2 iface 172.16.0.1 dist: 1

my read of step 4 of section 10 of RFC 3626 suggests that the above entry should be added, because N3 does have a mid message from N0.  It looks like step 4 of RFC 3626 is implemented in the ns-3 code, but I haven't debugged yet why it adds no routes at this step.

2) however, even if the above entry is added, I don't think it will route correctly.  OLSR will still return 192.168.0.1 to the caller for destination 192.168.0.3.  There needs to be some intelligence somewhere (either in the caller or in OLSR) to know that 192.168.0.1 is not on-link, and that 172.16.0.2 is the next hop for 192.168.0.1.  So, either OLSR would need to add those routes (which seem to be outside of what the RFC specifies) or Ipv4L3Protocol would need to know to recurse, ask again for next hop to 192.168.0.1, and OLSR would then have to return 172.16.0.2 instead of 192.168.0.1.

So, I'm doubtful there is an easy, purely OLSR fix for this.

I would like to move this to a suggested ns-3.4 resolution.  What is needed is to finish off the HNA (associated networks and hosts) support for OLSR.  Here is a comment in the current code:

 *  - HNA (Host/Network Association) messages are almost-but-not-quite supported in this version.


     WIRED CSMA             WIRELESS CHANNEL
   ______________
   O            O  ))          (( O ))           (( O
   N3           N0                N1                N2  


I suggest that the proper configuration would be to run OLSR on nodes N0, N1, and N2, with node N0 advertising the N0N3 CSMA subnet in an HNA (so that N1 and N2 can route to it).   Then, put a static route to 192.168.0/24 at N3, or else configure a stub prefix interface on N0 and run global routing.

Stop priority oscillations.

Created attachment 737 [details]
hna.patch

Adds HNA support to OLSR.

(In reply to comment #5)
> Created an attachment (id=737) [details]
> hna.patch
> 
> Adds HNA support to OLSR.

This method highlights a bit of naming inconsistency that I found:

+OlsrState::EraseHostNetAssociation (const Association &tuple)

Make up your mind, either call the HNA state "Association" or "HostNetAssociation", or "AssociationTuple".  Get/Set/Insert/Erase/Find methods should be named in accordance to the information they manipulate.

I'll finish my review later...

I am looking at RFC 3626, 12.6.  Routing Table Calculation, and,

     1    If there is no entry in the routing table with:

               R_dest_addr     == A_network_addr/A_netmask

The corresponding code is:

+      bool have_entry1 = Lookup (tuple.networkAddr.CombineMask (tuple.netmask), entry1);
[...]
+      if (!have_entry1)
+        {
+          to_add = true;
+        }
[...]
+      if(to_add == true)
+        {
+          AddEntry (tuple.networkAddr.CombineMask (tuple.netmask),
+                    entry2.nextAddr,
+                    entry2.interface,
+                    entry2.distance);
+        }

At this point, one code path leads to entry2 being uninitialized (have_entry1=true and have_entry2=false), but being used.  Not correct.

Now the other condition:

    2    If a new routing entry was created at the previous step, or
          else if there existed one with:

               R_dest_addr     == A_network_addr/A_netmask


               R_dist          >  dist to A_gateway_addr of
                                  current association set tuple,

And the code:

+      bool have_entry1 = Lookup (tuple.networkAddr.CombineMask (tuple.netmask), entry1);
+      bool have_entry2 = Lookup (tuple.gatewayAddr, entry2);
[...]
+      else if(have_entry2 && entry1.distance > entry2.distance)
+        {
+           RemoveEntry(tuple.networkAddr.CombineMask (tuple.netmask));
+           to_add = true;           
+        }

Check.  It seems to be what is intended.  Although:

+          AddEntry (tuple.networkAddr.CombineMask (tuple.netmask),
+                    entry2.nextAddr,
+                    entry2.interface,
+                    entry2.distance);

This is not your fault, it is RFC 3626's fault (see introductory text in "10.  Routing Table Calculation", explaining the exact routing table format), but it seems like there is some loss of information going on here, since the routing table does not record the network mask; it is only prepared to accept host entries.  I would expect a HNA entry like 192.168.0.0/16 to match an address like 192.168.1.1.  However, when the routing table is formed it will only receive a 192.168.0.0 address, and so a packet to destination 192.168.1.1 will not be matched.

In conclusion, it sounds to me like this HNA implementation will only work for host entries, not network entries.  But it's a good start.

For the non-OLSR interface problem, I suppose we need to add a method like:

olsr::RoutingProtocol::SetInterfaceNonOlsr (uint32_t interface);

We add that interface to a std::set<uint32_t>, and in DoStart () exclude that interface when creating the sockets.  Just a suggestion..

Thanks for the review, will work on the flaws.

Regarding the naming convention, here's what I'd tried:

Suppose a node A has an association named (networkAddr, netmask), it sends this association in an HNA message, which is received by a node B.

(gatewayAddr, networkAddr, netmask, expirationTime) is saved by node B as an AssociationTuple. The list of AssociationTuple's make an AssociationSet. (This is in accordance with RFC 3626).

Now here's the part where I tried to come up with my own names, was and am still confused, and would like some help:

(networkAddr, netmask) is an 'Association', used by A. I used the same name as was already used in olsr-message-header.h.

In the Get/Insert/Erase methods, I used HostNetAssociation to refer to 'Association' set of a node, which is what the node shares with the rest of the network through HNA messages.

Should I keep (networkAddr, netmask) as 'HostNetAssociation' rather than 'Assocation'? That will divide things into HostNetAssociation - HostNetAssocSet and AssociationTuple - AssociationSet ? Else can you please suggest what names I can use here? 

And after the discussion I had with Tom, I think it would be better if RoutingProtocol::InjectRoute() is renamed to RoutingProtocol::AddHostNetAssociation(). What do you think?

Let me know so that I can fix this first and then work on the remaining problems with the patch. Thanks again!

(In reply to comment #8)
> Thanks for the review, will work on the flaws.
> 
> Regarding the naming convention, here's what I'd tried:
> 
> Suppose a node A has an association named (networkAddr, netmask), it sends this
> association in an HNA message, which is received by a node B.
> 
> (gatewayAddr, networkAddr, netmask, expirationTime) is saved by node B as an
> AssociationTuple. The list of AssociationTuple's make an AssociationSet. (This
> is in accordance with RFC 3626).
OK

> 
> Now here's the part where I tried to come up with my own names, was and am
> still confused, and would like some help:
> 
> (networkAddr, netmask) is an 'Association', used by A. I used the same name as
> was already used in olsr-message-header.h.

It's not the same thing.  In olsr-header.h the struct Association {} is namespaced inside the Hna struct, which is inside olsr::Message.  So the full C++ identifier is actually ns3::olsr::Message::Hna::Association :-)

> 
> In the Get/Insert/Erase methods, I used HostNetAssociation to refer to
> 'Association' set of a node, which is what the node shares with the rest of the
> network through HNA messages.
> 
> Should I keep (networkAddr, netmask) as 'HostNetAssociation' rather than
> 'Assocation'? That will divide things into HostNetAssociation - HostNetAssocSet
> and AssociationTuple - AssociationSet ? Else can you please suggest what names
> I can use here? 

To remain consistent with the rfc, I think it should be InsertAssociationTuple, EraseAssociation, etc.  Although I don't agree with the rfc (I think "association tuple" is too vague), at least it's a documented naming.

> 
> And after the discussion I had with Tom, I think it would be better if
> RoutingProtocol::InjectRoute() is renamed to
> RoutingProtocol::AddHostNetAssociation(). What do you think?

I think the naming conventions in NS3 forbid abbreviated names like 'Net'.

Perhaps two separate methods could be added:

  1. AddHostAssociation(Ipv4Address)

  2. AddNetworkAssociation(Ipv4Address, Ipv4Mask)

_however_, I normally like code that is more intelligent.  In OLSR you don't have have AddOlsrInterface; it just uses all the available interfaces in the node.  Likewise, perhaps an automatic approach could be used here as well?  It would require the code to:
  1. Go to the Ipv4 interface of the node;
  2. Get the list of routing protocols;
  3. Iterate over the list and, for each "static ipv4 routing protocol":
    3a. Iterate over the static routing entries and generate Hna message(s) based on them.

(In reply to comment #9)


> 
> To remain consistent with the rfc, I think it should be InsertAssociationTuple,
> EraseAssociation, etc.  Although I don't agree with the rfc (I think
> "association tuple" is too vague), at least it's a documented naming.
> 

So I'll drop all the -HostNet- namings and keep it Association, Associations and [Get/Insert/Erase]Association() for the methods. Will make the changes as soon as you signal. :)


> > 
> > And after the discussion I had with Tom, I think it would be better if
> > RoutingProtocol::InjectRoute() is renamed to
> > RoutingProtocol::AddHostNetAssociation(). What do you think?
> 
> I think the naming conventions in NS3 forbid abbreviated names like 'Net'.
> 

How's RoutingProtocol::AddAssociation ()? :)

> Perhaps two separate methods could be added:
> 
>   1. AddHostAssociation(Ipv4Address)
> 
>   2. AddNetworkAssociation(Ipv4Address, Ipv4Mask)
> 
> _however_, I normally like code that is more intelligent.  In OLSR you don't
> have have AddOlsrInterface; it just uses all the available interfaces in the
> node.  Likewise, perhaps an automatic approach could be used here as well?  It
> would require the code to:
>   1. Go to the Ipv4 interface of the node;
>   2. Get the list of routing protocols;
>   3. Iterate over the list and, for each "static ipv4 routing protocol":
>     3a. Iterate over the static routing entries and generate Hna message(s)
> based on them.

Just wondering, wouldn't it make things easier if we add a netmask field to the RoutingTableEntry? Anything coming in from a TC message is automatically given a netmask of /32 whereas for the HNA messages, we specify the netmasks separately?

And looking at the olsrd implementation (http://www.olsr.org/docs/olsrd.conf.5.html), it seems like they expect the user to mention the associations themselves.

(In reply to comment #10)
> (In reply to comment #9)
> 
> 
> > 
> > To remain consistent with the rfc, I think it should be InsertAssociationTuple,
> > EraseAssociation, etc.  Although I don't agree with the rfc (I think
> > "association tuple" is too vague), at least it's a documented naming.
> > 
> 
> So I'll drop all the -HostNet- namings and keep it Association, Associations
> and [Get/Insert/Erase]Association() for the methods. Will make the changes as
> soon as you signal. :)

OK, fine by me.  Just be consistent.

> 
> 
> > > 
> > > And after the discussion I had with Tom, I think it would be better if
> > > RoutingProtocol::InjectRoute() is renamed to
> > > RoutingProtocol::AddHostNetAssociation(). What do you think?
> > 
> > I think the naming conventions in NS3 forbid abbreviated names like 'Net'.
> > 
> 
> How's RoutingProtocol::AddAssociation ()? :)
> 
> > Perhaps two separate methods could be added:
> > 
> >   1. AddHostAssociation(Ipv4Address)
> > 
> >   2. AddNetworkAssociation(Ipv4Address, Ipv4Mask)
> > 
> > _however_, I normally like code that is more intelligent.  In OLSR you don't
> > have have AddOlsrInterface; it just uses all the available interfaces in the
> > node.  Likewise, perhaps an automatic approach could be used here as well?  It
> > would require the code to:
> >   1. Go to the Ipv4 interface of the node;
> >   2. Get the list of routing protocols;
> >   3. Iterate over the list and, for each "static ipv4 routing protocol":
> >     3a. Iterate over the static routing entries and generate Hna message(s)
> > based on them.
> 
> Just wondering, wouldn't it make things easier if we add a netmask field to the
> RoutingTableEntry? Anything coming in from a TC message is automatically given
> a netmask of /32 whereas for the HNA messages, we specify the netmasks
> separately?

The main problem will be speed. Right now:

  std::map<Ipv4Address, RoutingTableEntry> m_table; ///< Data structure for the routing table.

A lookup is a simple std::map::find() operation.  If you change the key matching algorithm (to network/mask matching), can you still use std::map at all?

The problem is similar as with static routing.  I see static routing iterates simply over a std::list.  It is normally not a problem with static routing if the number of entries is low.  But OLSR has lots of /32 routes for every other node in  the network (could be hundreds); it is not a good idea to make it a list.

A better solution would be to have two tables, one for OLSR nodes, and a separate table for HNA routes.  We lookup in one table first, if the lookup fails we then try the other table.

OR..... we could just add a new Ipv4StaticRouting protocol to the list of the protocols of the node and configure OLSR to inject HNA routes into that special routing table.  Problem solved ;-)

> 
> And looking at the olsrd implementation
> (http://www.olsr.org/docs/olsrd.conf.5.html), it seems like they expect the
> user to mention the associations themselves.

I won't object to manual configuration.  Not a big deal.  It's your call.

(> 
> Perhaps two separate methods could be added:
> 
>   1. AddHostAssociation(Ipv4Address)
> 
>   2. AddNetworkAssociation(Ipv4Address, Ipv4Mask)

I would probably lean towards one method, because 1) can be supported by passing an all ones mask to method 2), and because the RFC tends to deprecate using HNA to advertise hosts (Sec 12.2):

   In HNA-messages, announcing
   reachability to an address sequence through a network- and netmask
   address is typically preferred over announcing reachability to
   individual host addresses
> 
> _however_, I normally like code that is more intelligent.  In OLSR you don't
> have have AddOlsrInterface; it just uses all the available interfaces in the
> node.  Likewise, perhaps an automatic approach could be used here as well?  It
> would require the code to:
>   1. Go to the Ipv4 interface of the node;
>   2. Get the list of routing protocols;
>   3. Iterate over the list and, for each "static ipv4 routing protocol":
>     3a. Iterate over the static routing entries and generate Hna message(s)
> based on them.

That would tend to generate HNAs for all interfaces, including OLSR active ones.  Is that what you want?

It seems from the RFC that HNA is intended for the non-OLSR interfaces.  If we were to add the suggested SetInterfaceNonOlsr() method, we could perhaps do it with a flag argument
SetInterfaceNonOlsr (bool generateHna) // default true

and then have the code auto-generate HNA only for all non-OLSR configured interfaces unless disabled by the above argument.  

Then, we could keep the AddAssociation(address/mask) for an additional manual injection of an HNA.

(In reply to comment #12)
> (> 
> > Perhaps two separate methods could be added:
> > 
> >   1. AddHostAssociation(Ipv4Address)
> > 
> >   2. AddNetworkAssociation(Ipv4Address, Ipv4Mask)
> 
> I would probably lean towards one method, because 1) can be supported by
> passing an all ones mask to method 2), and because the RFC tends to deprecate
> using HNA to advertise hosts (Sec 12.2):
> 
>    In HNA-messages, announcing
>    reachability to an address sequence through a network- and netmask
>    address is typically preferred over announcing reachability to
>    individual host addresses

I think you're right.

> > 
> > _however_, I normally like code that is more intelligent.  In OLSR you don't
> > have have AddOlsrInterface; it just uses all the available interfaces in the
> > node.  Likewise, perhaps an automatic approach could be used here as well?  It
> > would require the code to:
> >   1. Go to the Ipv4 interface of the node;
> >   2. Get the list of routing protocols;
> >   3. Iterate over the list and, for each "static ipv4 routing protocol":
> >     3a. Iterate over the static routing entries and generate Hna message(s)
> > based on them.
> 
> That would tend to generate HNAs for all interfaces, including OLSR active
> ones.  Is that what you want?
> 
> It seems from the RFC that HNA is intended for the non-OLSR interfaces.  If we
> were to add the suggested SetInterfaceNonOlsr() method, we could perhaps do it
> with a flag argument
> SetInterfaceNonOlsr (bool generateHna) // default true
> 
> and then have the code auto-generate HNA only for all non-OLSR configured
> interfaces unless disabled by the above argument.  
> 
> Then, we could keep the AddAssociation(address/mask) for an additional manual
> injection of an HNA.

Yes, my idea by default was to inject routing entries only if those entries are associated with a non-OLSR interface.  I wouldn't even make it configurable.

(In reply to comment #13)


> > 
> > That would tend to generate HNAs for all interfaces, including OLSR active
> > ones.  Is that what you want?
> > 
> > It seems from the RFC that HNA is intended for the non-OLSR interfaces.  If we
> > were to add the suggested SetInterfaceNonOlsr() method, we could perhaps do it
> > with a flag argument
> > SetInterfaceNonOlsr (bool generateHna) // default true
> > 
> > and then have the code auto-generate HNA only for all non-OLSR configured
> > interfaces unless disabled by the above argument.  
> > 
> > Then, we could keep the AddAssociation(address/mask) for an additional manual
> > injection of an HNA.
> 
> Yes, my idea by default was to inject routing entries only if those entries are
> associated with a non-OLSR interface.  I wouldn't even make it configurable.

So the idea is to generate HNA messages for all routing table entries which have a non-OLSR interface as the interface field?

For example, take the following scenario:

  172.16.1.0/24   172.16.2.0/24   172.16.3.0/24
A<------------->B<------------->C<------------>D
     OLSR            non-OLSR        non-OLSR


So B should announce 172.16.2.0/24 and 172.16.3.0/24 automatically right?

Anyhow, I'll update the patch with the naming corrections and the other fixes and show it to you before proceeding with the mixed-interfaces/hna-auto-generation problem.

> So the idea is to generate HNA messages for all routing table entries which
> have a non-OLSR interface as the interface field?
> 
> For example, take the following scenario:
> 
>   172.16.1.0/24   172.16.2.0/24   172.16.3.0/24
> A<------------->B<------------->C<------------>D
>      OLSR            non-OLSR        non-OLSR
> 
> 
> So B should announce 172.16.2.0/24 and 172.16.3.0/24 automatically right?
> 

I don't know how B knows about 172.16.3.0/24 automatically.  I would assume that in this case, B would automatically associate 172.16.2.0/24 and the user would have to specify 172.16.3.0/24 as an additional associated network to B.  I would probably not make it a programming error for the user to also manually specify 172.16.2.0/24 despite it being autogenerated.

(In reply to comment #15)
> > So the idea is to generate HNA messages for all routing table entries which
> > have a non-OLSR interface as the interface field?
> > 
> > For example, take the following scenario:
> > 
> >   172.16.1.0/24   172.16.2.0/24   172.16.3.0/24
> > A<------------->B<------------->C<------------>D
> >      OLSR            non-OLSR        non-OLSR
> > 
> > 
> > So B should announce 172.16.2.0/24 and 172.16.3.0/24 automatically right?
> > 
> 
> I don't know how B knows about 172.16.3.0/24 automatically.  I would assume
> that in this case, B would automatically associate 172.16.2.0/24 and the user
> would have to specify 172.16.3.0/24 as an additional associated network to B. 
> I would probably not make it a programming error for the user to also manually
> specify 172.16.2.0/24 despite it being autogenerated.

I was wondering if B eventually learns about 172.16.3.0/24, should it announce 172.16.3.0/24 automatically?

But I think it's safer if we ensure that the user himself specify all the associations (no auto-generation of HNA messages) using a method like AddAssociation(). If required, we could later add another method that would autmoatically add associations using AddAssociation() itself? Let me know so I can update the patch. :)

And thanks for your time!

(In reply to comment #16)
> (In reply to comment #15)
> > > So the idea is to generate HNA messages for all routing table entries which
> > > have a non-OLSR interface as the interface field?
> > > 
> > > For example, take the following scenario:
> > > 
> > >   172.16.1.0/24   172.16.2.0/24   172.16.3.0/24
> > > A<------------->B<------------->C<------------>D
> > >      OLSR            non-OLSR        non-OLSR
> > > 
> > > 
> > > So B should announce 172.16.2.0/24 and 172.16.3.0/24 automatically right?
> > > 
> > 
> > I don't know how B knows about 172.16.3.0/24 automatically.  I would assume
> > that in this case, B would automatically associate 172.16.2.0/24 and the user
> > would have to specify 172.16.3.0/24 as an additional associated network to B. 
> > I would probably not make it a programming error for the user to also manually
> > specify 172.16.2.0/24 despite it being autogenerated.
> 
> I was wondering if B eventually learns about 172.16.3.0/24, should it announce
> 172.16.3.0/24 automatically?
> 
> But I think it's safer if we ensure that the user himself specify all the
> associations (no auto-generation of HNA messages) using a method like
> AddAssociation(). If required, we could later add another method that would
> autmoatically add associations using AddAssociation() itself? Let me know so I
> can update the patch. :)
> 
> And thanks for your time!

My opinion is that OLSR should announce routing entries of a static routing table, filtered to include only routing entries that reference the non-OLSR interfaces.  In this example, should the 172.16.3.0/24 network be learned by B, won't it end up in a different routing table or routing protocol?  And can't you have multiple static routing tables in a single node?

How about a couple of methods like these could make things clearer:

 1. AddHostNetworkAssociation (Ipv4Address, Ipv4Mask);

 2. AddRoutingTableAssociation (Ptr<Ipv4StaticRouting>);

The case 2 could be more efficient in case you have a static routing table with high number of entries, because you avoid copying the data from the table into the olsr agent; instead, you just copy a pointer to the table and OLSR picks the entries from there at HNA generation time.

> 
> My opinion is that OLSR should announce routing entries of a static routing
> table, filtered to include only routing entries that reference the non-OLSR
> interfaces.  In this example, should the 172.16.3.0/24 network be learned by B,
> won't it end up in a different routing table or routing protocol?  And can't
> you have multiple static routing tables in a single node?
> 
> How about a couple of methods like these could make things clearer:
> 
>  1. AddHostNetworkAssociation (Ipv4Address, Ipv4Mask);
> 
>  2. AddRoutingTableAssociation (Ptr<Ipv4StaticRouting>);
> 
> The case 2 could be more efficient in case you have a static routing table with
> high number of entries, because you avoid copying the data from the table into
> the olsr agent; instead, you just copy a pointer to the table and OLSR picks
> the entries from there at HNA generation time.

+1

Great! I'll work on it and post on the list if I have any doubts. :)

On 2/11/10 11:40 PM, Lalith Suresh wrote:
> Hello all,
> 
> Regarding the problem of OLSR running on either all or none of the
> interfaces of a node, I was wondering if it could be worked out as follows:
> 
> 1) Maintain a std::set<uint32_t>  in OlsrHelper which will contain the
> interfaces to be kept as non-OLSR.

How does OlsrHelper know which node these integers are associated with?  It seems like you would need a separate set per node.

> 2) Add a method to the helper which will allow the user to add required
> interfaces to that set.
> 3) Pass this set to the agent from within the OlsrHelper::Create() method.
> 4) In olsr::RoutingProtocol::DoStart(), we avoid creating sockets on the
> interfaces mentioned in non-OLSR-interface set.
> 
> Is this ok?
> 

What is the lower layer API for this?  Is a std::set being passed or is it just being used within the helper?

(In reply to comment #20)

> How does OlsrHelper know which node these integers are associated with?  It
> seems like you would need a separate set per node.

I'm thinking of maintaining an exception list (separate set per node). For nodes that are not included in this list, install OLSR on all interfaces, whereas for nodes in the exception list, avoid installing it on the interfaces specified.

I'm thinking of doing this with a std::map< Ptr<node>, std::set<uint32_t> >.

> What is the lower layer API for this?  Is a std::set being passed or is it just
> being used within the helper?

My current plan is to implement something like AddException(Ptr<node>,uint32_t) and using the std::set only within the helper. Should I also add AddException(Ptr<node>. std::set<uint32_t>)?

(In reply to comment #21)
> (In reply to comment #20)
> 
> > How does OlsrHelper know which node these integers are associated with?  It
> > seems like you would need a separate set per node.
> 
> I'm thinking of maintaining an exception list (separate set per node). For
> nodes that are not included in this list, install OLSR on all interfaces,
> whereas for nodes in the exception list, avoid installing it on the interfaces
> specified.
> 
> I'm thinking of doing this with a std::map< Ptr<node>, std::set<uint32_t> >.

That type of implementation seems fine to me.  I am mainly wondering about the helper API at this point, and what methods OlsrHelper provides.  Probably at a minimum, you would want to provide an OlsrHelper::ExcludeInterface (Ptr<Node>, uint32_t) but maybe also some variants that work on containers.  I might suggest to add a helper API method such as the above and revisit it if there are use cases involving containers that might argue for more public methods.

Maybe examples/wireless/mixed-wireless.cc would be a good script to examine what would be the most convenient public API for OlsrHelper.

> 
> > What is the lower layer API for this?  Is a std::set being passed or is it just
> > being used within the helper?
> 
> My current plan is to implement something like AddException(Ptr<node>,uint32_t)
> and using the std::set only within the helper. Should I also add
> AddException(Ptr<node>. std::set<uint32_t>)?

I might suggest naming it olsr::RoutingProtocol::ExcludeInterface (uint32_t) (for symmetry with "SetMainInterface") and avoid the std::set, at the lower layer API.

Created attachment 765 [details]
Adds non-OLSR interface support

Attaching a patch.

(In reply to comment #22)

> 
> That type of implementation seems fine to me.  I am mainly wondering about the
> helper API at this point, and what methods OlsrHelper provides.  Probably at a
> minimum, you would want to provide an OlsrHelper::ExcludeInterface (Ptr<Node>,
> uint32_t) but maybe also some variants that work on containers.  I might
> suggest to add a helper API method such as the above and revisit it if there
> are use cases involving containers that might argue for more public methods.
> 
> Maybe examples/wireless/mixed-wireless.cc would be a good script to examine
> what would be the most convenient public API for OlsrHelper.

I've tested against examples/wireless/wifi-simple-adhoc-grid.cc. Seems like it's working fine.

> 
> I might suggest naming it olsr::RoutingProtocol::ExcludeInterface (uint32_t)
> (for symmetry with "SetMainInterface") and avoid the std::set, at the lower
> layer API.

Done. :)

+  std::set<uint32_t> GetInterfaceExclusions ()
Method could be const.

+      const std::set<uint32_t> ifaceExclusions = GetInterfaceExclusions();
+
+      if(ifaceExclusions.find(i) != ifaceExclusions.end ())
+        continue;

It seems kind of silly to call a method to get a copy of m_interfaceExclusions, not?  Why not just access the member variable directly?

@@ -33,6 +33,12 @@
 OlsrHelper::OlsrHelper (const OlsrHelper &o)
   : m_agentFactory (o.m_agentFactory)
 {
+  std::map< Ptr<Node>, std::set<uint32_t> >::const_iterator i;
+  
+  for (i = o.m_interfaceExclusions.begin (); i != o.m_interfaceExclusions.end (); i++)
+    {
+      m_interfaceExclusions.insert (std::make_pair (i->first, std::set<uint32_t> (i->second) ));
+    }
 }
 
 OlsrHelper* 
---------
Huh?  Wouldn't just m_interfaceExclusions = o.m_interfaceExclusions work just as well?

Created attachment 766 [details]
Adds non-OLSR interface support (updated)

(In reply to comment #24)
> +  std::set<uint32_t> GetInterfaceExclusions ()
> Method could be const.
> 
> +      const std::set<uint32_t> ifaceExclusions = GetInterfaceExclusions();
> +
> +      if(ifaceExclusions.find(i) != ifaceExclusions.end ())
> +        continue;
> 
> It seems kind of silly to call a method to get a copy of m_interfaceExclusions,
> not?  Why not just access the member variable directly?
> 
> @@ -33,6 +33,12 @@
>  OlsrHelper::OlsrHelper (const OlsrHelper &o)
>    : m_agentFactory (o.m_agentFactory)
>  {
> +  std::map< Ptr<Node>, std::set<uint32_t> >::const_iterator i;
> +  
> +  for (i = o.m_interfaceExclusions.begin (); i != o.m_interfaceExclusions.end
> (); i++)
> +    {
> +      m_interfaceExclusions.insert (std::make_pair (i->first,
> std::set<uint32_t> (i->second) ));
> +    }
>  }
> 
>  OlsrHelper* 
> ---------
> Huh?  Wouldn't just m_interfaceExclusions = o.m_interfaceExclusions work just
> as well?

Wonder what I was thinking when I wrote those! Patch updated.

reassigning to gustavo as OLSR maintainer.  BTW, I am fine with the current API proposal for this aspect of the HNA addition.

The helper API is fine.  The HNA core patch itself needs to be updated, it does not apply cleanly anymore.  There were some minor issues I had found in the patch, and one big issue: it only works for Host associations, not Network associations.  Finally, a test scenario would be welcome.  Maybe the original reporter's test script modified to enable HNA support...

(In reply to comment #27)
> The helper API is fine.  The HNA core patch itself needs to be updated, it does
> not apply cleanly anymore.  There were some minor issues I had found in the
> patch, and one big issue: it only works for Host associations, not Network
> associations.  Finally, a test scenario would be welcome.  Maybe the original
> reporter's test script modified to enable HNA support...

Thanks. Will continue with the HNA core patch.

Created attachment 781 [details]
non-OLSR interface support + HNA support

Sorry for the long delay, been busy. As Gustavo suggested, I've decided to use an Ipv4StaticRouting protocol to manage the HNA routes. What would be the best way to instantiate the required Ipv4StaticRouting instance to be used for this? I don't think it's a good idea to rely on the user to add an extra Ipv4StaticRouting protocol to the list of protocols and then probably call a method that would state this instance as the HNA route manager?

I'm attaching an implementation in which I've added an IpvStaticRouting protocol instance named HnaRoutingTable as a private member to olsr::RoutingProtocol(). It seems to work, but I'm not sure if its a good way to go about it. Currently, the only way to add Associations is using AddHostNetworkAssociation(). I haven't implemented AddRoutingTableAssociation() yet.

And should I add methods to the helper to allow injection of Associations?

(In reply to comment #29)
> Created an attachment (id=781) [details]
> non-OLSR interface support + HNA support
> 
> Sorry for the long delay, been busy. As Gustavo suggested, I've decided to use
> an Ipv4StaticRouting protocol to manage the HNA routes. What would be the best
> way to instantiate the required Ipv4StaticRouting instance to be used for this?
> I don't think it's a good idea to rely on the user to add an extra
> Ipv4StaticRouting protocol to the list of protocols and then probably call a
> method that would state this instance as the HNA route manager?

I think that whatever object in which you decide to keep these associations stored should be allocated by OLSR and not passed in by the user.  

I don't know whether it makes sense to also allow a routing object to be passed in to OLSR in addition to the one that OLSR maintains, if it is easier for the user than adding each association incrementally.  For starters, I would just have users pass these associations in one-by-one.

> 
> I'm attaching an implementation in which I've added an IpvStaticRouting
> protocol instance named HnaRoutingTable as a private member to
> olsr::RoutingProtocol(). It seems to work, but I'm not sure if its a good way
> to go about it. 

That class Ipv4StaticRouting derives from ns3::Object and is meant to be instantiated dynamically so I would suggest that you store instead a Ptr<Ipv4StaticRouting> and create it in the constructor (and dispose of it properly in the destructor).

> Currently, the only way to add Associations is using
> AddHostNetworkAssociation(). I haven't implemented AddRoutingTableAssociation()
> yet.
> 
> And should I add methods to the helper to allow injection of Associations?

I think that a first issue to deal with at the helper layer is to allow a user to get a handle to a specific Olsr on a specific node (presently, the helper lacks this API).

You should not call a member variable HnaRoutingTable.  You know this, right?  It should be m_hnaRoutingTable.  Tom is right, Object instances should always be handled via Ptr smart pointer, never allocated directly like this.

I don't get the logic of ProcessHna()/SendHna().  SendHna sends the HNA entries stored in m_state.  ProcessHna() reads entries from a HNA message and stores them in m_state.  In the next timeout, a node will advertise HNA messages that it received from another node as its own.  This is wrong.  HNA messages from other nodes are forwarded intact using the default forwarding algorithm (in fact, they are already).  Excuse me if I read the source code wrong.

I see no implementation of AddRoutingTableAssociation, only the method declared.  I was expecting that AddRoutingTableAssociation would store the pointer to the Ipv4StaticRouting internally.  Then SendHna would grab the routing table entries from there, excluding entries that mention excluded interfaces.

Also keep in mind not to use underscored_variable_names.  I know there are some already in OLSR, but it's just that I missed them when converting the code from the NS-2 patch.

(In reply to comment #31)
> You should not call a member variable HnaRoutingTable.  You know this, right? 
> It should be m_hnaRoutingTable.  Tom is right, Object instances should always
> be handled via Ptr smart pointer, never allocated directly like this.

Yes I knew it, silly me! Will change to m_hnaRoutingTable. And I'll also make the required conversions to Ptr smart pointers.

> 
> I don't get the logic of ProcessHna()/SendHna().  SendHna sends the HNA entries
> stored in m_state.  ProcessHna() reads entries from a HNA message and stores
> them in m_state.  In the next timeout, a node will advertise HNA messages that
> it received from another node as its own.  This is wrong.  HNA messages from
> other nodes are forwarded intact using the default forwarding algorithm (in
> fact, they are already).  Excuse me if I read the source code wrong.

Actually, notice that SendHna is reading entries from m_associations and ProcessHna is storing entries from the HNA message into m_associationSet. So that ProcessHna-SendHna-Loop you guessed would occur isn't happening. Yes, the names are confusing, but this is what was decided in the previous discussions (comment #9).

> 
> I see no implementation of AddRoutingTableAssociation, only the method
> declared.  I was expecting that AddRoutingTableAssociation would store the
> pointer to the Ipv4StaticRouting internally.  Then SendHna would grab the
> routing table entries from there, excluding entries that mention excluded
> interfaces.
> 

I'll add that as well and submit the patch soon.

> Also keep in mind not to use underscored_variable_names.  I know there are some
> already in OLSR, but it's just that I missed them when converting the code from
> the NS-2 patch.

Will correct that too.

Thanks for the review. :)

Created attachment 786 [details]
 non-OLSR interface support + HNA support

Implemented AddRoutingTableAssociation () and also made corrections as per previous review.

Created attachment 787 [details]
non-OLSR interface support + HNA support   

Updated version.

///
/// \brief Adds an Ipv4StaticRouting protocol Association
///        can generate an HNA message for
///
void
RoutingProtocol::AddRoutingTableAssociation (Ptr<Ipv4StaticRouting> routingTable)
{
  m_routingTableAssociation = routingTable;
}

The naming does not agree with the function.  If there is only one static table that can be used as source for HNA messages, then the method should be SetRoutingTableAssociation.  If you wish to keep AddRoutingTableAssociation then you need to provide an implementation that can really accept multiple tables, not just one.

Also, I don't see the point of this method and AddHostNetworkAssociation to be declared as private.  Isn't this supposed to be used by the simulation scripts?

In olsr-state.h:

+  AssociationSet m_associationSet;	///< Association Set (RFC 3626, section 12.2)
+  Associations m_associations;	///< The Host Network Associations of the node

I think you should revise the comments to make it clearer the difference between the two associations.  I suggest for instance:

+  AssociationSet m_associationSet;	///< Association Set (RFC 3626, section 12.2).  These associations are the result of HNA messages received from other OLSR nodes.
+  Associations m_associations;	///< The local Host Network Associations of the node that it will advertise to the network using OLSR HNA messages.


[in several places] Please don't use this construct to initialize a structure tmp value:
+            associations.push_back((olsr::MessageHeader::Hna::Association){route.GetDestNetwork (), route.GetDestNetworkMask ()});

I think this is a GCC extension. Instead use the typical notation with an intermediate variable (hopefully the optimized code will be the same):

+ olsr::MessageHeader::Hna::Association assoc = {route.GetDestNetwork (), route.GetDestNetworkMask ()};
+ associations.push_back (assoc);

Finally, we could really use an example program that uses this, to test the implementation and exemplify the API.  We can't be sure this works without an example.  In fact, with those methods private, I suspect it doesn't do anything for now... :-)

(In reply to comment #35)
> ///
> /// \brief Adds an Ipv4StaticRouting protocol Association
> ///        can generate an HNA message for
> ///
> void
> RoutingProtocol::AddRoutingTableAssociation (Ptr<Ipv4StaticRouting>
> routingTable)
> {
>   m_routingTableAssociation = routingTable;
> }
> 
> The naming does not agree with the function.  If there is only one static table
> that can be used as source for HNA messages, then the method should be
> SetRoutingTableAssociation.  If you wish to keep AddRoutingTableAssociation
> then you need to provide an implementation that can really accept multiple
> tables, not just one.
> 
> Also, I don't see the point of this method and AddHostNetworkAssociation to be
> declared as private.  Isn't this supposed to be used by the simulation scripts?

Actually, I kept testing by using these methods from within the agent code itself. My bad! As you've suggested, I'll prepare an example script and attach that as well next.

> 
> In olsr-state.h:
> 
> +  AssociationSet m_associationSet;    ///< Association Set (RFC 3626, section
> 12.2)
> +  Associations m_associations;    ///< The Host Network Associations of the
> node
> 
> I think you should revise the comments to make it clearer the difference
> between the two associations.  I suggest for instance:
> 
> +  AssociationSet m_associationSet;    ///< Association Set (RFC 3626, section
> 12.2).  These associations are the result of HNA messages received from other
> OLSR nodes.
> +  Associations m_associations;    ///< The local Host Network Associations of
> the node that it will advertise to the network using OLSR HNA messages.
> 

Ok.

> 
> [in several places] Please don't use this construct to initialize a structure
> tmp value:
> +           
> associations.push_back((olsr::MessageHeader::Hna::Association){route.GetDestNetwork
> (), route.GetDestNetworkMask ()});
> 
> I think this is a GCC extension. Instead use the typical notation with an
> intermediate variable (hopefully the optimized code will be the same):
> 
> + olsr::MessageHeader::Hna::Association assoc = {route.GetDestNetwork (),
> route.GetDestNetworkMask ()};
> + associations.push_back (assoc);

Ok.

> 
> Finally, we could really use an example program that uses this, to test the
> implementation and exemplify the API.  We can't be sure this works without an
> example.  In fact, with those methods private, I suspect it doesn't do anything
> for now... :-)

Will do that as well! :)

Created attachment 788 [details]
non-OLSR interface support + HNA support

Made the corrections suggested by Gustavo.

Created attachment 789 [details]
Test script

Here's a test script which uses methods SetRoutingTableAssociation () and AddHostNetworkAssociation (). It is adapted from examples/wireless/wifi-simple-adhoc.cc.

Topology:

          OLSR                non-OLSR
  A ))))         (((( B ------------------- C
10.1.1.1     10.1.1.2   172.16.1.2     172.16.1.1

A needs to send a packet to C. This can be done only once A receives an HNA message from B, which provides A the necessary association.

To run the script, do:
$: ./waf --run "hna-test-script --assocMethod1=<1/0> --assocMethod2=<1/0>"

--assocMethod1=1 uses SetRoutingTableAssociation ()
--assocMethod2=1 uses AddHostNetworkAssociation ()

Setting either or both allows the packet to be sent by A and to be received by C via B.

Hope this is ok.

This does not build with ns-3-dev:

[1051/1196] cxx: scratch/hna-test-script.cc -> build/debug/scratch/hna-test-script_3.o
../scratch/hna-test-script.cc: In function ‘int main(int, char**)’:
../scratch/hna-test-script.cc:147: error: ‘class ns3::YansWifiPhyHelper’ has no member named ‘SetPcapFormat’
../scratch/hna-test-script.cc:147: error: ‘PCAP_FORMAT_80211_RADIOTAP’ is not a member of ‘ns3::YansWifiPhyHelper’
../scratch/hna-test-script.cc:181: error: ‘class ns3::OlsrHelper’ has no member named ‘ExcludeInterface’
Waf: Leaving directory `/home/gjc/projects/ns/ns-3-allinone/ns-3-dev/build'
Build failed

But I looked at the HNA part of the example and it looks good.  The core patch itself looks good also.  You just need to make the example build with ns-3-dev, put it as an example (examples/routing/olsr-hna.cc) and we're good to go as far as I can tell.

Then there is the issue of whether we can merge this for NS 3.8 or 3.9.  Big feature merge period has ended already.  I guess this i not so big feature, so maybe we can merge it for 3.8.  This does not change existing API nor regression traces, so I guess it should be safe to merge the patch.

  March 8 -- Begin the phase of small feature development and bug fixing
  March 27 -- Small feature development and bug fixing ends

I guess we are on schedule to merge this HNA patch for 3.8.

Created attachment 790 [details]
non-OLSR interface support, HNA support and example script

Final patch. Includes:

1) non-OLSR interface support with helper API.
2) HNA core patch.
3) Sample script: examples/routing/olsr-hna.cc

Also compiled and tested against latest ns-3-dev.

(In reply to comment #40)
> Created an attachment (id=790) [details]
> non-OLSR interface support, HNA support and example script
> 
> Final patch. Includes:
> 
> 1) non-OLSR interface support with helper API.
> 2) HNA core patch.
> 3) Sample script: examples/routing/olsr-hna.cc
> 
> Also compiled and tested against latest ns-3-dev.

When I run with no arguments and with NS_LOG=OlsrRoutingProtocol, it crashes here:

2964	      NS_LOG_DEBUG ("Found route to " << rtentry->GetDestination () << " via nh " << rtentry->GetGateway () << " with source addr " << rtentry->GetSource () << " and output dev " << rtentry->GetOutputDevice());

That's because rtentry is a NULL pointer.  Yes, I know, I must pass one of the command-line arguments, my fault.  But still would be nice to not crash..

Otherwise, no comments.  I'll wait a few days, then commit if no objections.  Thanks for the hard work! :-)

Created attachment 791 [details]
non-OLSR interface support, HNA support and example script

The final _final_ patch. :)

My bad actually, the NS_LOG_DEBUG statement should have been inside the if() block. Fixed.

And thanks for your patience! :)

changeset:   6140:12bb87242796
tag:         tip
user:        Lalith Suresh  <suresh.lalith@gmail.com>
date:        Wed Mar 17 15:46:42 2010 +0000
summary:     Bug 407 - OLSR is missing HNA support

(In reply to comment #42)
> 
> And thanks for your patience! :)

Thank you, looks great!

Created attachment 796 [details]
MID fix

Sorry for not noticing this earlier, but seems like MID messages are being generated for non-OLSR interfaces. Attaching the fix. Also removes two stale comments.

Comment on attachment 796 [details]
MID fix

committed, thanks!