Bug 659

Summary:	Static routing with support for metrics and longest-prefix-match-first support
Product:	ns-3	Reporter:	Antti Mäkelä <antti.makela>
Component:	routing	Assignee:	ns-bugs <ns-bugs>
Status:	RESOLVED FIXED
Severity:	normal	CC:	tomh
Priority:	P5
Version:	pre-release
Hardware:	All
OS:	All
Bug Depends on:	634
Bug Blocks:
Attachments:	Patch to improve static routing Patch to improve static routing Reworked default route handling One-liner fix, actually failed the longest-match test case before Fix for notifyInterfaceDown New version aligned with GetPrefixLength()

Description Antti Mäkelä 2009-08-11 06:22:49 UTC

Created attachment 547 [details]
Patch to improve static routing

This patch uses the GetLength patch from bug #634, http://www.nsnam.org/bugzilla/﷒0﷓

I've started to work on better Ipv4StaticRouting support that would allow me to have (most importantly) multiple routes to same destination, and also defining the "destination" as prefix and mask. 

Attached patch now includes support for metrics (with default parameter =0), and when performing route lookup (LookupStatic), first matches for longest prefix and then based on metrics. If metrics are equal, latest addition is picked.

"Default route" and "Host Routes" are no longer considered anything special: Default is simply 0.0.0.0/0 and Host Route is simply a /32. A small change to Ipv4-routing-table-entry now has AddHostRouteTo and IsHost using Ipv4Mask::GetOnes() instead of GetZero - as the route IS to a /32, that's how it should be anyway.

API is backwards-compatible. The default metric is zero, and added as last parameter to any function calls.

NotifyInterfaceDown (without any changes) should take care of deleting routes appropriately when interface goes down. This *should* take care of e.g. automatically switching to secondary default route. Up to user to reconfigure any routes when interface comes back up.

Comments and testing appreciated. For example, I haven't tested multicast.

Comment 1 Antti Mäkelä 2009-08-11 07:52:28 UTC

Created attachment 548 [details]
Patch to improve static routing

Slightly better logging for lookups.

Comment 2 Antti Mäkelä 2009-08-11 08:48:21 UTC

Created attachment 549 [details]
Reworked default route handling

Reworked default route handling to completely do away old structures while maintaining backwards compatibility (old system broke down at IfDown).

Comment 3 Antti Mäkelä 2009-08-11 08:55:38 UTC

Created attachment 550 [details]
One-liner fix, actually failed the longest-match test case before

Comment 4 Tom Henderson 2009-08-12 00:40:05 UTC

(In reply to comment #3)
> Created an attachment (id=550) [details]
> One-liner fix, actually failed the longest-match test case before
> 

+1.  If you already wrote a set of test cases for this new code, can we add them as a unit test to this model?  (such as populating the routing table with a number of routes,  and then querying for a route)

Comment 5 Antti Mäkelä 2009-08-12 04:09:22 UTC

(In reply to comment #4)
> +1.  If you already wrote a set of test cases for this new code, can we add
> them as a unit test to this model?  (such as populating the routing table with
> a number of routes,  and then querying for a route)

  I don't have formal test cases limited only to testing this - I'm testing this right now in the scope of my own work. 

  I guess I could cook up something based on examples/static-routing-slash32.cc though.

Comment 6 Antti Mäkelä 2009-08-17 08:45:33 UTC

Created attachment 562 [details]
Fix for notifyInterfaceDown

Actually fixing bug in *original* NotifyInterfaceDown. The problem is that indexing changes when you RemoveRoute(), thus, if a route is getting removed you should NOT increase the index. Reworked for into a while loop instead.

Comment 7 Antti Mäkelä 2009-08-30 07:23:18 UTC

One place where backwards compatibility right now is broken - it used to be that if you have a default route set, doing a GetRoute(0) would always return the default first. Wondering if such a quirk should be included or not.

Comment 8 Tom Henderson 2009-08-30 17:58:26 UTC

(In reply to comment #7)
> One place where backwards compatibility right now is broken - it used to be
> that if you have a default route set, doing a GetRoute(0) would always return
> the default first. Wondering if such a quirk should be included or not.
> 

I'm not in favor of retaining the quirk.  It duplicates GetDefaultRoute () and it seems to prevent a user from obtaining the first route stored in m_hostRoutes.

Comment 9 Tom Henderson 2009-08-31 02:11:07 UTC

I reviewed this again tonight and plan to push it in the next day or so (along with the patch in bug 614) if there are no other comments.

Comment 10 Antti Mäkelä 2009-08-31 03:18:05 UTC

Created attachment 580 [details]
New version aligned with GetPrefixLength()

Uses GetPrefixLength instead of GetMaskLength, as per changes in other bug.

Also, fix for GetDefaultRoute, now it returns empty Ipv4RoutingTableEntry instead of pure zero when there isn't a default route (just like in the old version).

Comment 11 Tom Henderson 2009-09-01 02:39:34 UTC

added as changeset 3dc675bb8b20