patchlwIP - A Lightweight TCP/IP stack - Patches: patch #8680, Add support for LwIP static route...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #8680: Add support for LwIP static route table

Submitted by:  Pradip De <pradipde>
Submitted on:  Tue 02 Jun 2015 10:49:06 PM UTC  
 
Category:  IPv6 Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Mon 30 Jan 2017 07:09:43 PM UTC, comment #40:

Ok. I will defer to adding these helpers to the static routing code then.
Thanks everyone.

Pradip De <pradipde77>
Thu 26 Jan 2017 02:38:26 PM UTC, comment #39:

Task #14283 is for more detailed callbacks. Those won't give you a gateway address.

Since these additional functions would only be useful for static routing code, I feel they are better off there. Perhaps in a 2nd file or something?

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 25 Jan 2017 09:57:57 PM UTC, comment #38:

In theory they could have been. Although, since those APIs are overloaded forms of some of the existing netif APIs, I felt it natural to include them in the same place. With this, they could be used with other static routing implementations as well.
Task 14283 could also tackle this provided it captured a way to pass in a gateway parameter.

Pradip De <pradipde77>
Wed 25 Jan 2017 09:43:13 PM UTC, comment #37:

> Thoughts?


Is there any reason that such convenience APIs, which essentially wrap the existing netif calls with extra parameters for static routing only, couldn't be added to the static route implementation itself?

There's also task #14283 though..

David van Moolenbroek <dcvmoole>
Wed 25 Jan 2017 09:32:01 PM UTC, comment #36:

The hooks that I was proposing earlier were about adding/deleting prefix routes in the routing table when an address is added/deleted to a netif. Currently, the core code does not have a place that these hooks can be placed in(because they need additional parameters supplied, e.g., gateway address) so I was thinking of adding 2 APIs in netif.c that could have these hooks embedded.
Again, these are convenience APIs and I was planning on encapsulating them within the hook ifdef so that they would be compiled out if the hook is not defined. Otherwise, the application is burdened with adding/deleting these routes whenever it adds/deletes the address to the netif. Thoughts?

Pradip De <pradipde77>
Wed 25 Jan 2017 08:59:21 PM UTC, comment #35:

It did, now only one left :-)

BTW: Unix thing solved, -Wconversion is missing from travis... We'll have to work on that... some day...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 25 Jan 2017 08:56:06 PM UTC, comment #34:

> Which hooks are missing for you two?


Assuming that includes me, my concerns are resolved here! :)

David van Moolenbroek <dcvmoole>
Wed 25 Jan 2017 08:32:50 PM UTC, comment #33:

