From e805201f4b75434641b400c59a1a0f30a9422ca9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Sep 2017 07:10:42 +0200 Subject: [PATCH] platform: delete routes after adding in nm_platform_ip_route_sync() Previously, we would first delete routes that are not to be added, before adding the new ones. This has the advantage, that even if delete removes the wrong route, add would restore the expected state. This tries to workaround the fact that RTM_DELROUTE allows for wild-card fields, and might delete the wrong route. However, for example when bumping the route metric after connectivty check (removing the default-route with metric 20100 and adding the one with metric 100), there is a short moment when there is no default-route. To avoid that, don't do delete-then-add, but add-then-delete. --- src/platform/nm-platform.c | 106 ++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 60 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 1b47702cc6..0d40e0773d 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3592,54 +3592,7 @@ nm_platform_ip_route_sync (NMPlatform *self, ? &nm_platform_vtable_route_v4 : &nm_platform_vtable_route_v6; - plat_routes = nm_platform_lookup_addrroute_clone (self, - vt->obj_type, - ifindex, - kernel_delete_predicate, - kernel_delete_userdata); - /* first delete routes which are in platform (@plat_routes), but not to configure (@routes/@routes_idx). */ - if (plat_routes) { - - /* create a lookup index. */ - if (routes && routes->len > 0) { - routes_idx = g_hash_table_new ((GHashFunc) nmp_object_id_hash, - (GEqualFunc) nmp_object_id_equal); - for (i = 0; i < routes->len; i++) { - conf_o = routes->pdata[i]; - if (!nm_g_hash_table_insert (routes_idx, (gpointer) conf_o, (gpointer) conf_o)) { - /* we ignore duplicate @routes. */ - } - } - } - - for (i = 0; i < plat_routes->len; i++) { - plat_o = plat_routes->pdata[i]; - - if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (NMP_OBJECT_CAST_IP_ROUTE (plat_o))) { - /* don't delete default routes. */ - continue; - } - - if ( routes_idx - && (conf_o = g_hash_table_lookup (routes_idx, plat_o)) - && vt->route_cmp (NMP_OBJECT_CAST_IPX_ROUTE (conf_o), - NMP_OBJECT_CAST_IPX_ROUTE (plat_o), - NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) == 0) { - /* the route in platform is identical to the one we want to add. - * Keep it. */ - continue; - } - - if (!nm_platform_ip_route_delete (self, plat_o)) { - /* ignore error... */ - } - } - } - - if (!routes) - return success; - - for (i_type = 0; i_type < 2; i_type++) { + for (i_type = 0; routes && i_type < 2; i_type++) { for (i = 0; i < routes->len; i++) { NMPlatformError plerr; @@ -3657,23 +3610,32 @@ nm_platform_ip_route_sync (NMPlatform *self, continue; } + if (!routes_idx) { + routes_idx = g_hash_table_new ((GHashFunc) nmp_object_id_hash, + (GEqualFunc) nmp_object_id_equal); + } + if (!nm_g_hash_table_insert (routes_idx, (gpointer) conf_o, (gpointer) conf_o)) { + _LOGD ("route-sync: skip adding duplicate route %s", + nmp_object_to_string (conf_o, NMP_OBJECT_TO_STRING_PUBLIC, sbuf1, sizeof (sbuf1))); + continue; + } + plat_entry = nm_platform_lookup_entry (self, NMP_CACHE_ID_TYPE_OBJECT_TYPE, conf_o); if (plat_entry) { - /* we alreay have a route with the same ID in the platform cache. - * Skip adding it again. It should be identical already, otherwise we - * would have deleted above. */ - if (_LOGD_ENABLED ()) { - if (vt->route_cmp (NMP_OBJECT_CAST_IPX_ROUTE (conf_o), - NMP_OBJECT_CAST_IPX_ROUTE (plat_entry->obj), - NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) != 0) { - _LOGD ("route-sync: skip adding route %s due to existing (different!) route %s", - nmp_object_to_string (conf_o, NMP_OBJECT_TO_STRING_PUBLIC, sbuf1, sizeof (sbuf1)), - nmp_object_to_string (plat_entry->obj, NMP_OBJECT_TO_STRING_PUBLIC, sbuf2, sizeof (sbuf2))); - } + plat_o = plat_entry->obj; + + if (vt->route_cmp (NMP_OBJECT_CAST_IPX_ROUTE (conf_o), + NMP_OBJECT_CAST_IPX_ROUTE (plat_o), + NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) == 0) + continue; + + /* we need to replace the existing route with a (slightly) differnt + * one. Delete it first. */ + if (!nm_platform_ip_route_delete (self, plat_o)) { + /* ignore error. */ } - continue; } plerr = nm_platform_ip_route_add (self, @@ -3725,6 +3687,30 @@ nm_platform_ip_route_sync (NMPlatform *self, } } + plat_routes = nm_platform_lookup_addrroute_clone (self, + vt->obj_type, + ifindex, + kernel_delete_predicate, + kernel_delete_userdata); + + if (plat_routes) { + for (i = 0; i < plat_routes->len; i++) { + plat_o = plat_routes->pdata[i]; + + if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (NMP_OBJECT_CAST_IP_ROUTE (plat_o))) { + /* don't delete default routes. */ + continue; + } + + if ( !routes_idx + || !g_hash_table_lookup (routes_idx, plat_o)) { + if (!nm_platform_ip_route_delete (self, plat_o)) { + /* ignore error... */ + } + } + } + } + return success; }