Created attachment 701 [details]
Use oif in RouteOutput

I think that I understand what should be done here.  Please let me know if I am wrong.  Essentially, if oif parameter is not null, then this net-device must be used as the output device.  I think I have accomplished this with the attached patch.

(In reply to comment #1)
> Created an attachment (id=701) [details]
> Use oif in RouteOutput
> 
> I think that I understand what should be done here.  Please let me know if I am
> wrong.  Essentially, if oif parameter is not null, then this net-device must be
> used as the output device.  I think I have accomplished this with the attached
> patch.

Sorry, I did not see your patch until tonight (the cc list is fixed now).

I am not sure your patch is totally correct because I would assume that you want to build a nix vector with the outbound device as a constraining argument to the below method:

       GetNixVector (m_node, header.GetDestination ());

but it seems that you are not constraining the construction of nix vectors with your patch but just modifying the resulting rtentry to force an output device, regardless of what the code would naturally choose.  Do I understand your patch correctly?

Created attachment 710 [details]
Use of oif in RouteOutput

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=701) [details] [details]
> > Use oif in RouteOutput
> > 
> > I think that I understand what should be done here.  Please let me know if I am
> > wrong.  Essentially, if oif parameter is not null, then this net-device must be
> > used as the output device.  I think I have accomplished this with the attached
> > patch.
> 
> Sorry, I did not see your patch until tonight (the cc list is fixed now).
> 
> I am not sure your patch is totally correct because I would assume that you
> want to build a nix vector with the outbound device as a constraining argument
> to the below method:
> 
>        GetNixVector (m_node, header.GetDestination ());
> 
> but it seems that you are not constraining the construction of nix vectors with
> your patch but just modifying the resulting rtentry to force an output device,
> regardless of what the code would naturally choose.  Do I understand your patch
> correctly?

You are right. I should have been creating nix-vectors differently if the output device is specified. Now GetNixVector take in oif, which eventually passes it down to the breadth-first search. The BFS checks to see if oif is specified, and if so, on the first hop it forces this direction. Then it proceeds as normal.(In reply to comment #3)
> Created an attachment (id=710) [details]
> Use of oif in RouteOutput

You are right. I should have been creating nix-vectors differently if the output device is specified. Now GetNixVector take in oif, which eventually passes it down to the breadth-first search. The BFS checks to see if oif is specified, and if so, on the first hop it forces this direction. Then it proceeds as normal.

> 
> You are right. I should have been creating nix-vectors differently if the
> output device is specified. Now GetNixVector take in oif, which eventually
> passes it down to the breadth-first search. The BFS checks to see if oif is
> specified, and if so, on the first hop it forces this direction. Then it
> proceeds as normal.

My one remaining comment is:

+      // Search here in a cache for this node index 
+      // and look for a Ipv4Route, if output device, 
+      // oif, is not specified
+      if (!oif)
+        {
+          rtentry = GetIpv4RouteInCache (header.GetDestination ());
+        }
+      // not in cache or a specified output
+      // device is to be used

it seems like presence of oif will mean that the route is never cached.  Would it be better to still cache it and just check whether the oif is valid?

rtentry = GetIpv4RouteInCache()
if (oif && rtentry.oif == oif)
 {
   // use cache entry
 }
else
 {
   // flush cache entry, and continue (fall through to the step that does a new BFS)
 }

I think that the above pseudocode would avoid doing the BFS for each packet.  It would still not catch the case where there was a cache entry built with a previous value of oif, user then sets oif to zero, route output is called, and the cache entry is used even though there is a better route that could be found if oif was not previously constrained.  Some kind of check at the top of RouteOutput such as 

if (m_savedOif != oif)
  {
    // flush cache
  }

would probably do the trick.

> My one remaining comment is:
> 
> +      // Search here in a cache for this node index 
> +      // and look for a Ipv4Route, if output device, 
> +      // oif, is not specified
> +      if (!oif)
> +        {
> +          rtentry = GetIpv4RouteInCache (header.GetDestination ());
> +        }
> +      // not in cache or a specified output
> +      // device is to be used
> 
> it seems like presence of oif will mean that the route is never cached.  Would
> it be better to still cache it and just check whether the oif is valid?
> 
> rtentry = GetIpv4RouteInCache()
> if (oif && rtentry.oif == oif)
>  {
>    // use cache entry
>  }
> else
>  {
>    // flush cache entry, and continue (fall through to the step that does a new
> BFS)
>  }
> 
> I think that the above pseudocode would avoid doing the BFS for each packet. 
> It would still not catch the case where there was a cache entry built with a
> previous value of oif, user then sets oif to zero, route output is called, and
> the cache entry is used even though there is a better route that could be found
> if oif was not previously constrained.  Some kind of check at the top of
> RouteOutput such as 
> 
> if (m_savedOif != oif)
>   {
>     // flush cache
>   }
> 
> would probably do the trick.

Right again.  I'm confused about this part though:

> It would still not catch the case where there was a cache entry built with a
> previous value of oif, user then sets oif to zero, route output is called, and
> the cache entry is used even though there is a better route that could be found

Given your psuedo-code, if oif was set at first and then reset later, the cached route wouldn't be used.  It would fall through to the else statement, erase the value in the map, proceed with the BFS, and cache the new route.  Did I understand this incorrectly?  Here is what I was thinking of doing:

 rtentry = GetIpv4RouteInCache()
 if (rtentry && rtentry.oif == oif)
  {
    // we are cached, and the oif is the same,
    // use it
  }
 else
  {
    // need a new route
  }

> Given your psuedo-code, if oif was set at first and then reset later, the
> cached route wouldn't be used.  It would fall through to the else statement,
> erase the value in the map, proceed with the BFS, and cache the new route.  Did
> I understand this incorrectly?  Here is what I was thinking of doing:
> 
>  rtentry = GetIpv4RouteInCache()
>  if (rtentry && rtentry.oif == oif)
>   {
>     // we are cached, and the oif is the same,
>     // use it
>   }
>  else
>   {
>     // need a new route
>   }

agree.

Created attachment 713 [details]
Use of oif in RouteOutput

changeset 408970e134cb

(In reply to comment #8)
> Created an attachment (id=713) [details]
> Use of oif in RouteOutput
> 
> changeset 408970e134cb

Above changeset was reverted due to code freeze.  Bug is now fixed in changeset 681058254595