It is included in the unix port (it's in Filelists.mk)

Dirk Ziegelmeier <dziegel>
Project Administrator
Wed 25 Jan 2017 08:22:37 PM UTC, comment #32:

> Pushed Pradip's patch to contrib.


Can we add this file to the unix build so that yarrick's travis compiles it? I get build errors when compiling with VS2010...

Coming back on this (and I have to apologize I had too much different things on my mind lately), what's open here now that Dirk pushed the files? Which hooks are missing for you two?

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 15 Jan 2017 04:10:41 PM UTC, comment #31:

Pushed Pradip's patch to contrib. I'll leave this patch open, maybe you want to go on discussing the hooks?

Dirk Ziegelmeier <dziegel>
Project Administrator
Wed 11 Jan 2017 09:26:02 PM UTC, comment #30:

Attached a cleaned-up version of the patch (constness, unused args).

(file #39442)

Dirk Ziegelmeier <dziegel>
Project Administrator
Tue 10 Jan 2017 01:24:54 AM UTC, comment #29:

Folks,
Any update on getting this route table implementation into LwIP contrib?

Pradip De <pradipde77>
Thu 15 Dec 2016 09:20:13 PM UTC, comment #28:

I have attached a README file StaticRouteTable.README for the route table implementation in lwip_rt_table_impl_for_contrib.diff.

(file #39243)

Pradip De <pradipde77>
Thu 15 Dec 2016 07:14:44 PM UTC, comment #27:

By now I guess the additional functions to netif.c might not be generic enough to justify adding them.

Adding and removing (without specific prefix length) would be covered by the standard netif callbacks, so the routing algorithm files for contrib + a short description of how to use them (hooks, callbacks) should be good.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 15 Dec 2016 04:25:59 PM UTC, comment #26:

Alright, so I may have misunderstood the intention of your hooks. My assumption was that you do not control the netif calls to add addresses, so you needed hooks to be able to set routes at all. Now I understand that you do control the netif calls (otherwise, where is the gateway address coming from?) but prefer to have hooks for the route changes instead of having to make those route changes explicitly from your own code, in order to allow easier drop-in of the static-route implementation. If that is the case, then generalizing the hooks would indeed not make sense for you, and I have nothing meaningful to add to this discussion. :)

David van Moolenbroek <dcvmoole>
Wed 14 Dec 2016 11:49:35 PM UTC, comment #25:

I understand, but I presume there is a limit to which we can generalize a hook. This limitation manifests itself in terms of the parameters that that hook is defined with.
For example, it is hard to think of a hook meant for adding a netif address to also take a prefix and gateway parameter just so that it covers the functionality of adding a route entry to a routing table. It is probably true for the MLD case for joining a group as well.
So, it may be ok to define these hooks for the specific purpose they are meant to serve as long as it captures the API requirements fully.

I have attached the routing table implementation(lwip_rt_table_impl_for_contrib.diff) with the #define LWIP_IPV6_NUM_ROUTE_ENTRIES moved from opt.h to the ip6_route_table.h.

(file #39229)

Pradip De <pradipde77>
Wed 14 Dec 2016 09:34:28 PM UTC, comment #24:

Hmm.. wouldn't it then make more sense to make the hooks for local address addition and deletion (ie the events happening), rather than specifically for adding and deleting static routes (ie one use case for those hooks)?

For instance, yesterday I found out that when LWIP_IPV6_MLD is enabled, each address addition also has to be accompanied by a join-group on the corresponding solicited-node multicast group. For me it is relatively easy to implement such things because I use my own wrapping code around the netif calls anyway, but if I understand you correctly, it sounds like this could be another application of such hooks in your case?

David van Moolenbroek <dcvmoole>
Wed 14 Dec 2016 09:18:57 PM UTC, comment #23:

I'm not opposed to more hooks as long as they are cleanly separated and described. Having the netif_* functions might help to keep applications independent of the routing algorithm after all...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 09:16:41 PM UTC, comment #22:

Thanks for the discussion and input and thanks David for the patches.

One of the things with using a routing table is adding/removing a corresponding route(typically a /64) along with an address addition/removal. This is why I had included the netif_[add/remove]_address_with route() functions in LwIP core.

However, if the responsibility to add and remove these routes can be delegated to the application then I think my route table implementation (in lwip_rt_table_impl.diff) can be used by applications that require simple route addition/deletion and lookup functionality.
There is a small pending correction of moving the definition of LWIP_IPV6_NUM_ROUTE_ENTRIES from opt.h but I would be happy to do that.

Alternatively, if we choose to allow the LwIP core to help with this when adding addresses(instead of having the application handle that) then we could potentially surface 2 more hooks in opt.h for addition/removal of a static route: something to the tune of the following:

/**

  • LWIP_HOOK_IP6_ADD_STATIC_ROUTE(prefix, netif, gateway, idx):
  • - called from ip6_route() (IPv6)
  • - prefix: An IPv6 prefix (address and prefix length)
  • - netif: Target netif to point the route to.
  • - gateway: Optional Gateway IP6 address, or NULL if unused.
  • - idx: Optional returned index of added route entry

*/
#ifdef _DOXYGEN_
#define LWIP_HOOK_IP6_ADD_STATIC_ROUTE(prefix, netif, gateway, idx)
#endif

#ifdef _DOXYGEN_
#define LWIP_HOOK_IP6_REMOVE_STATIC_ROUTE(prefix)
#endif

Then we can add a couple of helper functions which are encapsulated with #ifdefs of the above two hooks.

Pradip De <pradipde77>
Wed 14 Dec 2016 08:26:57 PM UTC, comment #21:

> I'm afraid you forgot to check in nd6_priv.h though.. :)


D'oh! There it is now... thanks for the hint ;-)

> forgotten "static"


Pushed.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 08:23:57 PM UTC, comment #20:

Awesome! The new name is indeed better, long is better than vague.

I'm afraid you forgot to check in nd6_priv.h though.. :)

I also spotted a mistake of my own (a forgotten "static"); patch attached for that.

(file #39226)

David van Moolenbroek <dcvmoole>
Wed 14 Dec 2016 08:09:03 PM UTC, comment #19:

Thanks again for all the patches. I've applied them, only change the name of the new function (last patch) - it's not much better, but maybe a little more self explaining...?

After this being rather off-topic, I'm still open to add a (good quality) version of a routing table to contrib, given it is based on the available hooks (unless we need to add yet something more) and really separated from the core.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 04:36:43 PM UTC, comment #18:

Oh, just to make sure: the actual patches were already attached to my last post. :)

As for the software engineering stuff--yes, definitely. Even though I have no direct stake in this, I'm also attaching an attempt at removing the other cases where internal nd6 data structures are accessed externally. This is the attached 0003 patch. The result is not as clean as I hoped because it ends up with as somewhat poorly named function that may return three possible results. On the other hand, it also effectively solves two bugs in the lowpan6 copy of the code, which were already fixed in the original ethip6 version (one of these bugs I missed in an earlier patch here). So perhaps this patch can at least offer a starting point.

Important note however: I have not been able to test even so much as compilation of the lowpan6 changes in this patch!

(admittedly this is getting a bit off-topic now)

(file #39223)

David van Moolenbroek <dcvmoole>
Wed 14 Dec 2016 02:58:08 PM UTC, comment #17:

These changes are of course welcome, and as long as they don't add code for those of us not needing it, I'm fine with letting the linker throw unused code :-) so no #ifdefs should be needed.

As to the "general software engineering perspective" - there are still some other issues left in the v6 code in that direction where function calls could be favoured over exporting variables and types...

I'm always appreciating your (and anyone's) help on IPv6 :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 02:33:55 PM UTC, comment #16:

Alright, thank you!!

Now, I really hate to do a "now that you've merged X, here's Y and Z" kind of thing, and so I apologize for this, but I'm just figuring things out myself here.. :) and as such I would like to suggest the following additional patches, all in the context of static routing. These would be helpful to me, and hopefully to others, but they're definitely not crucial. As always, I'd be happy to make additional changes to the patches if that helps.

0001-nd6-add-nd6_clear_destination_cache-function:

Given that the new hook sits behind the destination cache, any change to what the hook would return should also cause the destination cache to be invalidated. For example, after adding or removing a static route, the destination cache may contain stale entries. So, any static-route code would want to clear it, and preferably without having to mess with the nd6 data structures directly. So, this patch adds a function to clear the nd6 destination cache. Note that I do not see a good way (or a pressing reason) to make it invalidate only a subset of the cache.

I would be happy to put this new function behind a specific #if (suggestions welcome!) but as noted in the patch, strictly speaking, the destination cache may also have to be cleared in other cases, such as when adding or removing local IPv6 addresses to netifs. So it just might be useful for a larger group of users.

In any case, I'm afraid this might only be scratching the surface when it comes to nd6 cache invalidation. For example, I expected code that would clear the destination cache whenever it is decided that a particular router is no longer usable, but I can find no such thing..

0002-nd6-use-default_router_list-internally-only:

This patch just moves code around a bit. Specifically, the route code in ip6.c currently uses the nd6 "default_router_list" data structure directly. From a general software engineering perspective it would be cleaner to move that part into nd6, as also suggested in a todo item in nd6.h [*].

The gain for me here is that I can then use this new nd6_find_route() function directly from my routing hook, so that I can fully control the routing decisions myself (and, for instance, know when to generate RTM_MISS routing socket messages--stuff like that) without relying on nd6 data structure internals.

As an aside, nd6_select_router() keeps a "last_router" variable but doesn't actually use it, what's up with that?

[*] I would actually be against making those data structures static though, but for another reason: I'd like to be able to support an ndp(8) utility that can dump ND information. But that is a lot less important to me overall. Also, I haven't actually gotten that far yet.. ugh, so much to do!

(file #39221, file #39222)

David van Moolenbroek <dcvmoole>
Wed 14 Dec 2016 08:19:23 AM UTC, comment #15:

I've reverted yesterday's hook and chosen the nd6 approach instead, which works for other link layers, too (like 6lowpan).

I'm not sure we need the additional functions in netif.c. For adding an address, the application has to call into routing manually. For removing an address, we'd rather need a better callback mechanism (than the current NETIF_STATUS_CALLBACK()) to get the routing table informed of address removal.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 02:10:13 AM UTC, comment #14:

Unfortunately, we are behind on updating to the latest LwIP master and I introduced some bugs in the patch while trying to align that to LwIP master. Apologies. I did not get a chance to test the patch to LwIP core.

1. netif_matches_ip6_addr(..) should be netif_get_ip6_addr_match(..)
2. The nd6 changes are not needed. Previously, I was adding the gateway to
the default router list and needed to create public functions in nd6.c
towards that.
3. The netif_add/remove_address_with_route functions were functions to add addresses to netifs and I thought that they should be present in netif.c.

I will fix these and upload a corrected patch soon.

Pradip De <pradipde77>
Tue 13 Dec 2016 09:12:53 PM UTC, comment #13:

Ah hehe. Cool, thanks. Generally speaking, as long as there is a hook at all I'm good.

David van Moolenbroek <dcvmoole>
Tue 13 Dec 2016 09:07:12 PM UTC, comment #12:

Oops, I did not see your post. I'll have a look at your files tomorrow, I'm not fixed to Pradip's version of the hook.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 13 Dec 2016 09:06:37 PM UTC, comment #11:

Uh, okay. You're moving fast. The new hook will do I suppose, although my other patch here (the 0001 one) is still a relevant bugfix..

David van Moolenbroek <dcvmoole>
Tue 13 Dec 2016 08:58:42 PM UTC, comment #10:

I've pushed LWIP_HOOK_ETHIP6_GET_GW (slightly adapted) and fixed netif_add_ip6_address().

David, which functions or hooks would you need to implement your code?

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 13 Dec 2016 08:56:55 PM UTC, comment #9:

FWIW! Here is what I came up with today. I've tested this version of the hook for two of the three purposes I had in mind (routing with gateway in case of router that does not advertise itself properly, routing with local address in case of multiple assigned ranges on the local network, and -yet untested- selection of a preferred router in the presence of multiple). Compared to Pradip's patch, I believe my version is closer to the IPv4 hook in that it checks local networks first. Also, it uses the destination cache the same way as the regular (ND-router based routing) case. But I for one would be just fine with Pradip's version of the hook (with the bug fixed) too, although I have no use for any of the rest either.

(file #39217, file #39218)

David van Moolenbroek <dcvmoole>
Tue 13 Dec 2016 08:39:05 PM UTC, comment #8:

I'm sorry but I can't apply this:
- I'm a bit confused, where have the nd6 changes gone, are they not needed any more?
- the routing code should be an addon, you can't put includes to it in core files!
- the LWIP_HOOK_ETHIP6_GET_GW is still not separated from the routing implementation
- the netif_add/remove_address_with_route are comfort functions that reference routing algorithm functions -> can't make it into the core when the actual routing implementation is not (and I don't think these are necessary here, you can put them into the routing implementation)
- netif_matches_ip6_addr() is unknown but referenced

I'll just start to add some things I'm Ok with...

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 13 Dec 2016 07:00:08 PM UTC, comment #7:

I have uploaded a couple of diff files containing updated route table implementation code.
1. lwip_rt_table_impl.diff : This is the static route table implementation.
2. lwip_core_changes_rt_table.diff : This is an associated patch to LwIP core
containing changes to add addresses to netifs and implant associated
routes in the static route table. The changes in ethip6.c uses a new hook to
fetch the gateway. The route table implementation in 1) contains functions
that can be assigned to hooks(e.g., route lookup, get gateway).

Pradip De <pradipde77>
Mon 12 Dec 2016 04:25:26 PM UTC, comment #6:

I agree, maybe put it to contrib if it makes sense.

Ivan Delamer <idelamer>
Project Member
Mon 12 Dec 2016 09:30:35 AM UTC, comment #5:

Thanks for the input :-) If it's not part of the core code, using the hooks is definitively a good idea.

In that case (also for RPL), some hooks seem to be missing. This patch adds them, so we should extract them to get the hooks to a usable state.

Other than that, I don't want this patch tracker to be a platform for exchanging patches in the long term (or is it meant for that?), so I'll close this one when we're done. We'd need a different place to put this. Possibly in contrib?

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 11 Dec 2016 05:24:36 PM UTC, comment #4:

I believe at that time I suggested using the hooks as well.
For example, in 6LowPAN, we have the implementation of RPL routing pending, and it would be a great use case.

This patch was a nice effort, but it's application scope is too narrow in my opinion to be merged to the core code.

Cheers

Ivan Delamer <idelamer>
Project Member
Fri 09 Dec 2016 12:53:42 PM UTC, comment #3:

A patch would be very welcome! I also don't like the idea of this patch hijacking the hooks meant for external addons. I didn't mean to apply the patch as is...

Anyway, as you said, what would be ideal is if we could get lwIP in a state to allow just dropping in this implementation (or anyone else's). Adding this implementation to our sources is then a different issue. Since I'm still not using IPv6 in products, I still can't tell if this routing table is something needed often or not...

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 09 Dec 2016 12:40:18 PM UTC, comment #2:

I for one think it would make more sense to provide the hooks and facilities to implement the actual routing outside of the lwIP core. Most importantly, that would match the current model for IPv4 as well. In addition, it would allow for a different implementation of the routing table.

Case in point: I am currently working on my own routing implementation, with the expectation (beyond my control) of a whole lot more routing entries, and therefore with radix tree backing. As such, I did notice that there is no ETHIP_GET_GW hook to match the ETHARP_GET_GW hook for IPv4, and that was actually next on my list. However, this patch adds that hook only when the patch's static routing is also enabled, which would clash with my own implementation.

So, I for one would rather see only the ETHIP6_GET_GW hook and ND6 helper functions merged, while leaving out the actual static routing table code and netif changes. I believe that the author of this patch would then be able to maintain the latter on top of lwIP without any problems, hopefully making everyone happy.. :)

If a consensus can be reached here, I'd be more than willing to provide an alternative patch.

If merging of this patch is still considered, I have to point out that that there are several issues with the netif changes in this patch: incorrect error handling upon address addition (marking the new address as TENTATIVE even on failure), and issues with removal of routes that do not exist (for example for the link-local address in slot #0). However, the netif !isvalid -> isinvalid change should be merged in any case, and really should have been a separate patch..

David van Moolenbroek <dcvmoole>
Fri 09 Dec 2016 11:51:20 AM UTC, comment #1:

Does anyone really need/use this? If yes, I'd be OK to add it by now... :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 02 Jun 2015 10:49:06 PM UTC, original submission:

Static route table management in LwIP.

APIs are:
1) s8_t ip6_add_route_entry (struct ip6_prefix *ip6_prefix,
struct netif *netif,
ip6_addr_t *gateway);

2) err_t ip6_remove_route_entry (struct ip6_prefix *ip6_prefix);

