Bug 735

Created attachment 643 [details]
fix

(In reply to comment #0)
> Created an attachment (id=642) [details]
> OLSR routing usage example
> 
> OLSR routing doesn't work at all since no packets are locally delivered in
> RouteInput() methods (lcb callback is not used).

I would clarify instead that OLSR routing does not work outside of the ListRouting context (which has been documented).  But OLSR does work with the current OlsrHelper and examples that insert Olsr into a list routing protocol.  The Ipv4ListRouting class supports local delivery-- those packets never make it to OLSR.

> 
> Attached olsr.cc example demonstrates this. Attached olsr.patch fixes this
> (contains examples/routing/olsr.cc as well).
> 

>+  // Local delivery
>+  NS_ASSERT (m_ipv4->GetInterfaceForDevice (idev) >= 0);
>+  int32_t iif = m_ipv4->GetInterfaceForDevice (idev);
>+  for (std::map<Ptr<Socket> , Ipv4InterfaceAddress>::const_iterator j = m_socketAddresses.begin (); 
>+      j != m_socketAddresses.end (); ++j)
>+    {
>+      Ipv4InterfaceAddress iface = j->second;
>+      if (dst == iface.GetLocal () || dst == iface.GetBroadcast ())
>+        {
>+          NS_LOG_LOGIC ("Local delivery to " << iface.GetLocal () << ":" << iface.GetBroadcast ());
>+          lcb (p, header, iif);
>+          return true;
>+        }
>+    }

This logic is similar but not identical to ipv4-list-routing-protocol.cc.  I didn't copy the ipv4-list-routing-protocol.cc logic into olsr a while back because I wasn't sure that it was a good idea to clone this type of code all around the routing protocols.  Instead, I think that some kind of helper function that encapsulated the above in one place (such as non-member non-friend in ipv4.{h,cc}) could be reusable across protocols.  

  Tom,

  thank you for clarifying this point.

> I would clarify instead that OLSR routing does not work outside of the
> ListRouting context (which has been documented).  

  Please point me that documentation, I've definitely missed it. 

> This logic is similar but not identical to ipv4-list-routing-protocol.cc.  I
> didn't copy the ipv4-list-routing-protocol.cc logic into olsr a while back
> because I wasn't sure that it was a good idea to clone this type of code all
> around the routing protocols.  Instead, I think that some kind of helper
> function that encapsulated the above in one place (such as non-member
> non-friend in ipv4.{h,cc}) could be reusable across protocols.  

  I didn't catch your resolution. Do you prefer to leave local delivery to list routing? If so -- please document this decision in ipv4-routing-protocol.h and I will remove local delivery code from AODV. If not, do you plan to write this helper function to be used by all routing protocols to locally deliver packets?

  For now I have AODV which can work standalone and OLSR which can't, this is not convenient.  

(In reply to comment #3)
>   Tom,
> 
>   thank you for clarifying this point.
> 
> > I would clarify instead that OLSR routing does not work outside of the
> > ListRouting context (which has been documented).  
> 
>   Please point me that documentation, I've definitely missed it. 

It is in the manual, in the OLSR section:
http://www.nsnam.org/docs/manual/manual_95.html#SEC161

but I don't see it now documented in src/routing/olsr code.  So I see how this detail could be missed.

> 
> > This logic is similar but not identical to ipv4-list-routing-protocol.cc.  I
> > didn't copy the ipv4-list-routing-protocol.cc logic into olsr a while back
> > because I wasn't sure that it was a good idea to clone this type of code all
> > around the routing protocols.  Instead, I think that some kind of helper
> > function that encapsulated the above in one place (such as non-member
> > non-friend in ipv4.{h,cc}) could be reusable across protocols.  
> 
>   I didn't catch your resolution. Do you prefer to leave local delivery to list
> routing? 

No, I support your proposal to make all of these protocols operate standalone-- I just would prefer to factor out the common logic such as deciding whether an address belongs to the node.

> If so -- please document this decision in ipv4-routing-protocol.h and
> I will remove local delivery code from AODV. If not, do you plan to write this
> helper function to be used by all routing protocols to locally deliver packets?
> 
>   For now I have AODV which can work standalone and OLSR which can't, this is
> not convenient.  

Agreed.  I will suggest a small revision and post it here.

Created attachment 647 [details]
Ipv4 patch for assisting in local delivery

So that each routing protocol doesn't have to reimplement similar logic, this patch adds a method to class Ipv4 called IsDestinationAddress(address, iif) that checks whether the address for a packet arriving on interface iif is acceptable for local delivery.  It also adds an attribute that can toggle the RFC1122 strong end host/weak end host behavior for this method (namely, the default weak end host allows a host to receive packets on an interface even if the destination address belongs to a different interface).  Finally, it moves this logic out of Ipv4ListRoutingProtocol.

Created attachment 648 [details]
revised fix for OLSR

This Olsr fix makes use of the previous patch, to enable local delivery.

Created attachment 649 [details]
revised OLSR routing usage example

This has a few minor revisions to Pavel's example:
- start the ping a little bit later than time zero (15 seconds by default), allowing OLSR to have a chance to converge
- attach a debugging method to the Ping Rtt callback so that user can see the successful ping result
- add ascii trace capability

dropping priority according to 
http://www.nsnam.org/wiki/index.php/User_FAQ#Bug_Priorities

I think there need to be a couple of things done still to finish this off:

1) these methods are null in OlsrRoutingProtocol

void
RoutingProtocol::NotifyInterfaceUp (uint32_t i)
{}
void
RoutingProtocol::NotifyInterfaceDown (uint32_t i)
{}
void
RoutingProtocol::NotifyAddAddress (uint32_t interface, Ipv4InterfaceAddress address)
{}
void
RoutingProtocol::NotifyRemoveAddress (uint32_t interface, Ipv4InterfaceAddress address)
{}

What should be the behavior for AddAddress or NotifyInterfaceUp?  If a new interface or a zeroth address on an interface being added, should we add to m_socketAddresses and open a new socket?


2) I have a question about this code:

