From cd7c79e83f85150dce05dde1e7ef4b4303ca146e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 12:21:19 +0100 Subject: [PATCH 1/3] 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 2fd2c7db47..c1327ece02 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -770,9 +770,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 9e81a339499960dd644b8f8a5544974daccef309 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 13:13:21 +0100 Subject: [PATCH 2/3] 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 c1327ece02..0764a702b0 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -522,8 +522,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; @@ -578,12 +576,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) { @@ -661,7 +662,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); @@ -698,7 +703,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; @@ -863,12 +870,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); @@ -876,13 +889,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 6cc6c429934dc674d40fb5956d11f27803885fe0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Mar 2017 15:26:18 +0100 Subject: [PATCH 3/3] 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 0764a702b0..a045c59f77 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -110,7 +110,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, \ @@ -121,7 +121,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__)); \ } \