Bug 592

I have not looked at the details but it would be safer to make it Ptr<const Packet> to avoid weird interactions between the caller and callee. The callee can call Packet::Copy to get a non-const version if needed.

Feel free to ignore the above is you envision cases where it's important that the caller and callee share the same packet.

Currently I am using Ptr<Packet> in the RouteOutput signature because I am actually adding a NixVector pointer to the packet (rather than using packet tags).  If packet tags were used in the future for nix-vector routing, then a const packet could work, but I ran into some problems with packet tags (my base nix-vector, ie. no entries, is already > 20 bytes).  So I took a simpler approach for now just so I could try to get it working.

(In reply to comment #1)
> I have not looked at the details but it would be safer to make it Ptr<const
> Packet> to avoid weird interactions between the caller and callee. The callee
> can call Packet::Copy to get a non-const version if needed.
> 
> Feel free to ignore the above is you envision cases where it's important that
> the caller and callee share the same packet.
> 

I prefer to change this to Ptr<Packet>; callee is allowed to modify the packet (such as an explicit source routing header).  If caller doesn't like this behavior, can call Packet::Copy itself.  

Created attachment 476 [details]
caller graph of RouteOutput()

Attached is a graph of a list of caller methods that call the ns3::Ipv4RoutingProtocol::RouteOutput() method. I purpose to change the signature of ns3::Ipv4RoutingProtocol::RouteOutput() to

RouteOutput(const Ipv4Header &header, uint32_t oif, Socket::SocketErrno &sockerr, Ptr<const Packet> p = 0)

and modify all its callers accordingly.

The Ptr<const Packet> is added to the back as I would like the new argument to be an optional argument, and thus keep being backward compatible. 

Any comments? And, btw, does backward compatibility a requirement? 

Either Ptr<const Packet> or Ptr<Packet> is equally perfect for me. ^^

Created attachment 478 [details]
Patch for bug 592

Added a patch for consideration for this bug.  All of the RouteOutput declarations and calls have been changed to include a Ptr<Packet> parameter.  This compiles and passes regression tests (with the exception of test-wifi-wired-bridging, which is failing for me in ns-3-dev)  A few things worth noting:

1) In arp-l3-protocol.cc I had to move where the packet was created in order to add it to the RouteOutput function.  I don't think this is a problem.

2) In tcp-socket-impl.cc I created a null packet pointer to be used for RouteOutput, because at this point there is no packet.  I wasn't sure if this was the best thing to do.

3) Every other change was minimal.  If there was a call to RouteOutput, I grabbed the packet from either the calling function parameters, or from the newly created packet.

(In reply to comment #4)
> Created an attachment (id=476) [details]
> caller graph of RouteOutput()
> 
> Attached is a graph of a list of caller methods that call the
> ns3::Ipv4RoutingProtocol::RouteOutput() method. I purpose to change the
> signature of ns3::Ipv4RoutingProtocol::RouteOutput() to
> 
> RouteOutput(const Ipv4Header &header, uint32_t oif, Socket::SocketErrno
> &sockerr, Ptr<const Packet> p = 0)
> 
> and modify all its callers accordingly.
> 
> The Ptr<const Packet> is added to the back as I would like the new argument to
> be an optional argument, and thus keep being backward compatible. 
> 
> Any comments? And, btw, does backward compatibility a requirement? 
> 
> Either Ptr<const Packet> or Ptr<Packet> is equally perfect for me. ^^
> 

Sorry Wilson, I didn't mean to completely disregard your suggestions here.  I was actually just finished with what I was doing when I read your comments.

Disregard part in comment #6 about failing a regression test.  I forgot to update my traces.  All pass.

(In reply to comment #4)
> The Ptr<const Packet> is added to the back as I would like the new argument to
> be an optional argument, and thus keep being backward compatible. 
> 
> Any comments? And, btw, does backward compatibility a requirement? 


Normally, yes, backward compatibility would be a requirement or goal, but the whole Ipv4RoutingProtocol API is new in this release cycle so I think we should just fix it without such a constraint before the ns-3.5 release.

Josh~ please go ahead. no worries~ I don't do anything on my suggestion. So I gain a patch from you and lose nothing~ ^^

(In reply to comment #7)
> (In reply to comment #4)
> > Created an attachment (id=476) [details] [details]
> > caller graph of RouteOutput()
> > 
> > Attached is a graph of a list of caller methods that call the
> > ns3::Ipv4RoutingProtocol::RouteOutput() method. I purpose to change the
> > signature of ns3::Ipv4RoutingProtocol::RouteOutput() to
> > 
> > RouteOutput(const Ipv4Header &header, uint32_t oif, Socket::SocketErrno
> > &sockerr, Ptr<const Packet> p = 0)
> > 
> > and modify all its callers accordingly.
> > 
> > The Ptr<const Packet> is added to the back as I would like the new argument to
> > be an optional argument, and thus keep being backward compatible. 
> > 
> > Any comments? And, btw, does backward compatibility a requirement? 
> > 
> > Either Ptr<const Packet> or Ptr<Packet> is equally perfect for me. ^^
> > 
> 
> Sorry Wilson, I didn't mean to completely disregard your suggestions here.  I
> was actually just finished with what I was doing when I read your comments.
> 

understood~ thanks~ ^^

(In reply to comment #9)
> (In reply to comment #4)
> > The Ptr<const Packet> is added to the back as I would like the new argument to
> > be an optional argument, and thus keep being backward compatible. 
> > 
> > Any comments? And, btw, does backward compatibility a requirement? 
> 
> 
> Normally, yes, backward compatibility would be a requirement or goal, but the
> whole Ipv4RoutingProtocol API is new in this release cycle so I think we should
> just fix it without such a constraint before the ns-3.5 release.
> 

Comment on attachment 478 [details]
Patch for bug 592


>diff -r 8c0ff401237e src/internet-stack/tcp-socket-impl.cc
>--- a/src/internet-stack/tcp-socket-impl.cc	Sun Jun 21 23:38:40 2009 -0700
>+++ b/src/internet-stack/tcp-socket-impl.cc	Mon Jun 22 11:13:38 2009 -0400
>@@ -354,7 +354,8 @@
>       Ptr<Ipv4Route> route;
>       uint32_t oif = 0; //specify non-zero if bound to a source address
>       // XXX here, cache the route in the endpoint?
>-      route = ipv4->GetRoutingProtocol ()->RouteOutput (header, oif, errno_);
>+      Ptr<Packet> packet = 0;
>+      route = ipv4->GetRoutingProtocol ()->RouteOutput (packet, header, oif, errno_);
>       if (route != 0)
>         {
>           NS_LOG_LOGIC ("Route exists");
>@@ -855,7 +856,7 @@
>             Ptr<Ipv4Route> route;
>             uint32_t oif = 0; //specify non-zero if bound to a source address
>             header.SetDestination (m_remoteAddress);
>-            route = ipv4->GetRoutingProtocol ()->RouteOutput (header, oif, errno_);
>+            route = ipv4->GetRoutingProtocol ()->RouteOutput (p, header, oif, errno_);
>             if (route != 0)
>               {

I agree with most of this patch but the change immediately above is probably not correct since "p" in this case is an input packet and is not being outputted in this branch of the code.  Would instead put a Ptr<Packet> () here.

Created attachment 485 [details]
patch with bindings and correction

This is basically Josh's patch with the fix mentioned in the comments, plus bindings update, plus a bit more doxygen about this parameter.

changeset 98cb594222af