From 9a322b4e19e8a5d38ade00c25aa76d65c41800c6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 12:21:19 +0100 Subject: [PATCH 1/6] default-route-manager: simplify determining synced flag in _ipx_update_default_route() No change in behavior at all. The same logic applies, but this should be simpler to understand. (cherry picked from commit 0b3ba99409c135716292b6141d2161d395bca46b) --- src/nm-default-route-manager.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 5944654cff..60422dd6bc 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -784,9 +784,8 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, default_route = &rt.rx; never_default = TRUE; - synced = TRUE; - } else - synced = default_route && !is_assumed; + } + synced = !is_assumed; } else { NMConnection *connection = nm_active_connection_get_applied_connection ((NMActiveConnection *) vpn); From 27c7b786d9acfd7dea980721c9bd92a402b3fd19 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 12:46:40 +0100 Subject: [PATCH 2/6] default-route-manager: simplify _platform_changed_cb() handling There is only one caller of _platform_ipx_route_changed_cb(). Inline it, it is simpler. (cherry picked from commit 70ab174e0e486676c2372e63d3ae4a142dc31a9d) --- src/nm-default-route-manager.c | 61 +++++++++++++++------------------- 1 file changed, 27 insertions(+), 34 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 60422dd6bc..1bf7d50146 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -1335,15 +1335,36 @@ _resync_idle_reschedule (NMDefaultRouteManager *self) } static void -_platform_ipx_route_changed_cb (const VTableIP *vtable, - NMDefaultRouteManager *self, - const NMPlatformIPRoute *route) +_platform_changed_cb (NMPlatform *platform, + int obj_type_i, + int ifindex, + gpointer platform_object, + int change_type_i, + NMDefaultRouteManager *self) { NMDefaultRouteManagerPrivate *priv; + const NMPObjectType obj_type = obj_type_i; + const VTableIP *vtable; - if (route && !NM_PLATFORM_IP_ROUTE_IS_DEFAULT (route)) { - /* we only care about address changes or changes of default route. */ - return; + switch (obj_type) { + case NMP_OBJECT_TYPE_IP4_ADDRESS: + vtable = &vtable_ip4; + break; + case NMP_OBJECT_TYPE_IP6_ADDRESS: + vtable = &vtable_ip6; + break; + case NMP_OBJECT_TYPE_IP4_ROUTE: + if (!NM_PLATFORM_IP_ROUTE_IS_DEFAULT (platform_object)) + return; + vtable = &vtable_ip4; + break; + case NMP_OBJECT_TYPE_IP6_ROUTE: + if (!NM_PLATFORM_IP_ROUTE_IS_DEFAULT (platform_object)) + return; + vtable = &vtable_ip6; + break; + default: + g_return_if_reached (); } priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); @@ -1361,34 +1382,6 @@ _platform_ipx_route_changed_cb (const VTableIP *vtable, _resync_idle_reschedule (self); } -static void -_platform_changed_cb (NMPlatform *platform, - int obj_type_i, - int ifindex, - gpointer platform_object, - int change_type_i, - NMDefaultRouteManager *self) -{ - const NMPObjectType obj_type = obj_type_i; - - switch (obj_type) { - case NMP_OBJECT_TYPE_IP4_ADDRESS: - _platform_ipx_route_changed_cb (&vtable_ip4, self, NULL); - break; - case NMP_OBJECT_TYPE_IP6_ADDRESS: - _platform_ipx_route_changed_cb (&vtable_ip6, self, NULL); - break; - case NMP_OBJECT_TYPE_IP4_ROUTE: - _platform_ipx_route_changed_cb (&vtable_ip4, self, (const NMPlatformIPRoute *) platform_object); - break; - case NMP_OBJECT_TYPE_IP6_ROUTE: - _platform_ipx_route_changed_cb (&vtable_ip6, self, (const NMPlatformIPRoute *) platform_object); - break; - default: - g_return_if_reached (); - } -} - /*****************************************************************************/ static void From 5e12056c9e6f646a5c39dbb73f9993c6ddfc50d0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 13:07:40 +0100 Subject: [PATCH 3/6] default-route-manager: add nm_default_route_manager_resync() function (cherry picked from commit e181956fdd038f96685b7ca0bff5948f799c2b2d) --- src/nm-default-route-manager.c | 62 ++++++++++++++++++++++++++++++++-- src/nm-default-route-manager.h | 3 ++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 1bf7d50146..8c6f9840b2 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -1258,7 +1258,7 @@ static const VTableIP vtable_ip6 = { /*****************************************************************************/ static gboolean -_resync_idle_now (NMDefaultRouteManager *self) +_resync_now (NMDefaultRouteManager *self) { gboolean has_v4_changes, has_v6_changes; gboolean changed = FALSE; @@ -1273,7 +1273,7 @@ _resync_idle_now (NMDefaultRouteManager *self) priv->resync.has_v4_changes = FALSE; priv->resync.has_v6_changes = FALSE; - priv->resync.idle_handle = 0; + nm_clear_g_source (&priv->resync.idle_handle); priv->resync.backoff_wait_time_ms = priv->resync.backoff_wait_time_ms == 0 ? 100 @@ -1290,6 +1290,64 @@ _resync_idle_now (NMDefaultRouteManager *self) _resync_idle_cancel (self); } + return changed; +} + +/** + * nm_default_route_manager_resync: + * @self: the #NMDefaultRouteManager instance + * @af_family: the address family to resync, can be + * AF_INET, AF_INET6 or AF_UNSPEC to sync both. + * + * #NMDefaultRouteManager keeps an internal list of configured + * routes. Usually, it configures routes in the system only + * - when that internal list changes due to + * nm_default_route_manager_ip4_update_default_route() or + * nm_default_route_manager_ip6_update_default_route(). + * - when platform notifies about changes, via _resync_idle_now(). + * This forces a resync to update the internal bookkeeping + * with what is currently configured in the system, but also + * reconfigure the system with all non-assumed default routes. + * + * Returns: %TRUE if anything changed during resync. + */ +gboolean +nm_default_route_manager_resync (NMDefaultRouteManager *self, + int af_family) +{ + NMDefaultRouteManagerPrivate *priv; + + g_return_val_if_fail (NM_IS_DEFAULT_ROUTE_MANAGER (self), FALSE); + g_return_val_if_fail (NM_IN_SET (af_family, AF_INET, AF_INET6, AF_UNSPEC), FALSE); + + priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + if (priv->disposed) + return FALSE; + + switch (af_family) { + case AF_INET: + priv->resync.has_v4_changes = TRUE; + break; + case AF_INET6: + priv->resync.has_v6_changes = TRUE; + break; + default: + priv->resync.has_v4_changes = TRUE; + priv->resync.has_v6_changes = TRUE; + break; + } + + return _resync_now (self); +} + +static gboolean +_resync_idle_now (NMDefaultRouteManager *self) +{ + NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + priv->resync.idle_handle = 0; + _resync_now (self); return G_SOURCE_REMOVE; } diff --git a/src/nm-default-route-manager.h b/src/nm-default-route-manager.h index bd5e873295..cecc9501a9 100644 --- a/src/nm-default-route-manager.h +++ b/src/nm-default-route-manager.h @@ -61,4 +61,7 @@ NMIP6Config *nm_default_route_manager_ip6_get_best_config (NMDefaultRouteManager NMDevice **out_device, NMVpnConnection **out_vpn); +gboolean nm_default_route_manager_resync (NMDefaultRouteManager *self, + int af_family); + #endif /* NM_DEFAULT_ROUTE_MANAGER_H */ From ac515194a5a1ad0a3715aaa02ffb739242437c23 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 14:01:26 +0100 Subject: [PATCH 4/6] default-route-manager: use nm_cmp_uint32_p_with_data() instead of reimplementation (cherry picked from commit 0057dc332e597bf067672eb65ae8f26c2a19287f) --- src/nm-default-route-manager.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 8c6f9840b2..c9620b6b02 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -483,17 +483,6 @@ _get_assumed_interface_metrics (const VTableIP *vtable, NMDefaultRouteManager *s return result; } -static int -_sort_metrics_ascending_fcn (gconstpointer a, gconstpointer b) -{ - guint32 m_a = *((guint32 *) a); - guint32 m_b = *((guint32 *) b); - - if (m_a < m_b) - return -1; - return m_a == m_b ? 0 : 1; -} - static gboolean _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry, gboolean external_change) { @@ -624,7 +613,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c g_array_free (routes, TRUE); - g_array_sort (changed_metrics, _sort_metrics_ascending_fcn); + g_array_sort_with_data (changed_metrics, nm_cmp_uint32_p_with_data, NULL); last_metric = -1; for (j = 0; j < changed_metrics->len; j++) { expected_metric = g_array_index (changed_metrics, guint32, j); From 6c7ef310b1f86575dbb79e8bf312810060bb55b3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 13:13:21 +0100 Subject: [PATCH 5/6] default-route-manager: alyways force a sync of the default route Whenever we call update for a non-assumed, synced route, we must force a resync with the platform. Even if according to our internal book-keeping the route is already configured, the route may have been removed externally. So we cannot assume that everything is still up-to-date. https://bugzilla.redhat.com/show_bug.cgi?id=1431268 (cherry picked from commit c3c251ea129eeb562d32ac44029714f8d644ad18) --- src/nm-default-route-manager.c | 43 +++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index c9620b6b02..dfad2b0654 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -525,8 +525,6 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c for (i = 0; i < entries->len; i++) { entry = g_ptr_array_index (entries, i); - g_assert (entry != old_entry); - if (entry->never_default) continue; @@ -581,12 +579,15 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c /* for the changed entry, the previous metric was either old_entry->effective_metric, * or none. Hence, we only have to remember what is going to change. */ g_array_append_val (changed_metrics, expected_metric); - if (old_entry) { + if (!old_entry) { + _LOG2D (vtable, i, entry, "sync:add %s (%u)", + vtable->vt->route_to_string (&entry->route, NULL, 0), (guint) expected_metric); + } else if (old_entry != changed_entry) { _LOG2D (vtable, i, entry, "sync:update %s (%u -> %u)", vtable->vt->route_to_string (&entry->route, NULL, 0), (guint) old_entry->effective_metric, (guint) expected_metric); } else { - _LOG2D (vtable, i, entry, "sync:add %s (%u)", + _LOG2D (vtable, i, entry, "sync:resync %s (%u)", vtable->vt->route_to_string (&entry->route, NULL, 0), (guint) expected_metric); } } else if (entry->effective_metric != expected_metric) { @@ -664,7 +665,11 @@ _entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry->effective_metric = entry->route.rx.metric; _LOG2D (vtable, entry_idx, entry, "%s %s (%"G_GUINT32_FORMAT")", - old_entry ? "record:update" : "record:add ", + old_entry + ? (entry != old_entry + ? "record:update" + : "record:resync") + : "record:add ", vtable->vt->route_to_string (&entry->route, NULL, 0), entry->effective_metric); @@ -701,7 +706,9 @@ _entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint /*****************************************************************************/ static void -_ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, gpointer source) +_ipx_update_default_route (const VTableIP *vtable, + NMDefaultRouteManager *self, + gpointer source) { NMDefaultRouteManagerPrivate *priv; Entry *entry; @@ -866,12 +873,18 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, new_entry.never_default = never_default; new_entry.synced = synced; - if (memcmp (entry, &new_entry, sizeof (new_entry)) == 0) - return; - - old_entry = *entry; - *entry = new_entry; - _entry_at_idx_update (vtable, self, entry_idx, &old_entry); + if (memcmp (entry, &new_entry, sizeof (new_entry)) == 0) { + if (!synced) { + /* the internal book-keeping doesn't change, so don't do a full + * sync of the configured routes. */ + return; + } + _entry_at_idx_update (vtable, self, entry_idx, entry); + } else { + old_entry = *entry; + *entry = new_entry; + _entry_at_idx_update (vtable, self, entry_idx, &old_entry); + } } else { /* delete */ _entry_at_idx_remove (vtable, self, entry_idx); @@ -879,13 +892,15 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, } void -nm_default_route_manager_ip4_update_default_route (NMDefaultRouteManager *self, gpointer source) +nm_default_route_manager_ip4_update_default_route (NMDefaultRouteManager *self, + gpointer source) { _ipx_update_default_route (&vtable_ip4, self, source); } void -nm_default_route_manager_ip6_update_default_route (NMDefaultRouteManager *self, gpointer source) +nm_default_route_manager_ip6_update_default_route (NMDefaultRouteManager *self, + gpointer source) { _ipx_update_default_route (&vtable_ip6, self, source); } From 172f5bb2e97539e8527be0d169a9fbb397c1f659 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 15:26:18 +0100 Subject: [PATCH 6/6] default-route-manager: decryptify logging line for default-route-manager The default route manager logs for each entry relevant information, in a compact but cryptic way: default-route: entry[0/dev:0x5633d5528560:enp0s25:1:+sync]: record:add 0.0.0.0/0 via 192.168.0.1 dev 2 metric 100 mss 0 rt-src user (100) The flag whether a route is configured or not, was only expressed via 0|1. Change that to log instead: default-route: entry[0/dev:0x5633d5528560:enp0s25:+has:+sync]: record:add 0.0.0.0/0 via 192.168.0.1 dev 2 metric 100 mss 0 rt-src user (100) (cherry picked from commit 82bfb6c46d2ff1ca01c7dffd0a812bf53e08ff33) --- src/nm-default-route-manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index dfad2b0654..9e7bc1e3b8 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -124,7 +124,7 @@ NM_DEFINE_SINGLETON_GETTER (NMDefaultRouteManager, nm_default_route_manager_get, const Entry *const __entry = (entry); \ \ _nm_log (__level, __domain, 0, \ - "%s: entry[%u/%s:%p:%s:%c:%csync]: "_NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + "%s: entry[%u/%s:%p:%s:%chas:%csync]: "_NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ self != singleton_instance \ ? nm_sprintf_buf (__prefix_buf, "%s%c[%p]", \ _NMLOG2_PREFIX_NAME, \ @@ -135,7 +135,7 @@ NM_DEFINE_SINGLETON_GETTER (NMDefaultRouteManager, nm_default_route_manager_get, NM_IS_DEVICE (__entry->source.pointer) ? "dev" : "vpn", \ __entry->source.pointer, \ NM_IS_DEVICE (__entry->source.pointer) ? nm_device_get_iface (__entry->source.device) : nm_active_connection_get_settings_connection_id (NM_ACTIVE_CONNECTION (__entry->source.vpn)), \ - (__entry->never_default ? '0' : '1'), \ + (__entry->never_default ? '-' : '+'), \ (__entry->synced ? '+' : '-') \ _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ } \