From 5a6fc10000d04c62a7c756a915f8e5ac4eecaae1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:17:54 +0200 Subject: [PATCH 1/7] connectivity: improve logging of completed connectivity check in device --- src/devices/nm-device.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index cc3df54ae8..ab743705d2 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2513,18 +2513,20 @@ concheck_cb (NMConnectivity *connectivity, handle->c_handle = NULL; self = g_object_ref (handle->self); - _LOGT (LOGD_CONCHECK, "connectivity: complete check (seq:%llu, state:%s%s%s%s)", - (long long unsigned) handle->seq, - nm_connectivity_state_to_string (state), - NM_PRINT_FMT_QUOTED (error, ", error: ", error->message, "", "")); - if (nm_utils_error_is_cancelled (error, FALSE)) { /* the only place where we nm_connectivity_check_cancel(@c_handle), is * from inside concheck_handle_event(). This is a recursive call, * nothing to do. */ + _LOGT (LOGD_CONCHECK, "connectivity: complete check (seq:%llu, obsoleted by later request returning)", + (long long unsigned) handle->seq); return; } + _LOGT (LOGD_CONCHECK, "connectivity: complete check (seq:%llu, state:%s%s%s%s)", + (long long unsigned) handle->seq, + nm_connectivity_state_to_string (state), + NM_PRINT_FMT_QUOTED (error, ", error: ", error->message, "", "")); + /* we keep NMConnectivity instance alive. It cannot be disposing. */ nm_assert (!nm_utils_error_is_cancelled (error, TRUE)); From 8e8efbdc3228abcabfbd0bbdfe5fd746abe2d6ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:27:04 +0200 Subject: [PATCH 2/7] connectivity: only take reference in concheck_cb() if necessary We don't need to take a reference, if we are not going to emit any signals. --- src/devices/nm-device.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ab743705d2..4607ae3113 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2499,7 +2499,8 @@ concheck_cb (NMConnectivity *connectivity, GError *error, gpointer user_data) { - gs_unref_object NMDevice *self = NULL; + _nm_unused gs_unref_object NMDevice *self_keep_alive = NULL; + NMDevice *self; NMDevicePrivate *priv; NMDeviceConnectivityHandle *handle; NMDeviceConnectivityHandle *other_handle; @@ -2511,7 +2512,7 @@ concheck_cb (NMConnectivity *connectivity, nm_assert (NM_IS_DEVICE (handle->self)); handle->c_handle = NULL; - self = g_object_ref (handle->self); + self = handle->self; if (nm_utils_error_is_cancelled (error, FALSE)) { /* the only place where we nm_connectivity_check_cancel(@c_handle), is @@ -2522,6 +2523,8 @@ concheck_cb (NMConnectivity *connectivity, return; } + self_keep_alive = g_object_ref (self); + _LOGT (LOGD_CONCHECK, "connectivity: complete check (seq:%llu, state:%s%s%s%s)", (long long unsigned) handle->seq, nm_connectivity_state_to_string (state), From 8bb058386d6a74dcb5283f46c2b0aeb3d06a9c5c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:31:39 +0200 Subject: [PATCH 3/7] connectivity: add connectivity-changed signal to NMDevice NMManager very much cares about changes to the connectivity state of the device and was therefore listening to notify::connectivity signals. However, property changed signals can be suppressed by g_object_freeze_notify(). That is something we even encourage for NMDBusObject instances, because the D-Bus glue makes use of the property changed notifications, and encourages to combine multiple changes by freezing the signal. Using the property changed notifications of NMDBusObject instances is ugly. Don't do that and instead add a special signal. --- src/devices/nm-device.c | 10 ++++++++++ src/devices/nm-device.h | 1 + src/nm-manager.c | 3 +-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4607ae3113..55667ffd78 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -180,6 +180,7 @@ enum { REMOVED, RECHECK_AUTO_ACTIVATE, RECHECK_ASSUME, + CONNECTIVITY_CHANGED, LAST_SIGNAL, }; static guint signals[LAST_SIGNAL] = { 0 }; @@ -2457,6 +2458,7 @@ concheck_update_state (NMDevice *self, NMConnectivityState state, gboolean is_pe priv->connectivity_state = state; _notify (self, PROP_CONNECTIVITY); + g_signal_emit (self, signals[CONNECTIVITY_CHANGED], 0); if ( priv->state == NM_DEVICE_STATE_ACTIVATED && !nm_device_sys_iface_state_is_external (self)) { @@ -15744,4 +15746,12 @@ nm_device_class_init (NMDeviceClass *klass) G_SIGNAL_RUN_FIRST, 0, NULL, NULL, NULL, G_TYPE_NONE, 0); + + signals[CONNECTIVITY_CHANGED] = + g_signal_new (NM_DEVICE_CONNECTIVITY_CHANGED, + G_OBJECT_CLASS_TYPE (object_class), + G_SIGNAL_RUN_FIRST, + 0, NULL, NULL, + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0); } diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index fd266d69ab..66720f015c 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -135,6 +135,7 @@ nm_device_state_reason_check (NMDeviceStateReason reason) #define NM_DEVICE_STATE_CHANGED "state-changed" #define NM_DEVICE_LINK_INITIALIZED "link-initialized" #define NM_DEVICE_AUTOCONNECT_ALLOWED "autoconnect-allowed" +#define NM_DEVICE_CONNECTIVITY_CHANGED "connectivity-changed" #define NM_DEVICE_STATISTICS_REFRESH_RATE_MS "refresh-rate-ms" #define NM_DEVICE_STATISTICS_TX_BYTES "tx-bytes" diff --git a/src/nm-manager.c b/src/nm-manager.c index fcebcfd314..cdaeb8a107 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2512,7 +2512,6 @@ device_realized (NMDevice *device, static void device_connectivity_changed (NMDevice *device, - GParamSpec *pspec, NMManager *self) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); @@ -2646,7 +2645,7 @@ add_device (NMManager *self, NMDevice *device, GError **error) G_CALLBACK (device_realized), self); - g_signal_connect (device, "notify::" NM_DEVICE_CONNECTIVITY, + g_signal_connect (device, NM_DEVICE_CONNECTIVITY_CHANGED, G_CALLBACK (device_connectivity_changed), self); From 8c805c943cbc291da2adf49d7c517b010ab5801d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:35:14 +0200 Subject: [PATCH 4/7] connectivity: optmize finding best connectivty state in NMManager::device_connectivity_changed() It doesn't get better than FULL, so we can abort the loop in that case early. --- src/nm-manager.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index cdaeb8a107..c267e465cf 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2519,11 +2519,20 @@ device_connectivity_changed (NMDevice *device, NMConnectivityState state; NMDevice *dev; - c_list_for_each_entry (dev, &priv->devices_lst_head, devices_lst) { - state = nm_device_get_connectivity_state (dev); - if (state > best_state) + best_state = nm_device_get_connectivity_state (device); + if (best_state < NM_CONNECTIVITY_FULL) { + c_list_for_each_entry (dev, &priv->devices_lst_head, devices_lst) { + state = nm_device_get_connectivity_state (dev); + if (state <= best_state) + continue; best_state = state; + if (best_state >= NM_CONNECTIVITY_FULL) { + /* it doesn't get better than this. */ + break; + } + } } + nm_assert (best_state <= NM_CONNECTIVITY_FULL); if (best_state != priv->connectivity_state) { priv->connectivity_state = best_state; From 835a5f7248aeb618ae00dd5ae85647f34611d718 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:37:04 +0200 Subject: [PATCH 5/7] connectivity: also expose fake connectivity states to dispatcher The fake states still encode whether the device have a default-route. So, they are not entirely useless. Also, don't add special handling of "#if !WITH_CONCHECK" where we don't need it. There is no fundamental difference between compiling without connectivity check and disabling connectivity checks. --- src/nm-dispatcher.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 29bf5c9748..2f88d468eb 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -631,9 +631,7 @@ _dispatcher_call (NMDispatcherAction action, if (!device_dhcp6_props) device_dhcp6_props = g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("{sv}"), NULL, 0)); -#if WITH_CONCHECK connectivity_state_string = nm_connectivity_state_to_string (connectivity_state); -#endif /* Send the action to the dispatcher */ if (blocking) { From f48b4af85035cbd9e539fc00e8432c2fece1c53d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 11:44:47 +0200 Subject: [PATCH 6/7] manager: merge set_state() in nm_manager_update_state() function --- src/nm-manager.c | 44 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index c267e465cf..034675b9e8 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1142,26 +1142,6 @@ _nm_state_to_string (NMState state) } } -static void -set_state (NMManager *self, NMState state) -{ - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - - if (priv->state == state) - return; - - priv->state = state; - - _LOGI (LOGD_CORE, "NetworkManager state is now %s", _nm_state_to_string (state)); - - _notify (self, PROP_STATE); - nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), - &interface_info_manager, - &signal_info_state_changed, - "(u)", - (guint32) priv->state); -} - static NMState find_best_device_state (NMManager *manager) { @@ -1232,26 +1212,38 @@ nm_manager_update_metered (NMManager *self) } static void -nm_manager_update_state (NMManager *manager) +nm_manager_update_state (NMManager *self) { NMManagerPrivate *priv; NMState new_state = NM_STATE_DISCONNECTED; - g_return_if_fail (NM_IS_MANAGER (manager)); + g_return_if_fail (NM_IS_MANAGER (self)); - priv = NM_MANAGER_GET_PRIVATE (manager); + priv = NM_MANAGER_GET_PRIVATE (self); - if (manager_sleeping (manager)) + if (manager_sleeping (self)) new_state = NM_STATE_ASLEEP; else - new_state = find_best_device_state (manager); + new_state = find_best_device_state (self); if ( new_state >= NM_STATE_CONNECTED_LOCAL && priv->connectivity_state == NM_CONNECTIVITY_FULL) { new_state = NM_STATE_CONNECTED_GLOBAL; } - set_state (manager, new_state); + if (priv->state == new_state) + return; + + priv->state = new_state; + + _LOGI (LOGD_CORE, "NetworkManager state is now %s", _nm_state_to_string (new_state)); + + _notify (self, PROP_STATE); + nm_dbus_object_emit_signal (NM_DBUS_OBJECT (self), + &interface_info_manager, + &signal_info_state_changed, + "(u)", + (guint32) priv->state); } static void From d0cee07a0f1bc788cb117446cedd9157a572d961 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 12:04:56 +0200 Subject: [PATCH 7/7] connectivity: fix the device's fake connectivity state Otherwise, if connectivity checking was disabled, we would never reset the connectivity state and leave it wrongly at UNKNOWN. nm_device_check_connectivity_update_interval() is already called during state-changes, so this is the right place. However, it's far from perfect still, because we might not notice when a default-route gets added or removed. Also, devices that are not in ACTIVATED state, are considered with connectivity NONE. Which might not be correct. Fixes: 0a62a0e9039e49e05dae6af9a96c381629bccd58 --- src/devices/nm-device.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 55667ffd78..9b9584ce71 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -632,6 +632,8 @@ static void _set_mtu (NMDevice *self, guint32 mtu); static void _commit_mtu (NMDevice *self, const NMIP4Config *config); static void _cancel_activation (NMDevice *self); +static void concheck_update_state (NMDevice *self, NMConnectivityState state, gboolean is_periodic); + /*****************************************************************************/ NM_UTILS_LOOKUP_STR_DEFINE_STATIC (queued_state_to_string, NMDeviceState, @@ -2390,6 +2392,9 @@ nm_device_check_connectivity_update_interval (NMDevice *self) if (!new_interval) { /* this will cancel any potentially pending timeout. */ concheck_periodic_schedule_do (self, 0); + + /* also update the fake connectivity state. */ + concheck_update_state (self, NM_CONNECTIVITY_FAKE, TRUE); return; } @@ -2419,6 +2424,10 @@ concheck_update_state (NMDevice *self, NMConnectivityState state, gboolean is_pe /* If the connectivity check is disabled and we obtain a fake * result, make an optimistic guess. */ if (priv->state == NM_DEVICE_STATE_ACTIVATED) { + /* FIXME: the fake connectivity state depends on the availablility of + * a default route. However, we have no mechanism that rechecks the + * value if a device route appears/disappears after the device + * was activated. */ if (nm_device_get_best_default_route (self, AF_UNSPEC)) state = NM_CONNECTIVITY_FULL; else