+  if (IsMyOwnAddress (origin)) 
+    return true; // ignore own packets
+  
+bool
+RoutingProtocol::IsMyOwnAddress (const Ipv4Address & a) const
+{
+  for (std::map<Ptr<Socket> , Ipv4InterfaceAddress>::const_iterator j =
+      m_socketAddresses.begin (); j != m_socketAddresses.end (); ++j)+    {
+      Ipv4InterfaceAddress iface = j->second;+      if (a == iface.GetLocal ())
+        {
+          return true; 
+        }
+    }+  return false;
+}
+

Do we need this check?  Will it block a node from opening a socket to another one of its own addresses?

3) we probably should document that OLSR only works with the zero-th IP address assigned to an interface (if multiple are assigned)

(In reply to comment #5)
> Created an attachment (id=647) [details]
> Ipv4 patch for assisting in local delivery
> 
> So that each routing protocol doesn't have to reimplement similar logic, this
> patch adds a method to class Ipv4 called IsDestinationAddress(address, iif)
> that checks whether the address for a packet arriving on interface iif is
> acceptable for local delivery.  It also adds an attribute that can toggle the
> RFC1122 strong end host/weak end host behavior for this method (namely, the
> default weak end host allows a host to receive packets on an interface even if
> the destination address belongs to a different interface).  Finally, it moves
> this logic out of Ipv4ListRoutingProtocol.
> 

  Tom,

  patch looks good, please commit and I will make AODV to use it. I have just two minor questions:

+      if (address.IsEqual (iaddr.GetLocal ()))

  why not == ?

+  if (GetWeakEsModel () == true)  // Check other interfaces

  == true is weird. Also in "weak end host" mode you will not detect broadcast addresses of other interfaces -- does this correspond to RFC1122?

(In reply to comment #6)
> Created an attachment (id=648) [details]
> revised fix for OLSR
> 
> This Olsr fix makes use of the previous patch, to enable local delivery.
> 

  good, please commit. One can note that this change:

-  std::map< Ptr<Socket>, Ipv4Address > m_socketAddresses;
+  std::map< Ptr<Socket>, Ipv4InterfaceAddress > m_socketAddresses;

  is now unused (it was done to detect broadcast addresses of OLSR interfaces, this is hidden in IsDestinationAddress() now), but I still suggest to keep it for future needs.  

(In reply to comment #7)

  Tom, 

> This has a few minor revisions to Pavel's example:
> - start the ping a little bit later than time zero (15 seconds by default),
> allowing OLSR to have a chance to converge

  Actually I was interested exactly about OLSR convergence, so I'd like to remove this extra delay. 

> - attach a debugging method to the Ping Rtt callback so that user can see the
> successful ping result

  Please, do not tear class comment from the class keyword, doxygen will not find it. Also I suggest to merge with AODV branch, in addition to aodv it adds verbose ping option which produce familiar output like this:

#./waf --run olsr
Creating 4 nodes 120 m apart.
Starting simulation for 60 s ...
PING  10.0.0.4 56(84) bytes of data.
64 bytes from 10.0.0.4: icmp_seq=11 ttl=62 time=13 ms
64 bytes from 10.0.0.4: icmp_seq=12 ttl=62 time=3 ms
64 bytes from 10.0.0.4: icmp_seq=13 ttl=62 time=4 ms
64 bytes from 10.0.0.4: icmp_seq=14 ttl=62 time=4 ms
64 bytes from 10.0.0.4: icmp_seq=15 ttl=62 time=4 ms
64 bytes from 10.0.0.4: icmp_seq=16 ttl=62 time=3 ms
64 bytes from 10.0.0.4: icmp_seq=17 ttl=62 time=4 ms
64 bytes from 10.0.0.4: icmp_seq=18 ttl=62 time=4 ms
64 bytes from 10.0.0.4: icmp_seq=19 ttl=62 time=3 ms
--- 10.0.0.4 ping statistics ---
60 packets transmitted, 9 received, 85% packet loss, time 60000ms
rtt min/avg/max/mdev = 3/4.667/13/1.054 ms
 
  I think this is better for example script than your PingRtt function. 
  
> - add ascii trace capability

  Good.

  So, now I'd like to: 1) remove ping start delay 2) remove PinRtt function 3) keep ascii option. 

  In the future (== after AODV will be merged) I'd like to make this example MANET generic with --protocol command line option.  

(In reply to comment #9)
> I think there need to be a couple of things done still to finish this off:
> 
> 1) these methods are null in OlsrRoutingProtocol

  I can say what they are doing in AODV model:

> void
> RoutingProtocol::NotifyInterfaceUp (uint32_t i)
> {}

  if not loopback, open socket, add address to m_socketAddresses and connect to MAC layer notifications. 

> void
> RoutingProtocol::NotifyInterfaceDown (uint32_t i)
> {}

  disconnect from MAC layer notifications, close socket and erase record from m_socketAddresses.

> void
> RoutingProtocol::NotifyAddAddress (uint32_t interface, Ipv4InterfaceAddress
> address)
> {}

  if this is the only address of this interface, open socket and add it to m_socketAddresses.

> void
> RoutingProtocol::NotifyRemoveAddress (uint32_t interface, Ipv4InterfaceAddress
> address)
> {}

  if this interface is used by AODV, close socket and erase it from m_socketAddresses.  If interface still has addresses, open socket and add it to m_socketAddresses using new 0-th address.

> What should be the behavior for AddAddress or NotifyInterfaceUp?  If a new
> interface or a zeroth address on an interface being added, should we add to
> m_socketAddresses and open a new socket?

  The logic above is somehow arbitrary and I am neutral about reproducing it in OLSR or not. 

 
> 2) I have a question about this code:
> 
> +  if (IsMyOwnAddress (origin)) 
> +    return true; // ignore own packets
> +  
> +bool
> +RoutingProtocol::IsMyOwnAddress (const Ipv4Address & a) const
> +{
> +  for (std::map<Ptr<Socket> , Ipv4InterfaceAddress>::const_iterator j =
> +      m_socketAddresses.begin (); j != m_socketAddresses.end (); ++j)+    {
> +      Ipv4InterfaceAddress iface = j->second;+      if (a == iface.GetLocal
> ())
> +        {
> +          return true; 
> +        }
> +    }+  return false;
> +}
> +
> 
> Do we need this check?  Will it block a node from opening a socket to another
> one of its own addresses?

  Now I think it is useless and can be removed, sorry for confusing you. 

> 3) we probably should document that OLSR only works with the zero-th IP address
> assigned to an interface (if multiple are assigned)

  Sure.

(In reply to comment #10)
> (In reply to comment #5)
> > Created an attachment (id=647) [details] [details]
> > Ipv4 patch for assisting in local delivery
> > 
> > So that each routing protocol doesn't have to reimplement similar logic, this
> > patch adds a method to class Ipv4 called IsDestinationAddress(address, iif)
> > that checks whether the address for a packet arriving on interface iif is
> > acceptable for local delivery.  It also adds an attribute that can toggle the
> > RFC1122 strong end host/weak end host behavior for this method (namely, the
> > default weak end host allows a host to receive packets on an interface even if
> > the destination address belongs to a different interface).  Finally, it moves
> > this logic out of Ipv4ListRoutingProtocol.
> > 
> 
>   Tom,
> 
>   patch looks good, please commit and I will make AODV to use it. I have just
> two minor questions:
> 
> +      if (address.IsEqual (iaddr.GetLocal ()))
> 
>   why not == ?

(trying to remember why this is like this...) I think that some mask comparisons use some similar syntax, but I agree that == is more readable.

> 
> +  if (GetWeakEsModel () == true)  // Check other interfaces
> 
>   == true is weird. 

OK
> Also in "weak end host" mode you will not detect broadcast
> addresses of other interfaces -- does this correspond to RFC1122?
> 

It is compliant, but not consistent with unicast for Weak ES.  I wasn't sure whether the rules still apply for broadcast, so I went back to look at what BSD (Stevens) did and they discarded such, so I did likewise.  I do not care too much about this corner case, but I will take a look at Linux behavior before concluding (and align with Linux).

(In reply to comment #12)
> (In reply to comment #7)
> 
>   Tom, 
> 
> > This has a few minor revisions to Pavel's example:
> > - start the ping a little bit later than time zero (15 seconds by default),
> > allowing OLSR to have a chance to converge
> 
>   Actually I was interested exactly about OLSR convergence, so I'd like to
> remove this extra delay. 
> 
> > - attach a debugging method to the Ping Rtt callback so that user can see the
> > successful ping result
> 
>   Please, do not tear class comment from the class keyword, doxygen will not
> find it. Also I suggest to merge with AODV branch, in addition to aodv it adds
> verbose ping option which produce familiar output like this:
> 
> #./waf --run olsr
> Creating 4 nodes 120 m apart.
> Starting simulation for 60 s ...
> PING  10.0.0.4 56(84) bytes of data.
> 64 bytes from 10.0.0.4: icmp_seq=11 ttl=62 time=13 ms
> 64 bytes from 10.0.0.4: icmp_seq=12 ttl=62 time=3 ms
> 64 bytes from 10.0.0.4: icmp_seq=13 ttl=62 time=4 ms
> 64 bytes from 10.0.0.4: icmp_seq=14 ttl=62 time=4 ms
> 64 bytes from 10.0.0.4: icmp_seq=15 ttl=62 time=4 ms
> 64 bytes from 10.0.0.4: icmp_seq=16 ttl=62 time=3 ms
> 64 bytes from 10.0.0.4: icmp_seq=17 ttl=62 time=4 ms
> 64 bytes from 10.0.0.4: icmp_seq=18 ttl=62 time=4 ms
> 64 bytes from 10.0.0.4: icmp_seq=19 ttl=62 time=3 ms
> --- 10.0.0.4 ping statistics ---
> 60 packets transmitted, 9 received, 85% packet loss, time 60000ms
> rtt min/avg/max/mdev = 3/4.667/13/1.054 ms
> 
>   I think this is better for example script than your PingRtt function. 

I agree, but I wasn't seeing this repetitive Ping behavior-- I was only seeing one Ping being sent at time zero.  Is the above trace from a different Ping than the one in ns-3-dev?

> 
> > - add ascii trace capability
> 
>   Good.
> 
>   So, now I'd like to: 1) remove ping start delay 2) remove PinRtt function 3)
> keep ascii option. 
> 
>   In the future (== after AODV will be merged) I'd like to make this example
> MANET generic with --protocol command line option.  

OK

> 

  Tom, 

> I agree, but I wasn't seeing this repetitive Ping behavior-- I was only seeing
> one Ping being sent at time zero.  Is the above trace from a different Ping
> than the one in ns-3-dev?

  Yes, this is a little bit enhanced ping from https://forge.iitp.ru/hgprojects/ns3aodv repository, you can see the changes here: http://codereview.appspot.com/115075/diff/11011/11014 I didn't advertise this enhancements as separate feature since I hope that this is a temporary solution and the final one should be real ping code ported to ns-3. 

  By the way, we have answered all your comments in aodv review cited above (it was really useful, thank you). That's why I suggest to merge aodv repository and do not reinvent verbose output from ping.   

(In reply to comment #13)
> (In reply to comment #9)
> > 2) I have a question about this code:
> > 
> > +  if (IsMyOwnAddress (origin)) 
> > +    return true; // ignore own packets
> > +  
> > +bool
> > +RoutingProtocol::IsMyOwnAddress (const Ipv4Address & a) const
> > +{
> > +  for (std::map<Ptr<Socket> , Ipv4InterfaceAddress>::const_iterator j =
> > +      m_socketAddresses.begin (); j != m_socketAddresses.end (); ++j)+    {
> > +      Ipv4InterfaceAddress iface = j->second;+      if (a == iface.GetLocal
> > ())
> > +        {
> > +          return true; 
> > +        }
> > +    }+  return false;
> > +}
> > +
> > 
> > Do we need this check?  Will it block a node from opening a socket to another
> > one of its own addresses?
> 
>   Now I think it is useless and can be removed, sorry for confusing you. 

  Tom,

  playing with OLSR in multi-interface configuration I recall why did I introduce that "IsMyOwnAddress" check. It is needed when you have configured 2-interface node with both interfaces working with the same WiFi channel number. In this case one interface should drop all packets from the other one -- this is done by IsMyOwnAddress() check.  

  Probably you can object that this case is a configuration error (who needs two interfaces on the same channel), but I think that protocol must correctly work in all cases.

(In reply to comment #9)
> I think there need to be a couple of things done still to finish this off:
> 
> 1) these methods are null in OlsrRoutingProtocol
[...] 
> 2) I have a question about this code:
[...]
> 3) we probably should document that OLSR only works with the zero-th IP address
> assigned to an interface (if multiple are assigned)

  Tom,

  I have created some basic trace-based OLSR system tests and want to push them just to start that test suite. To operate they need your patches from this bug which make OLSR self-consistent. Could you commit them moving open questions cited above to separate bug?

Pavel, the latest patch makes these changes:

> 
>   patch looks good, please commit and I will make AODV to use it. I have just
> two minor questions:
> 
> +      if (address.IsEqual (iaddr.GetLocal ()))
> 
>   why not == ?
> 

changed

> +  if (GetWeakEsModel () == true)  // Check other interfaces
> 
>   == true is weird. 

changed

> Also in "weak end host" mode you will not detect broadcast
> addresses of other interfaces -- does this correspond to RFC1122?

I changed this to be more permissive, and the doxygen for IsDestinationAddress() correspondingly.  I don't care too much about this case.

I also updated CHANGES.html.

Created attachment 664 [details]
Ipv4 patch for assisting in local delivery

(In reply to comment #18)
> (In reply to comment #9)
> > I think there need to be a couple of things done still to finish this off:
> > 
> > 1) these methods are null in OlsrRoutingProtocol
> [...] 
> > 2) I have a question about this code:
> [...]
> > 3) we probably should document that OLSR only works with the zero-th IP address
> > assigned to an interface (if multiple are assigned)
> 
>   Tom,
> 
>   I have created some basic trace-based OLSR system tests and want to push them
> just to start that test suite. To operate they need your patches from this bug
> which make OLSR self-consistent. Could you commit them moving open questions
> cited above to separate bug?

I just posted another revised patch based on your comments.  I noticed one trace regression (csma-multicast) that I should fix before pushing it.  

To be clear, I think you are asking me to push the first patch (with new function Ipv4::IsDestinationAddress()) and the second patch (Olsr alignment), but forget about the third (olsr example) which you will separately push at some point?  And there remains the issue of filling in the missing functions in OLSR for interface up/down detection, which can be left as an open issue.

If so, I will try to push those two patches by tomorrow or Friday (once I resolve the regression)

> To be clear, I think you are asking me to push the first patch (with new
> function Ipv4::IsDestinationAddress()) and the second patch (Olsr alignment),
> but forget about the third (olsr example) which you will separately push at
> some point?  

  Exactly. 

> And there remains the issue of filling in the missing functions in
> OLSR for interface up/down detection, which can be left as an open issue.

  Let's open new bug for that issues (e.g. "OLSR doesn't correctly handle interface up/down").
 
> If so, I will try to push those two patches by tomorrow or Friday (once I
> resolve the regression)

  Good, thank you!

(In reply to comment #22)
> > To be clear, I think you are asking me to push the first patch (with new
> > function Ipv4::IsDestinationAddress()) and the second patch (Olsr alignment),
> > but forget about the third (olsr example) which you will separately push at
> > some point?  
> 
>   Exactly. 
> 
> > And there remains the issue of filling in the missing functions in
> > OLSR for interface up/down detection, which can be left as an open issue.
> 
>   Let's open new bug for that issues (e.g. "OLSR doesn't correctly handle
> interface up/down").
> 
> > If so, I will try to push those two patches by tomorrow or Friday (once I
> > resolve the regression)
> 
>   Good, thank you!

OK, this is done (ns-3-dev: 55f21c521021) and bug 745 for the remaining open issues.