GNOME Bugzilla – Bug 730934
[review] th/bgo730934_platform_route_lookup
Last modified: 2014-05-30 18:23:55 UTC
Please review branch th/bgo*_platform_route_lookup. Quoting the commit message: platform: fix lookup of routes and deletion of IPv4 routes When doing a lookup for an libnl route, the cache comparison function for routes takes into account 'family', 'tos', 'table', 'dst', and 'prio'. In NetworkManager we don't use all of these properties for a route, so at several places when doing a cache lookup we don't have all identifying properties. Usually we only have 'family' and 'dst' ('table' is implicit 0, because NM does currently not care about any other tables). The problem is that NM sees routes with different 'tos', 'prio', but it cannot look them up in the cache. Add a hack to search the cache fuzzy. This is similar to the hack for link, where the identifying properties are 'family' and 'ifindex', but we only have 'ifindex' at hand. However, contrary to this hack, we coerce the 'family' to AF_UNSPEC for every link cache operation. This is not viable in this case, because we internally need the 'tos' field. We need the 'tos' field because when deleting an IPv4 route, the 'tos' field must match. See fib_table_delete(). This was already partially fixed by commit f0daf90298d1bd9cafac7b9c02dc905327e0b85a, but before the lookup to the cached object would fail for any non-zero 'tos'.
Branch looks OK to me, though I kinda wish libnl could get modified to help us out here, so less code in NM is necessary.
Agree with dcbw that this is becoming a total mess. We need to make this better somehow. > platform: fix out-of-bound access in ip6_route_exists() you could pass &in6addr_any instead? (It's not really so much "out-of-bound access" as it is "completely broken code"... it's accidentally passing INADDR_ANY instead of a pointer to it, which only compiles because INADDR_ANY is 0, which gets silently cast to NULL. And even if it was passing a pointer-to-an-address rather than an address, it would still have the problem that the address was too short if family was AF_INET6.) > platform: fix check_cache_items() to check items in two steps A GSList would be more idiomatic, but this works
(In reply to comment #2) > Agree with dcbw that this is becoming a total mess. We need to make this better > somehow. I agree, the solution is IMO to cache our native NMPlatform* objects, so we have full control. > > platform: fix out-of-bound access in ip6_route_exists() > > you could pass &in6addr_any instead? > > (It's not really so much "out-of-bound access" as it is "completely broken > code"... it's accidentally passing INADDR_ANY instead of a pointer to it, which > only compiles because INADDR_ANY is 0, which gets silently cast to NULL. And > even if it was passing a pointer-to-an-address rather than an address, it would > still have the problem that the address was too short if family was AF_INET6.) Ah, I actually got that wrong. So, it was not a bug in behavior (but certainly not intended as such). Commit now fixes platform/trivial: fix typo in ip6_route_exists() Uppss... just now noticed a left-over typo in the commit message... > > platform: fix check_cache_items() to check items in two steps > > A GSList would be more idiomatic, but this works I like the pointer array better (and it should perform better too!! :) ) Merged to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=master
Sorry, the merge broke something. Pushed additional fix http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=253bfa5c4729d4a04d9f2331350bfed43da3585f
(In reply to comment #3) > > Merged to master as: > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=master I meant to say: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=98052255ba2c4b344646985a07c551014ef22c1a hfff... [note to self: don't merge anything in the last hour before week end]