3) s8_t ip6_find_route_entry (ip6_addr_t *ip6_dest_addr);

4) struct netif ip6_static_route(ip6_addr_t src, ip6_addr_t *dest);

5)ip6_addr_t ip6_get_gateway(struct netif netif, ip6_addr_t *dest);

-When adding a route entry, if a gateway is passed, then it would be
added to the default router list with the neighbor cache netif entry
pointing to the netif passed.

-With this, a prefix match up to a specific prefix byte
boundary would be made possible while doing a longest prefix match
upon route lookup.

-APIs 4 and 5 would be used for assigning to LWIP_HOOK_IP6_ROUTE and
LWIP_HOOK_ETHIP6_GET_GW hooks.

Pradip De <pradipde>

 

Attached Files
file #39442:  ipv6_static_routing.zip added by dziegel (6KiB - application/zip)
file #39243:  StaticRouteTable.README added by pradipde77 (2KiB - application/octet-stream - README file for Static Route Table implementation in lwip_rt_table_impl_for_contrib.diff)
file #39229:  lwip_rt_table_impl_for_contrib.diff added by pradipde77 (12KiB - application/octet-stream - Static routing table implementation.)
file #39213:  lwip_rt_table_impl.diff added by pradipde77 (12KiB - application/octet-stream - Updated patches: 1. lwip_rt_table_impl: containing the route table implementation 2. lwip_core_changes_rt_table: containing associated changes to LwIP core)
file #39214:  lwip_core_changes_rt_table.diff added by pradipde77 (8KiB - application/octet-stream - Updated patches: 1. lwip_rt_table_impl: containing the route table implementation 2. lwip_core_changes_rt_table: containing associated changes to LwIP core)
file #34229:  LwIP-Static-Routing2.diff added by pde (20KiB - application/octet-stream - Fresh patch for route table management in LwIP. )
file #34151:  LwIP-Static-Routing.diff added by pradipde (17KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by pradipde77 (Updated the item)
  • -email is unavailable- added by idelamer (Posted a comment)
  • -email is unavailable- added by dcvmoole (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by pde (Updated the item)
  • -email is unavailable- added by pradipde (Submitted the item)
  • -email is unavailable- added by pradipde
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-26 goldsimon StatusNone => Done
        Open/ClosedOpen => Closed
    2017-01-11 dziegel Attached File- => Added ipv6_static_routing.zip, #39442
    2016-12-15 pradipde77 Attached File- => Added StaticRouteTable.README, #39243
    2016-12-14 pradipde77 Attached File- => Added lwip_rt_table_impl_for_contrib.diff, #39229
    2016-12-14 dcvmoole Attached File- => Added 0001-nd6-minor-static-consistency-fix.patch, #39226
    2016-12-14 dcvmoole Attached File- => Added 0003-nd6-centralize-link-local-packet-send-decision.patch, #39223
    2016-12-14 dcvmoole Attached File- => Added 0001-nd6-add-nd6_clear_destination_cache-function.patch, #39221
        Attached File- => Added 0002-nd6-use-default_router_list-internally-only.patch, #39222
    2016-12-13 dcvmoole Attached File- => Added 0001-ethip6-forward-correct-error-code.patch, #39217
        Attached File- => Added 0002-nd6-add-LWIP_HOOK_ND6_GET_GW-hook.patch, #39218
    2016-12-13 pradipde77 Attached File- => Added lwip_rt_table_impl.diff, #39213
        Attached File- => Added lwip_core_changes_rt_table.diff, #39214
    2016-12-12 goldsimon StatusNeed Info => None
    2016-12-09 goldsimon StatusNone => Need Info
    2015-06-16 pde Attached File- => Added LwIP-Static-Routing2.diff, #34229
    2015-06-02 pradipde Attached File- => Added LwIP-Static-Routing.diff, #34151
        Carbon-Copy- => Added -email is unavailable-

    Back to the top


    Powered by Savane 3.3