From 4417b8bf3eef8ca7cf45dc973ab107249bd10d67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 15:11:10 +0200 Subject: [PATCH 1/5] core: add nm_utils_get_monotonic_timestamp_ns_cached() helper Add a helper function to cache the current timestamp and return it. The caching is a performance optimization, but it serves a much more important purpose: repeatedly getting the timestamp likely will yield different timings. So, commonly, within a certain context we want to get the current time once, and stick to that as "now". --- src/nm-core-utils.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index d974c8af36..33773ab679 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -239,6 +239,13 @@ gint64 nm_utils_get_monotonic_timestamp_ms (void); gint32 nm_utils_get_monotonic_timestamp_s (void); gint64 nm_utils_monotonic_timestamp_as_boottime (gint64 timestamp, gint64 timestamp_ticks_per_ns); +static inline gint64 +nm_utils_get_monotonic_timestamp_ns_cached (gint64 *cache_now) +{ + return (*cache_now) + ?: (*cache_now = nm_utils_get_monotonic_timestamp_ns ()); +} + gboolean nm_utils_is_valid_path_component (const char *name); const char *NM_ASSERT_VALID_PATH_COMPONENT (const char *name); From d8a31794c8b9db243076ba0c24dfe6e496b78697 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jan 2018 17:46:49 +0100 Subject: [PATCH 2/5] connectivity: rework async connectivity check requests An asynchronous request should either be cancellable or not keep the target object alive. Preferably both. Otherwise, it is impossible to do a controlled shutdown when terminating NetworkManager. Currently, when NetworkManager is about to terminate, it just quits the mainloop and essentially leaks everything. That is a bug. If we ever want to fix that, every asynchronous request must be cancellable in a controlled way (or it must not prevent objects from getting disposed, where disposing the object automatically cancels the callback). Rework the asynchronous request for connectivity check to - return a handle that can be used to cancel the operation. Cancelling is optional. The caller may choose to ignore the handle because the asynchronous operation does not keep the target object alive. That means, it is still possible to shutdown, by everybody giving up their reference to the target object. In which case the callback will be invoked during dispose() of the target object. - also, the callback will always be invoked exactly once, and never synchronously from within the asynchronous start call. But during cancel(), the callback is invoked synchronously from within cancel(). Note that it's only allowed to cancel an action at most once, and never after the callback is invoked (also not from within the callback itself). - also, NMConnectivity already supports a fake handler, in case connectivity check is disabled via configuration. Hence, reuse the same code paths also when compiling without --enable-concheck. That means, instead of having #if WITH_CONCHECK at various callers, move them into NMConnectivity. The downside is, that if you build without concheck, there is a small overhead compared to before. The upside is, we reuse the same code paths when compiling with or without concheck. - also, the patch synchronizes the connecitivty states. For example, previously `nmcli networking connectivity check` would schedule requests in parallel, and return the accumulated result of the individual requests. However, the global connectivity state of the manager might have have been the same as the answer to the explicit connecitivity check, because while the answer for the manual check is waiting for all pending checks to complete, the global connectivity state could already change. That is just wrong. There are not multiple global connectivity states at the same time, there is just one. A manual connectivity check should have the meaning of ensure that the global state is up to date, but it still should return the global connectivity state -- not the answers for several connectivity checks issued in parallel. This is related to commit b799de281bc01073c31dd2c86171b29c8132441c (libnm: update property in the manager after connectivity check), which tries to address a similar problem client side. Similarly, each device has a connectivity state. While there might be several connectivity checks per device pending, whenever a check completes, it can update the per-device state (and return that device state as result), but the immediate answer of the individual check might not matter. This is especially the case, when a later request returns earlier and obsoletes all earlier requests. In that case, earlier requests return with the result of the currend devices connectivity state. This patch cleans up the internal API and gives a better defined behavior to the user (thus, the simple API which simplifies implementation for the caller). However, the implementation of getting this API right and properly handle cancel and destruction of the target object is more complicated and complex. But this but is not just for the sake of a nicer API. This fixes actual issues explained above. Also, get rid of GAsyncResult to track information about the pending request. Instead, allocate our own handle structure, which ends up to be nicer because it's strongly typed and has exactly the properties that are useful to track the request. Also, it gets rid of the awkward _finish() API by passing the relevant arguments to the callback directly. --- src/devices/nm-device.c | 358 ++++++++++++++-------- src/devices/nm-device.h | 14 +- src/main.c | 2 - src/nm-connectivity.c | 540 ++++++++++++++++++++------------- src/nm-connectivity.h | 25 +- src/nm-manager.c | 106 ++++--- src/tests/config/test-config.c | 8 +- 7 files changed, 660 insertions(+), 393 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9e1592a750..6142908a57 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -157,6 +157,15 @@ typedef struct { that the original configuration didn't change. */ } AppliedConfig; +struct _NMDeviceConnectivityHandle { + CList concheck_lst; + NMDevice *self; + NMDeviceConnectivityCallback callback; + gpointer user_data; + NMConnectivityCheckHandle *c_handle; + guint64 seq; +}; + /*****************************************************************************/ enum { @@ -531,9 +540,14 @@ typedef struct _NMDevicePrivate { NMNetns *netns; NMLldpListener *lldp_listener; - NMConnectivityState connectivity_state; + + NMConnectivity *concheck_mgr; + gulong concheck_periodic_id; - guint64 concheck_seq; + + NMConnectivityState connectivity_state; + + CList concheck_lst_head; guint check_delete_unrealized_id; @@ -728,6 +742,16 @@ nm_device_get_platform (NMDevice *self) return nm_netns_get_platform (nm_device_get_netns (self)); } +static NMConnectivity * +concheck_get_mgr (NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + if (G_UNLIKELY (!priv->concheck_mgr)) + priv->concheck_mgr = g_object_ref (nm_connectivity_get ()); + return priv->concheck_mgr; +} + static NMIP4Config * _ip4_config_new (NMDevice *self) { @@ -1861,16 +1885,13 @@ nm_device_get_route_metric_default (NMDeviceType device_type) static gboolean default_route_metric_penalty_detect (NMDevice *self) { -#if WITH_CONCHECK NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); /* currently we don't differentiate between IPv4 and IPv6 when detecting * connectivity. */ if ( priv->connectivity_state != NM_CONNECTIVITY_FULL - && nm_connectivity_check_enabled (nm_connectivity_get ())) { + && nm_connectivity_check_enabled (concheck_get_mgr (self))) return TRUE; - } -#endif return FALSE; } @@ -2165,126 +2186,219 @@ nm_device_get_physical_port_id (NMDevice *self) /*****************************************************************************/ static void -update_connectivity_state (NMDevice *self, NMConnectivityState state) +concheck_update_state (NMDevice *self, NMConnectivityState state) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - /* If the connectivity check is disabled, make an optimistic guess. */ - if (state == NM_CONNECTIVITY_UNKNOWN) { + if (state == NM_CONNECTIVITY_ERROR) + return; + + /* If the connectivity check is disabled and we obtain a fake + * result, make an optimistic guess. */ + if (state == NM_CONNECTIVITY_FAKE) { if (priv->state == NM_DEVICE_STATE_ACTIVATED) { if (nm_device_get_best_default_route (self, AF_UNSPEC)) state = NM_CONNECTIVITY_FULL; else state = NM_CONNECTIVITY_LIMITED; - } else { + } else state = NM_CONNECTIVITY_NONE; - } } - if (priv->connectivity_state != state) { -#if WITH_CONCHECK - _LOGD (LOGD_CONCHECK, "state changed from %s to %s", - nm_connectivity_state_to_string (priv->connectivity_state), - nm_connectivity_state_to_string (state)); -#endif - priv->connectivity_state = state; - _notify (self, PROP_CONNECTIVITY); + if (priv->connectivity_state == state) + return; - if ( priv->state == NM_DEVICE_STATE_ACTIVATED - && !nm_device_sys_iface_state_is_external (self)) { - if ( nm_device_get_best_default_route (self, AF_INET) - && !ip_config_merge_and_apply (self, AF_INET, TRUE)) - _LOGW (LOGD_IP4, "Failed to update IPv4 route metric"); - if ( nm_device_get_best_default_route (self, AF_INET6) - && !ip_config_merge_and_apply (self, AF_INET6, TRUE)) - _LOGW (LOGD_IP6, "Failed to update IPv6 route metric"); + _LOGD (LOGD_CONCHECK, "state changed from %s to %s", + nm_connectivity_state_to_string (priv->connectivity_state), + nm_connectivity_state_to_string (state)); + priv->connectivity_state = state; + + _notify (self, PROP_CONNECTIVITY); + + if ( priv->state == NM_DEVICE_STATE_ACTIVATED + && !nm_device_sys_iface_state_is_external (self)) { + if ( nm_device_get_best_default_route (self, AF_INET) + && !ip_config_merge_and_apply (self, AF_INET, TRUE)) + _LOGW (LOGD_IP4, "Failed to update IPv4 route metric"); + if ( nm_device_get_best_default_route (self, AF_INET6) + && !ip_config_merge_and_apply (self, AF_INET6, TRUE)) + _LOGW (LOGD_IP6, "Failed to update IPv6 route metric"); + } +} + +static void +concheck_periodic (NMConnectivity *connectivity, NMDevice *self) +{ + nm_device_check_connectivity (self, NULL, NULL); +} + +static void +concheck_periodic_update (NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + if ( priv->state == NM_DEVICE_STATE_ACTIVATED + && nm_device_get_best_default_route (self, AF_UNSPEC)) { + if (!priv->concheck_periodic_id) { + priv->concheck_periodic_id = g_signal_connect (concheck_get_mgr (self), + NM_CONNECTIVITY_PERIODIC_CHECK, + G_CALLBACK (concheck_periodic), self); + nm_device_check_connectivity (self, NULL, NULL); + } + } else { + if (priv->concheck_periodic_id) { + /* The default route has gone off, trigger a final connectivity check. */ + nm_clear_g_signal_handler (priv->concheck_mgr, &priv->concheck_periodic_id); + nm_device_check_connectivity (self, NULL, NULL); } } } -typedef struct { - NMDevice *self; - NMDeviceConnectivityCallback callback; - gpointer user_data; +static void +concheck_handle_complete (NMDeviceConnectivityHandle *handle, + GError *error) +{ + /* The moment we invoke the callback, we unlink it. It signals + * that @handle is handled -- as far as the callee of callback + * is concerned. */ + c_list_unlink (&handle->concheck_lst); + + if (handle->c_handle) + nm_connectivity_check_cancel (g_steal_pointer (&handle->c_handle)); + + if (handle->callback) { + handle->callback (handle->self, + handle, + NM_DEVICE_GET_PRIVATE (handle->self)->connectivity_state, + error, + handle->user_data); + } + + g_slice_free (NMDeviceConnectivityHandle, handle); +} + +static void +concheck_cb (NMConnectivity *connectivity, + NMConnectivityCheckHandle *c_handle, + NMConnectivityState state, + GError *error, + gpointer user_data) +{ + gs_unref_object NMDevice *self = NULL; + NMDevicePrivate *priv; + NMDeviceConnectivityHandle *handle; + NMDeviceConnectivityHandle *other_handle; + gboolean handle_is_alive; guint64 seq; -} ConnectivityCheckData; -static void -concheck_done (ConnectivityCheckData *data) -{ - NMDevice *self = data->self; - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - - /* The unsolicited connectivity checks don't hook a callback. */ - if (data->callback) - data->callback (data->self, priv->connectivity_state, data->user_data); - g_object_unref (data->self); - g_slice_free (ConnectivityCheckData, data); -} - -#if WITH_CONCHECK -static void -concheck_cb (GObject *source_object, GAsyncResult *result, gpointer user_data) -{ - ConnectivityCheckData *data = user_data; - NMDevice *self = data->self; - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - NMConnectivity *connectivity = NM_CONNECTIVITY (source_object); - NMConnectivityState state; - GError *error = NULL; - - state = nm_connectivity_check_finish (connectivity, result, &error); - if (error) { - _LOGW (LOGD_DEVICE, "connectivity checking on '%s' failed: %s", - nm_device_get_iface (self), error->message); - g_error_free (error); + 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. */ + return; } - if (data->seq == priv->concheck_seq) - update_connectivity_state (data->self, state); - concheck_done (data); -} -#endif /* WITH_CONCHECK */ + /* we keep NMConnectivity instance alive. It cannot be disposing. */ + nm_assert (!nm_utils_error_is_cancelled (error, TRUE)); -static gboolean -no_concheck (gpointer user_data) -{ - ConnectivityCheckData *data = user_data; + handle = user_data; + nm_assert (handle->c_handle == c_handle); + handle->c_handle = NULL; - concheck_done (data); - return G_SOURCE_REMOVE; + /* keep @self alive, while we invoke callbacks. */ + self = g_object_ref (handle->self); + priv = NM_DEVICE_GET_PRIVATE (self); + + nm_assert (!handle || c_list_contains (&priv->concheck_lst_head, &handle->concheck_lst)); + + seq = handle->seq; + + /* first update the new state, and emit signals. */ + concheck_update_state (self, state); + + handle_is_alive = FALSE; + + /* we might have invoked callbacks during concheck_update_state(). The caller might have + * cancelled and thus destroyed @handle. We have to check whether handle is still alive, + * by searching it in the list of alive handles. + * + * Also, we might want to complete all pending callbacks that were started before + * @handle, as they are automatically obsoleted. */ +check_handles: + c_list_for_each_entry (other_handle, &priv->concheck_lst_head, concheck_lst) { + if (other_handle->seq >= seq) { + /* it's not guaranteed that @handle is still in the list. It might already + * be canceled while invoking callbacks for a previous other_handle. + * If it is already cancelled, @handle is a dangling pointer. + * + * Since @seq is assigned uniquely and increasing, either @other_handle is + * @handle (and thus, handle is alive), or it isn't. */ + if (other_handle == handle) + handle_is_alive = TRUE; + break; + } + + nm_assert (other_handle != handle); + + if (!NM_IN_SET (state, NM_CONNECTIVITY_ERROR)) { + /* we also want to complete handles that were started before the current + * @handle. Their response is out-dated. */ + concheck_handle_complete (other_handle, NULL); + + /* we invoked callbacks, other handles might be cancelled and removed from the list. + * Need to iterate the list from the start. */ + goto check_handles; + } + } + + if (!handle_is_alive) { + /* We didn't find @handle in the list of alive handles. Thus, the handles + * was cancelled while we were invoking events. Nothing to do, and don't + * touch the dangling pointer. */ + return; + } + + concheck_handle_complete (handle, NULL); } -void +NMDeviceConnectivityHandle * nm_device_check_connectivity (NMDevice *self, NMDeviceConnectivityCallback callback, gpointer user_data) { - ConnectivityCheckData *data; -#if WITH_CONCHECK - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); -#endif + static guint64 seq_counter = 0; + NMDevicePrivate *priv; + NMDeviceConnectivityHandle *handle; - data = g_slice_new0 (ConnectivityCheckData); - data->self = g_object_ref (self); - data->callback = callback; - data->user_data = user_data; + g_return_val_if_fail (NM_IS_DEVICE (self), NULL); -#if WITH_CONCHECK - if (priv->concheck_periodic_id) { - data->seq = ++priv->concheck_seq; + priv = NM_DEVICE_GET_PRIVATE (self); - /* Kick off a real connectivity check. */ - nm_connectivity_check_async (nm_connectivity_get (), - nm_device_get_ip_iface (self), - concheck_cb, - data); - return; - } -#endif + handle = g_slice_new0 (NMDeviceConnectivityHandle); + handle->seq = ++seq_counter; + handle->self = self; + handle->callback = callback; + handle->user_data = user_data; + c_list_link_tail (&priv->concheck_lst_head, &handle->concheck_lst); - /* Fake one. */ - g_idle_add (no_concheck, data); + handle->c_handle = nm_connectivity_check_start (concheck_get_mgr (self), + nm_device_get_ip_iface (self), + concheck_cb, + handle); + return handle; +} + +void +nm_device_check_connectivity_cancel (NMDeviceConnectivityHandle *handle) +{ + gs_free_error GError *cancelled_error = NULL; + + g_return_if_fail (handle); + g_return_if_fail (NM_IS_DEVICE (handle->self)); + g_return_if_fail (!c_list_is_empty (&handle->concheck_lst)); + + nm_utils_error_set_cancelled (&cancelled_error, FALSE, "NMDevice"); + concheck_handle_complete (handle, cancelled_error); } NMConnectivityState @@ -2295,43 +2409,6 @@ nm_device_get_connectivity_state (NMDevice *self) return NM_DEVICE_GET_PRIVATE (self)->connectivity_state; } -#if WITH_CONCHECK -static void -concheck_periodic (NMConnectivity *connectivity, NMDevice *self) -{ - nm_device_check_connectivity (self, NULL, NULL); -} -#endif - -static void -concheck_periodic_update (NMDevice *self) -{ -#if WITH_CONCHECK - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - gboolean check_enable; - - check_enable = (priv->state == NM_DEVICE_STATE_ACTIVATED) - && nm_device_get_best_default_route (self, AF_UNSPEC); - - if (check_enable && !priv->concheck_periodic_id) { - /* We just gained a default route. Enable periodic checking. */ - priv->concheck_periodic_id = g_signal_connect (nm_connectivity_get (), - NM_CONNECTIVITY_PERIODIC_CHECK, - G_CALLBACK (concheck_periodic), self); - /* Also kick off a check right away. */ - nm_device_check_connectivity (self, NULL, NULL); - } else if (!check_enable && priv->concheck_periodic_id) { - /* The default route has gone off, and so has connectivity. */ - nm_clear_g_signal_handler (nm_connectivity_get (), &priv->concheck_periodic_id); - update_connectivity_state (self, NM_CONNECTIVITY_NONE); - } -#else - /* update_connectivity_state() figures out how to lie about - * connectivity state if the actual state is not really known. */ - update_connectivity_state (self, NM_CONNECTIVITY_UNKNOWN); -#endif -} - /*****************************************************************************/ static SlaveInfo * @@ -14429,10 +14506,12 @@ nm_device_init (NMDevice *self) self->_priv = priv; + c_list_init (&priv->concheck_lst_head); c_list_init (&self->devices_lst); - c_list_init (&priv->slaves); + priv->connectivity_state = NM_CONNECTIVITY_UNKNOWN; + priv->netns = g_object_ref (NM_NETNS_GET); priv->autoconnect_blocked_flags = DEFAULT_AUTOCONNECT @@ -14546,11 +14625,19 @@ dispose (GObject *object) NMDevice *self = NM_DEVICE (object); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); NMPlatform *platform; + NMDeviceConnectivityHandle *con_handle; + gs_free_error GError *cancelled_error = NULL; _LOGD (LOGD_DEVICE, "disposing"); nm_assert (c_list_is_empty (&self->devices_lst)); + while ((con_handle = c_list_first_entry (&priv->concheck_lst_head, NMDeviceConnectivityHandle, concheck_lst))) { + if (!cancelled_error) + nm_utils_error_set_cancelled (&cancelled_error, FALSE, "NMDevice"); + concheck_handle_complete (con_handle, cancelled_error); + } + nm_clear_g_cancellable (&priv->deactivating_cancellable); nm_device_assume_state_reset (self); @@ -14624,6 +14711,8 @@ dispose (GObject *object) g_clear_object (&priv->lldp_listener); } + nm_clear_g_signal_handler (priv->concheck_mgr, &priv->concheck_periodic_id); + G_OBJECT_CLASS (nm_device_parent_class)->dispose (object); if (nm_clear_g_source (&priv->queued_state.id)) { @@ -14665,8 +14754,9 @@ finalize (GObject *object) /* for testing, NMDeviceTest does not invoke NMDevice::constructed, * and thus @settings might be unset. */ - if (priv->settings) - g_object_unref (priv->settings); + nm_g_object_unref (priv->settings); + + nm_g_object_unref (priv->concheck_mgr); g_object_unref (priv->netns); } diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index f783cffbf3..24d184657f 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -775,12 +775,20 @@ gboolean nm_device_hw_addr_get_cloned (NMDevice *self, gboolean *preserve, GError **error); +typedef struct _NMDeviceConnectivityHandle NMDeviceConnectivityHandle; + typedef void (*NMDeviceConnectivityCallback) (NMDevice *self, + NMDeviceConnectivityHandle *handle, NMConnectivityState state, + GError *error, gpointer user_data); -void nm_device_check_connectivity (NMDevice *self, - NMDeviceConnectivityCallback callback, - gpointer user_data); + +NMDeviceConnectivityHandle *nm_device_check_connectivity (NMDevice *self, + NMDeviceConnectivityCallback callback, + gpointer user_data); + +void nm_device_check_connectivity_cancel (NMDeviceConnectivityHandle *handle); + NMConnectivityState nm_device_get_connectivity_state (NMDevice *self); typedef struct _NMBtVTableNetworkServer NMBtVTableNetworkServer; diff --git a/src/main.c b/src/main.c index 45704945ff..69902c8437 100644 --- a/src/main.c +++ b/src/main.c @@ -402,9 +402,7 @@ main (int argc, char *argv[]) manager)) goto done; -#if WITH_CONCHECK NM_UTILS_KEEP_ALIVE (manager, nm_connectivity_get (), "NMManager-depends-on-NMConnectivity"); -#endif nm_dispatcher_init (); diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 52e40d6031..dd11bdc0f7 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -30,34 +30,55 @@ #include #endif +#include "nm-utils/c-list.h" #include "nm-config.h" #include "NetworkManagerUtils.h" +#define HEADER_STATUS_ONLINE "X-NetworkManager-Status: online\r\n" + /*****************************************************************************/ -NM_UTILS_LOOKUP_STR_DEFINE (nm_connectivity_state_to_string, NMConnectivityState, +NM_UTILS_LOOKUP_STR_DEFINE_STATIC (_state_to_string, int /*NMConnectivityState*/, NM_UTILS_LOOKUP_DEFAULT_WARN ("???"), NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_UNKNOWN, "UNKNOWN"), NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_NONE, "NONE"), NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_LIMITED, "LIMITED"), NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_PORTAL, "PORTAL"), NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_FULL, "FULL"), + + NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_ERROR, "ERROR"), + NM_UTILS_LOOKUP_STR_ITEM (NM_CONNECTIVITY_FAKE, "FAKE"), ); +const char * +nm_connectivity_state_to_string (NMConnectivityState state) +{ + return _state_to_string (state); +} + /*****************************************************************************/ -#if WITH_CONCHECK +struct _NMConnectivityCheckHandle { + CList handles_lst; + NMConnectivity *self; + NMConnectivityCheckCallback callback; + gpointer user_data; -typedef struct { - GSimpleAsyncResult *simple; - char *response; - CURL *curl_ehandle; - size_t msg_size; - char *msg; - struct curl_slist *request_headers; - guint timeout_id; char *ifspec; -} NMConnectivityCheckHandle; + +#if WITH_CONCHECK + struct { + char *response; + + CURL *curl_ehandle; + struct curl_slist *request_headers; + + GString *recv_msg; + } concheck; +#endif + + guint timeout_id; +}; enum { PERIODIC_CHECK, @@ -68,14 +89,19 @@ enum { static guint signals[LAST_SIGNAL] = { 0 }; typedef struct { + CList handles_lst_head; char *uri; char *response; gboolean enabled; guint interval; NMConfig *config; - guint periodic_check_id; - CURLM *curl_mhandle; - guint curl_timer; +#if WITH_CONCHECK + struct { + CURLM *curl_mhandle; + guint curl_timer; + guint periodic_check_id; + } concheck; +#endif } NMConnectivityPrivate; struct _NMConnectivity { @@ -105,111 +131,172 @@ NM_DEFINE_SINGLETON_GETTER (NMConnectivity, nm_connectivity_get, NM_TYPE_CONNECT \ if (nm_logging_enabled (__level, _NMLOG2_DOMAIN)) { \ _nm_log (__level, _NMLOG2_DOMAIN, 0, \ - &cb_data->ifspec[3], NULL, \ - "connectivity: (%s) " \ - _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ - &cb_data->ifspec[3] \ - _NM_UTILS_MACRO_REST (__VA_ARGS__)); \ + (cb_data->ifspec ? &cb_data->ifspec[3] : NULL), \ + NULL, \ + "connectivity: (%s) " \ + _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ + (cb_data->ifspec ? &cb_data->ifspec[3] : "") \ + _NM_UTILS_MACRO_REST (__VA_ARGS__)); \ } \ } G_STMT_END /*****************************************************************************/ static void -finish_cb_data (NMConnectivityCheckHandle *cb_data, NMConnectivityState new_state) +cb_data_invoke_callback (NMConnectivityCheckHandle *cb_data, + NMConnectivityState state, + GError *error, + const char *log_message) { - /* Contrary to what cURL manual claim it is *not* safe to remove - * the easy handle "at any moment"; specifically not from the - * write function. Thus here we just dissociate the cb_data from - * the easy handle and the easy handle will be cleaned up when the - * message goes to CURLMSG_DONE in curl_check_connectivity(). */ - curl_easy_setopt (cb_data->curl_ehandle, CURLOPT_PRIVATE, NULL); + NMConnectivityCheckCallback callback; - g_simple_async_result_set_op_res_gssize (cb_data->simple, new_state); - g_simple_async_result_complete (cb_data->simple); - g_object_unref (cb_data->simple); - curl_slist_free_all (cb_data->request_headers); - g_free (cb_data->response); - g_free (cb_data->msg); - g_free (cb_data->ifspec); - g_source_remove (cb_data->timeout_id); - g_slice_free (NMConnectivityCheckHandle, cb_data); + nm_assert (cb_data); + nm_assert (NM_IS_CONNECTIVITY (cb_data->self)); + + callback = cb_data->callback; + if (!callback) + return; + + cb_data->callback = NULL; + + nm_assert (log_message); + + _LOG2D ("check completed: %s; %s", + nm_connectivity_state_to_string (state), + log_message); + + callback (cb_data->self, + cb_data, + state, + error, + cb_data->user_data); } static void -curl_check_connectivity (CURLM *mhandle, CURLMcode ret) +cb_data_free (NMConnectivityCheckHandle *cb_data, + NMConnectivityState state, + GError *error, + const char *log_message) +{ + NMConnectivity *self; + + nm_assert (cb_data); + + self = cb_data->self; + + nm_assert (NM_IS_CONNECTIVITY (self)); + + c_list_unlink (&cb_data->handles_lst); + +#if WITH_CONCHECK + if (cb_data->concheck.curl_ehandle) { + NMConnectivityPrivate *priv; + + /* Contrary to what cURL manual claim it is *not* safe to remove + * the easy handle "at any moment"; specifically not from the + * write function. Thus here we just dissociate the cb_data from + * the easy handle and the easy handle will be cleaned up when the + * message goes to CURLMSG_DONE in curl_check_connectivity(). */ + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_WRITEFUNCTION, NULL); + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_WRITEDATA, NULL); + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_HEADERFUNCTION, NULL); + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_HEADERDATA, NULL); + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_PRIVATE, NULL); + curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_HTTPHEADER, NULL); + + priv = NM_CONNECTIVITY_GET_PRIVATE (self); + + curl_multi_remove_handle (priv->concheck.curl_mhandle, cb_data->concheck.curl_ehandle); + curl_easy_cleanup (cb_data->concheck.curl_ehandle); + + curl_slist_free_all (cb_data->concheck.request_headers); + } +#endif + + nm_clear_g_source (&cb_data->timeout_id); + + cb_data_invoke_callback (cb_data, state, error, log_message); + +#if WITH_CONCHECK + g_free (cb_data->concheck.response); + if (cb_data->concheck.recv_msg) + g_string_free (cb_data->concheck.recv_msg, TRUE); +#endif + g_free (cb_data->ifspec); + g_slice_free (NMConnectivityCheckHandle, cb_data); +} + +/*****************************************************************************/ + +#if WITH_CONCHECK +static const char * +_check_handle_get_response (NMConnectivityCheckHandle *cb_data) +{ + return cb_data->concheck.response ?: NM_CONFIG_DEFAULT_CONNECTIVITY_RESPONSE; +} + +static void +curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) { NMConnectivityCheckHandle *cb_data; CURLMsg *msg; CURLcode eret; - CURL *easy_handle; gint m_left; long response_code; + CURLMcode ret; + int running_handles; + ret = curl_multi_socket_action (mhandle, sockfd, ev_bitmask, &running_handles); if (ret != CURLM_OK) - _LOGW ("connectivity check failed"); + _LOGE ("connectivity check failed: %d", ret); while ((msg = curl_multi_info_read (mhandle, &m_left))) { + if (msg->msg != CURLMSG_DONE) continue; /* Here we have completed a session. Check easy session result. */ eret = curl_easy_getinfo (msg->easy_handle, CURLINFO_PRIVATE, (char **) &cb_data); if (eret != CURLE_OK) { - _LOG2E ("curl cannot extract cb_data for easy handle %p, skipping msg", msg->easy_handle); + _LOGE ("curl cannot extract cb_data for easy handle, skipping msg"); continue; } - if (cb_data) { - NMConnectivityState c; + if (!cb_data->callback) { + /* callback was already invoked earlier. */ + cb_data_free (cb_data, NM_CONNECTIVITY_UNKNOWN, NULL, NULL); + } else if (msg->data.result != CURLE_OK) { + gs_free char *log_message = NULL; - /* If cb_data is still there this message hasn't been - * taken care of. Do so now. */ - if (msg->data.result != CURLE_OK) { - _LOG2D ("check failed (%d)", msg->data.result); - c = NM_CONNECTIVITY_LIMITED; - } else if ( !cb_data->response[0] - && (curl_easy_getinfo (msg->easy_handle, CURLINFO_RESPONSE_CODE, &response_code) == CURLE_OK) - && response_code == 204) { - /* If we got a 204 response code (no content) and we actually - * requested no content, report full connectivity. */ - _LOG2D ("response with no content received, check successful"); - c = NM_CONNECTIVITY_FULL; - } else { - /* If we get here, it means that easy_write_cb() didn't read enough - * bytes to be able to do a match, or that we were asking for no content - * (204 response code) and we actually got some. Either way, that is - * an indication of a captive portal */ - _LOG2I ("response did not match expected response '%s'; assuming captive portal.", - cb_data->response); - c = NM_CONNECTIVITY_PORTAL; - } - - finish_cb_data (cb_data, c); + log_message = g_strdup_printf ("check failed with curl status %d", msg->data.result); + cb_data_free (cb_data, NM_CONNECTIVITY_LIMITED, NULL, + log_message); + } else if ( !((_check_handle_get_response (cb_data))[0]) + && (curl_easy_getinfo (msg->easy_handle, CURLINFO_RESPONSE_CODE, &response_code) == CURLE_OK) + && response_code == 204) { + /* If we got a 204 response code (no content) and we actually + * requested no content, report full connectivity. */ + cb_data_free (cb_data, NM_CONNECTIVITY_FULL, NULL, + "no content, as expected"); + } else { + /* If we get here, it means that easy_write_cb() didn't read enough + * bytes to be able to do a match, or that we were asking for no content + * (204 response code) and we actually got some. Either way, that is + * an indication of a captive portal */ + cb_data_free (cb_data, NM_CONNECTIVITY_PORTAL, NULL, + "unexpected short response"); } - - /* Do not use message data after calling curl_multi_remove_handle() */ - easy_handle = msg->easy_handle; - curl_multi_remove_handle (mhandle, easy_handle); - curl_easy_cleanup (easy_handle); } } static gboolean curl_timeout_cb (gpointer user_data) { - NMConnectivity *self = NM_CONNECTIVITY (user_data); + gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data)); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CURLMcode ret; - int pending_conn; - - priv->curl_timer = 0; - - ret = curl_multi_socket_action (priv->curl_mhandle, CURL_SOCKET_TIMEOUT, 0, &pending_conn); - _LOGT ("timeout elapsed - multi_socket_action (%d conn remaining)", pending_conn); - - curl_check_connectivity (priv->curl_mhandle, ret); + priv->concheck.curl_timer = 0; + curl_check_connectivity (priv->concheck.curl_mhandle, CURL_SOCKET_TIMEOUT, 0); return G_SOURCE_REMOVE; } @@ -219,21 +306,17 @@ multi_timer_cb (CURLM *multi, long timeout_ms, void *userdata) NMConnectivity *self = NM_CONNECTIVITY (userdata); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - nm_clear_g_source (&priv->curl_timer); + nm_clear_g_source (&priv->concheck.curl_timer); if (timeout_ms != -1) - priv->curl_timer = g_timeout_add (timeout_ms, curl_timeout_cb, self); - + priv->concheck.curl_timer = g_timeout_add (timeout_ms, curl_timeout_cb, self); return 0; } static gboolean -curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer data) +curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data) { - NMConnectivity *self = NM_CONNECTIVITY (data); + gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data)); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CURLMcode ret; - int pending_conn = 0; - gboolean bret = TRUE; int fd = g_io_channel_unix_get_fd (ch); int action = 0; @@ -241,16 +324,11 @@ curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer data) action |= CURL_CSELECT_IN; if (condition & G_IO_OUT) action |= CURL_CSELECT_OUT; + if (condition & G_IO_ERR) + action |= CURL_CSELECT_ERR; - ret = curl_multi_socket_action (priv->curl_mhandle, fd, 0, &pending_conn); - - curl_check_connectivity (priv->curl_mhandle, ret); - - if (pending_conn == 0) { - nm_clear_g_source (&priv->curl_timer); - bret = FALSE; - } - return bret; + curl_check_connectivity (priv->concheck.curl_mhandle, fd, action); + return G_SOURCE_CONTINUE; } typedef struct { @@ -263,11 +341,12 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void { NMConnectivity *self = NM_CONNECTIVITY (userdata); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CurlSockData *fdp = (CurlSockData *) socketp; + CurlSockData *fdp = socketp; GIOCondition condition = 0; if (what == CURL_POLL_REMOVE) { if (fdp) { + curl_multi_assign (priv->concheck.curl_mhandle, s, NULL); nm_clear_g_source (&fdp->ev); g_io_channel_unref (fdp->ch); g_slice_free (CurlSockData, fdp); @@ -276,6 +355,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void if (!fdp) { fdp = g_slice_new0 (CurlSockData); fdp->ch = g_io_channel_unix_new (s); + curl_multi_assign (priv->concheck.curl_mhandle, s, fdp); } else nm_clear_g_source (&fdp->ev); @@ -283,19 +363,16 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void condition = G_IO_IN; else if (what == CURL_POLL_OUT) condition = G_IO_OUT; - else if (condition == CURL_POLL_INOUT) + else if (what == CURL_POLL_INOUT) condition = G_IO_IN | G_IO_OUT; if (condition) fdp->ev = g_io_add_watch (fdp->ch, condition, curl_socketevent_cb, self); - curl_multi_assign (priv->curl_mhandle, s, fdp); } return CURLM_OK; } -#define HEADER_STATUS_ONLINE "X-NetworkManager-Status: online\r\n" - static size_t easy_header_cb (char *buffer, size_t size, size_t nitems, void *userdata) { @@ -304,8 +381,8 @@ easy_header_cb (char *buffer, size_t size, size_t nitems, void *userdata) if ( len >= sizeof (HEADER_STATUS_ONLINE) - 1 && !g_ascii_strncasecmp (buffer, HEADER_STATUS_ONLINE, sizeof (HEADER_STATUS_ONLINE) - 1)) { - _LOG2D ("status header found, check successful"); - finish_cb_data (cb_data, NM_CONNECTIVITY_FULL); + cb_data_invoke_callback (cb_data, NM_CONNECTIVITY_FULL, + NULL, "status header found"); return 0; } @@ -317,23 +394,24 @@ easy_write_cb (void *buffer, size_t size, size_t nmemb, void *userdata) { NMConnectivityCheckHandle *cb_data = userdata; size_t len = size * nmemb; + const char *response = _check_handle_get_response (cb_data);; - cb_data->msg = g_realloc (cb_data->msg, cb_data->msg_size + len); - memcpy (cb_data->msg + cb_data->msg_size, buffer, len); - cb_data->msg_size += len; + if (!cb_data->concheck.recv_msg) + cb_data->concheck.recv_msg = g_string_sized_new (len + 10); - /* Check matching prefix if a expected response is given */ - if ( cb_data->response[0] - && cb_data->msg_size >= strlen (cb_data->response)) { + g_string_append_len (cb_data->concheck.recv_msg, buffer, len); + + if ( response + && cb_data->concheck.recv_msg->len >= strlen (response)) { /* We already have enough data -- check response */ - if (g_str_has_prefix (cb_data->msg, cb_data->response)) { - _LOG2D ("check successful."); - finish_cb_data (cb_data, NM_CONNECTIVITY_FULL); + if (g_str_has_prefix (cb_data->concheck.recv_msg->str, response)) { + cb_data_invoke_callback (cb_data, NM_CONNECTIVITY_FULL, NULL, + "expected response"); } else { - _LOG2I ("response did not match expected response '%s'; assuming captive portal.", - cb_data->response); - finish_cb_data (cb_data, NM_CONNECTIVITY_PORTAL); + cb_data_invoke_callback (cb_data, NM_CONNECTIVITY_PORTAL, NULL, + "unexpected response"); } + return 0; } @@ -341,90 +419,123 @@ easy_write_cb (void *buffer, size_t size, size_t nmemb, void *userdata) } static gboolean -timeout_cb (gpointer user_data) +_timeout_cb (gpointer user_data) { NMConnectivityCheckHandle *cb_data = user_data; - NMConnectivity *self = NM_CONNECTIVITY (g_async_result_get_source_object (G_ASYNC_RESULT (cb_data->simple))); - NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CURL *ehandle = cb_data->curl_ehandle; + NMConnectivity *self; - _LOG2I ("timed out"); - finish_cb_data (cb_data, NM_CONNECTIVITY_LIMITED); - curl_multi_remove_handle (priv->curl_mhandle, ehandle); - curl_easy_cleanup (ehandle); + nm_assert (NM_IS_CONNECTIVITY (cb_data->self)); + self = cb_data->self; + + nm_assert (c_list_contains (&NM_CONNECTIVITY_GET_PRIVATE (self)->handles_lst_head, &cb_data->handles_lst)); + + cb_data_free (cb_data, NM_CONNECTIVITY_LIMITED, NULL, "timeout"); + return G_SOURCE_REMOVE; +} +#endif + +static gboolean +_idle_cb (gpointer user_data) +{ + NMConnectivityCheckHandle *cb_data = user_data; + + nm_assert (NM_IS_CONNECTIVITY (cb_data->self)); + nm_assert (c_list_contains (&NM_CONNECTIVITY_GET_PRIVATE (cb_data->self)->handles_lst_head, &cb_data->handles_lst)); + + cb_data->timeout_id = 0; + if (!cb_data->ifspec) { + gs_free_error GError *error = NULL; + + /* the invocation was with an invalid ifname. It is a fail. */ + g_set_error (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, + "no interface specified for connectivity check"); + cb_data_free (cb_data, NM_CONNECTIVITY_ERROR, NULL, "missing interface"); + } else + cb_data_free (cb_data, NM_CONNECTIVITY_FAKE, NULL, "fake result"); return G_SOURCE_REMOVE; } -void -nm_connectivity_check_async (NMConnectivity *self, - const char *iface, - GAsyncReadyCallback callback, - gpointer user_data) +NMConnectivityCheckHandle * +nm_connectivity_check_start (NMConnectivity *self, + const char *iface, + NMConnectivityCheckCallback callback, + gpointer user_data) { NMConnectivityPrivate *priv; - GSimpleAsyncResult *simple; - CURL *ehandle = NULL; + NMConnectivityCheckHandle *cb_data; + + g_return_val_if_fail (NM_IS_CONNECTIVITY (self), NULL); + g_return_val_if_fail (!iface || iface[0], NULL); + g_return_val_if_fail (callback, NULL); - g_return_if_fail (NM_IS_CONNECTIVITY (self)); priv = NM_CONNECTIVITY_GET_PRIVATE (self); - simple = g_simple_async_result_new (G_OBJECT (self), callback, user_data, - nm_connectivity_check_async); + cb_data = g_slice_new0 (NMConnectivityCheckHandle); + cb_data->self = self; + c_list_link_tail (&priv->handles_lst_head, &cb_data->handles_lst); + cb_data->callback = callback; + cb_data->user_data = user_data; - if (priv->enabled) - ehandle = curl_easy_init (); - - if (ehandle) { - NMConnectivityCheckHandle *cb_data = g_slice_new0 (NMConnectivityCheckHandle); - - cb_data->curl_ehandle = ehandle; - cb_data->request_headers = curl_slist_append (NULL, "Connection: close"); + if (iface) cb_data->ifspec = g_strdup_printf ("if!%s", iface); - cb_data->simple = simple; - if (priv->response) - cb_data->response = g_strdup (priv->response); - else - cb_data->response = g_strdup (NM_CONFIG_DEFAULT_CONNECTIVITY_RESPONSE); - curl_easy_setopt (ehandle, CURLOPT_URL, priv->uri); - curl_easy_setopt (ehandle, CURLOPT_WRITEFUNCTION, easy_write_cb); - curl_easy_setopt (ehandle, CURLOPT_WRITEDATA, cb_data); - curl_easy_setopt (ehandle, CURLOPT_HEADERFUNCTION, easy_header_cb); - curl_easy_setopt (ehandle, CURLOPT_HEADERDATA, cb_data); - curl_easy_setopt (ehandle, CURLOPT_PRIVATE, cb_data); - curl_easy_setopt (ehandle, CURLOPT_HTTPHEADER, cb_data->request_headers); - curl_easy_setopt (ehandle, CURLOPT_INTERFACE, cb_data->ifspec); - curl_multi_add_handle (priv->curl_mhandle, ehandle); +#if WITH_CONCHECK + if (iface) { + CURL *ehandle; - cb_data->timeout_id = g_timeout_add_seconds (30, timeout_cb, cb_data); + if ( priv->enabled + && (ehandle = curl_easy_init ())) { - _LOG2D ("sending request to '%s'", priv->uri); - return; - } else { - _LOGD ("(%s) faking request. Connectivity check disabled", iface); + cb_data->concheck.response = g_strdup (priv->response); + cb_data->concheck.curl_ehandle = ehandle; + cb_data->concheck.request_headers = curl_slist_append (NULL, "Connection: close"); + curl_easy_setopt (ehandle, CURLOPT_URL, priv->uri); + curl_easy_setopt (ehandle, CURLOPT_WRITEFUNCTION, easy_write_cb); + curl_easy_setopt (ehandle, CURLOPT_WRITEDATA, cb_data); + curl_easy_setopt (ehandle, CURLOPT_HEADERFUNCTION, easy_header_cb); + curl_easy_setopt (ehandle, CURLOPT_HEADERDATA, cb_data); + curl_easy_setopt (ehandle, CURLOPT_PRIVATE, cb_data); + curl_easy_setopt (ehandle, CURLOPT_HTTPHEADER, cb_data->concheck.request_headers); + curl_easy_setopt (ehandle, CURLOPT_INTERFACE, cb_data->ifspec); + curl_multi_add_handle (priv->concheck.curl_mhandle, ehandle); + + cb_data->timeout_id = g_timeout_add_seconds (30, _timeout_cb, cb_data); + + _LOG2D ("start request to '%s'", priv->uri); + return cb_data; + } } +#endif - g_simple_async_result_set_op_res_gssize (simple, NM_CONNECTIVITY_UNKNOWN); - g_simple_async_result_complete_in_idle (simple); - g_object_unref (simple); + _LOG2D ("start fake request"); + cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); + return cb_data; } -NMConnectivityState -nm_connectivity_check_finish (NMConnectivity *self, - GAsyncResult *result, - GError **error) +void +nm_connectivity_check_cancel (NMConnectivityCheckHandle *cb_data) { - GSimpleAsyncResult *simple; + NMConnectivity *self; + gs_free_error GError *error = NULL; - g_return_val_if_fail (g_simple_async_result_is_valid (result, G_OBJECT (self), nm_connectivity_check_async), NM_CONNECTIVITY_UNKNOWN); + g_return_if_fail (cb_data); - simple = G_SIMPLE_ASYNC_RESULT (result); - if (g_simple_async_result_propagate_error (simple, error)) - return NM_CONNECTIVITY_UNKNOWN; - return (NMConnectivityState) g_simple_async_result_get_op_res_gssize (simple); + self = cb_data->self; + + g_return_if_fail (NM_IS_CONNECTIVITY (self)); + g_return_if_fail (!c_list_is_empty (&cb_data->handles_lst)); + g_return_if_fail (cb_data->callback); + + nm_assert (c_list_contains (&NM_CONNECTIVITY_GET_PRIVATE (self)->handles_lst_head, &cb_data->handles_lst)); + + nm_utils_error_set_cancelled (&error, FALSE, "NMConnectivity"); + + cb_data_free (cb_data, NM_CONNECTIVITY_ERROR, error, "cancelled"); } +/*****************************************************************************/ + gboolean nm_connectivity_check_enabled (NMConnectivity *self) { @@ -435,12 +546,14 @@ nm_connectivity_check_enabled (NMConnectivity *self) /*****************************************************************************/ +#if WITH_CONCHECK static gboolean periodic_check (gpointer user_data) { g_signal_emit (NM_CONNECTIVITY (user_data), signals[PERIODIC_CHECK], 0); return G_SOURCE_CONTINUE; } +#endif static void update_config (NMConnectivity *self, NMConfigData *config_data) @@ -484,13 +597,16 @@ update_config (NMConnectivity *self, NMConfigData *config_data) changed = TRUE; } - /* Set enabled flag. */ - enabled = nm_config_data_get_connectivity_enabled (config_data); + enabled = FALSE; +#if WITH_CONCHECK /* connectivity checking also requires a valid URI, interval and * curl_mhandle */ - if (!(priv->uri && priv->interval && priv->curl_mhandle)) { - enabled = FALSE; - } + if ( priv->uri + && priv->interval + && priv->concheck.curl_mhandle) + enabled = nm_config_data_get_connectivity_enabled (config_data); +#endif + if (priv->enabled != enabled) { priv->enabled = enabled; changed = TRUE; @@ -498,7 +614,7 @@ update_config (NMConnectivity *self, NMConfigData *config_data) /* Set the response. */ response = nm_config_data_get_connectivity_response (config_data); - if (g_strcmp0 (response, priv->response) != 0) { + if (!nm_streq0 (response, priv->response)) { /* a response %NULL means, NM_CONFIG_DEFAULT_CONNECTIVITY_RESPONSE. Any other response * (including "") is accepted. */ g_free (priv->response); @@ -506,11 +622,13 @@ update_config (NMConnectivity *self, NMConfigData *config_data) changed = TRUE; } +#if WITH_CONCHECK if (changed) { - nm_clear_g_source (&priv->periodic_check_id); + nm_clear_g_source (&priv->concheck.periodic_check_id); if (nm_connectivity_check_enabled (self)) - priv->periodic_check_id = g_timeout_add_seconds (priv->interval, periodic_check, self); + priv->concheck.periodic_check_id = g_timeout_add_seconds (priv->interval, periodic_check, self); } +#endif } static void @@ -523,34 +641,37 @@ config_changed_cb (NMConfig *config, update_config (self, config_data); } +/*****************************************************************************/ + static void nm_connectivity_init (NMConnectivity *self) { NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CURLcode retv; - retv = curl_global_init (CURL_GLOBAL_ALL); - if (retv == CURLE_OK) - priv->curl_mhandle = curl_multi_init (); - - if (!priv->curl_mhandle) - _LOGE ("unable to init cURL, connectivity check will not work"); - else { - curl_multi_setopt (priv->curl_mhandle, CURLMOPT_SOCKETFUNCTION, multi_socket_cb); - curl_multi_setopt (priv->curl_mhandle, CURLMOPT_SOCKETDATA, self); - curl_multi_setopt (priv->curl_mhandle, CURLMOPT_TIMERFUNCTION, multi_timer_cb); - curl_multi_setopt (priv->curl_mhandle, CURLMOPT_TIMERDATA, self); - curl_multi_setopt (priv->curl_mhandle, CURLOPT_VERBOSE, 1); - } + c_list_init (&priv->handles_lst_head); priv->config = g_object_ref (nm_config_get ()); - - update_config (self, nm_config_get_data (priv->config)); g_signal_connect (G_OBJECT (priv->config), NM_CONFIG_SIGNAL_CONFIG_CHANGED, G_CALLBACK (config_changed_cb), self); +#if WITH_CONCHECK + if (curl_global_init (CURL_GLOBAL_ALL) == CURLE_OK) + priv->concheck.curl_mhandle = curl_multi_init (); + + if (!priv->concheck.curl_mhandle) + _LOGE ("unable to init cURL, connectivity check will not work"); + else { + curl_multi_setopt (priv->concheck.curl_mhandle, CURLMOPT_SOCKETFUNCTION, multi_socket_cb); + curl_multi_setopt (priv->concheck.curl_mhandle, CURLMOPT_SOCKETDATA, self); + curl_multi_setopt (priv->concheck.curl_mhandle, CURLMOPT_TIMERFUNCTION, multi_timer_cb); + curl_multi_setopt (priv->concheck.curl_mhandle, CURLMOPT_TIMERDATA, self); + curl_multi_setopt (priv->concheck.curl_mhandle, CURLOPT_VERBOSE, 1); + } +#endif + + update_config (self, nm_config_get_data (priv->config)); } static void @@ -558,19 +679,34 @@ dispose (GObject *object) { NMConnectivity *self = NM_CONNECTIVITY (object); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); + NMConnectivityCheckHandle *cb_data; + GError *error = NULL; + +again: + c_list_for_each_entry (cb_data, &priv->handles_lst_head, handles_lst) { + if (!error) + nm_utils_error_set_cancelled (&error, TRUE, "NMConnectivity"); + cb_data_free (cb_data, NM_CONNECTIVITY_ERROR, error, "shutting down"); + goto again; + } + g_clear_error (&error); g_clear_pointer (&priv->uri, g_free); g_clear_pointer (&priv->response, g_free); +#if WITH_CONCHECK + nm_clear_g_source (&priv->concheck.curl_timer); + + curl_multi_cleanup (priv->concheck.curl_mhandle); + curl_global_cleanup (); + nm_clear_g_source (&priv->concheck.periodic_check_id); +#endif + if (priv->config) { g_signal_handlers_disconnect_by_func (priv->config, config_changed_cb, self); g_clear_object (&priv->config); } - curl_multi_cleanup (priv->curl_mhandle); - curl_global_cleanup (); - nm_clear_g_source (&priv->periodic_check_id); - G_OBJECT_CLASS (nm_connectivity_parent_class)->dispose (object); } @@ -588,5 +724,3 @@ nm_connectivity_class_init (NMConnectivityClass *klass) object_class->dispose = dispose; } - -#endif /* WITH_CONCHECK */ diff --git a/src/nm-connectivity.h b/src/nm-connectivity.h index d9a9f2338b..6c16982307 100644 --- a/src/nm-connectivity.h +++ b/src/nm-connectivity.h @@ -24,6 +24,9 @@ #include "nm-dbus-interface.h" +#define NM_CONNECTIVITY_ERROR ((NMConnectivityState) -1) +#define NM_CONNECTIVITY_FAKE ((NMConnectivityState) -2) + #define NM_TYPE_CONNECTIVITY (nm_connectivity_get_type ()) #define NM_CONNECTIVITY(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_CONNECTIVITY, NMConnectivity)) #define NM_CONNECTIVITY_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_CONNECTIVITY, NMConnectivityClass)) @@ -41,13 +44,21 @@ NMConnectivity *nm_connectivity_get (void); const char *nm_connectivity_state_to_string (NMConnectivityState state); -void nm_connectivity_check_async (NMConnectivity *self, - const char *iface, - GAsyncReadyCallback callback, - gpointer user_data); -NMConnectivityState nm_connectivity_check_finish (NMConnectivity *self, - GAsyncResult *result, - GError **error); gboolean nm_connectivity_check_enabled (NMConnectivity *self); +typedef struct _NMConnectivityCheckHandle NMConnectivityCheckHandle; + +typedef void (*NMConnectivityCheckCallback) (NMConnectivity *self, + NMConnectivityCheckHandle *handle, + NMConnectivityState state, + GError *error, + gpointer user_data); + +NMConnectivityCheckHandle *nm_connectivity_check_start (NMConnectivity *self, + const char *iface, + NMConnectivityCheckCallback callback, + gpointer user_data); + +void nm_connectivity_check_cancel (NMConnectivityCheckHandle *handle); + #endif /* __NETWORKMANAGER_CONNECTIVITY_H__ */ diff --git a/src/nm-manager.c b/src/nm-manager.c index 4bc5c5dd9a..e98b9fe03f 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2450,7 +2450,6 @@ device_realized (NMDevice *device, _emit_device_added_removed (self, device, nm_device_is_real (device)); } -#if WITH_CONCHECK static void device_connectivity_changed (NMDevice *device, GParamSpec *pspec, @@ -2478,7 +2477,6 @@ device_connectivity_changed (NMDevice *device, nm_dispatcher_call_connectivity (priv->connectivity_state, NULL, NULL, NULL); } } -#endif static void _device_realize_finish (NMManager *self, @@ -2588,11 +2586,9 @@ add_device (NMManager *self, NMDevice *device, GError **error) G_CALLBACK (device_realized), self); -#if WITH_CONCHECK g_signal_connect (device, "notify::" NM_DEVICE_CONNECTIVITY, G_CALLBACK (device_connectivity_changed), self); -#endif if (priv->startup) { g_signal_connect (device, "notify::" NM_DEVICE_HAS_PENDING_ACTION, @@ -5441,34 +5437,54 @@ impl_manager_get_logging (NMDBusObject *obj, } typedef struct { - guint remaining; + NMManager *self; GDBusMethodInvocation *context; - NMConnectivityState state; + guint remaining; } ConnectivityCheckData; static void -device_connectivity_done (NMDevice *device, NMConnectivityState state, gpointer user_data) +device_connectivity_done (NMDevice *device, + NMDeviceConnectivityHandle *handle, + NMConnectivityState state, + GError *error, + gpointer user_data) { ConnectivityCheckData *data = user_data; + NMManager *self; + NMManagerPrivate *priv; + + nm_assert (data); + nm_assert (data->remaining > 0); + nm_assert (NM_IS_MANAGER (data->self)); data->remaining--; - /* We check if the state is already FULL so that we can provide the - * response without waiting for slower devices that are not going to - * affect the overall state anyway. */ + self = data->self; + priv = NM_MANAGER_GET_PRIVATE (self); - if (data->state != NM_CONNECTIVITY_FULL) { - if (state > data->state) - data->state = state; - - if (data->state == NM_CONNECTIVITY_FULL || !data->remaining) { - g_dbus_method_invocation_return_value (data->context, - g_variant_new ("(u)", data->state)); - } + if ( data->context + && ( data->remaining == 0 + || ( state == NM_CONNECTIVITY_FULL + && priv->connectivity_state == NM_CONNECTIVITY_FULL))) { + /* despite having a @handle and @state returned by the requests, we always + * return the current connectivity_state. That is, because the connectivity_state + * and the answer to the connectivity check shall agree. + * + * However, if one of the requests (early) returns full connectivity and agrees with + * the accumulated connectivity state, we no longer have to wait. The result is set. + * + * This also works well, because NMDevice first emits change signals to it's own + * connectivity state, which is then taken into account for the accumulated global + * state. All this happens, before the callback is invoked. */ + g_dbus_method_invocation_return_value (g_steal_pointer (&data->context), + g_variant_new ("(u)", + (guint) priv->connectivity_state)); } - if (!data->remaining) + if (data->remaining == 0) { + g_object_unref (self); g_slice_free (ConnectivityCheckData, data); + } } static void @@ -5482,6 +5498,7 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, GError *error = NULL; NMAuthCallResult result; ConnectivityCheckData *data; + NMDevice *device; priv->auth_chains = g_slist_remove (priv->auth_chains, chain); @@ -5497,23 +5514,37 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to recheck connectivity"); - } else { - NMDevice *device; - - /* it's allowed */ - data = g_slice_new0 (ConnectivityCheckData); - data->context = context; - - c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { - data->remaining++; - nm_device_check_connectivity (device, - device_connectivity_done, - data); - } } - if (error) + if (error) { g_dbus_method_invocation_take_error (context, error); + goto out; + } + + data = g_slice_new (ConnectivityCheckData); + data->self = g_object_ref (self); + data->context = context; + data->remaining = 0; + + c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { + data->remaining++; + nm_device_check_connectivity (device, + device_connectivity_done, + data); + } + + if (data->remaining == 0) { + /* call the handler at least once. */ + data->remaining = 1; + device_connectivity_done (NULL, + NULL, + NM_CONNECTIVITY_UNKNOWN, + NULL, + data); + /* @data got destroyed. */ + } + +out: nm_auth_chain_unref (chain); } @@ -6559,7 +6590,6 @@ get_property (GObject *object, guint prop_id, const char *path; NMActiveConnection *ac; GPtrArray *ptrarr; - gboolean vbool; switch (prop_id) { case PROP_VERSION: @@ -6616,12 +6646,8 @@ get_property (GObject *object, guint prop_id, g_value_set_boolean (value, nm_config_data_get_connectivity_uri (config_data) != NULL); break; case PROP_CONNECTIVITY_CHECK_ENABLED: -#if WITH_CONCHECK - vbool = nm_connectivity_check_enabled (nm_connectivity_get ()); -#else - vbool = FALSE; -#endif - g_value_set_boolean (value, vbool); + g_value_set_boolean (value, + nm_connectivity_check_enabled (nm_connectivity_get ())); break; case PROP_PRIMARY_CONNECTION: nm_dbus_utils_g_value_set_object_path (value, priv->primary_connection); diff --git a/src/tests/config/test-config.c b/src/tests/config/test-config.c index a6b886ce43..5021575667 100644 --- a/src/tests/config/test-config.c +++ b/src/tests/config/test-config.c @@ -318,10 +318,10 @@ test_config_global_dns (void) g_object_unref (config); } -#if WITH_CONCHECK static void test_config_connectivity_check (void) { +#if WITH_CONCHECK const char *CONFIG_INTERN = BUILDDIR"/test-connectivity-check-intern.conf"; NMConfig *config; NMConnectivity *connectivity; @@ -351,8 +351,10 @@ test_config_connectivity_check (void) g_object_unref (config); g_assert (remove (CONFIG_INTERN) == 0); -} +#else + g_test_skip ("concheck disabled"); #endif +} static void test_config_no_auto_default (void) @@ -1048,9 +1050,7 @@ main (int argc, char **argv) g_test_add_func ("/config/set-values", test_config_set_values); g_test_add_func ("/config/global-dns", test_config_global_dns); -#if WITH_CONCHECK g_test_add_func ("/config/connectivity-check", test_config_connectivity_check); -#endif g_test_add_func ("/config/signal", test_config_signal); From e8e0ef6300526ab64eaba534c6b22b23631cf8a9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Feb 2018 16:51:24 +0100 Subject: [PATCH 3/5] connectivity: reduce timeout for connectivity checks to 20 seconds The main issue is that `nmcli networking connectivity check` uses nm_client_check_connectivity(), which has a timeout of 25 seconds. Using a timeout of 30 seconds server side, means that if the requests don't complete in time, the client side will time out and abort with a failure. That is not right. Fix that by using a shorter timeout server side. 20 seconds is still plenty for a small HTTP request. If the network takes longer than that, it's fair to call that LIMITED connectivity. --- src/nm-connectivity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index dd11bdc0f7..039ff71d05 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -500,7 +500,7 @@ nm_connectivity_check_start (NMConnectivity *self, curl_easy_setopt (ehandle, CURLOPT_INTERFACE, cb_data->ifspec); curl_multi_add_handle (priv->concheck.curl_mhandle, ehandle); - cb_data->timeout_id = g_timeout_add_seconds (30, _timeout_cb, cb_data); + cb_data->timeout_id = g_timeout_add_seconds (20, _timeout_cb, cb_data); _LOG2D ("start request to '%s'", priv->uri); return cb_data; From 0a62a0e9039e49e05dae6af9a96c381629bccd58 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Feb 2018 21:41:14 +0100 Subject: [PATCH 4/5] connectivity: schedule connectivity timers per-device and probe for short outages It might happen, that connectivitiy is lost only for a moment and returns soon after. Based on that assumption, when we loose connectivity we want to have a probe interval where we check for returning connectivity more frequently. For that, we handle tracking of the timeouts per-device. The intervall shall start with 1 seconds, and double the interval time until the full interval is reached. Actually, due to the implementation, it's unlikely that we already perform the second check 1 second later. That is because commonly the first check returns before the one second timeout is reached and bumps the interval to 2 seconds right away. Also, we go through extra lengths so that manual connectivity check delay the periodic checks. By being more smart about that, we can reduce the number of connectivity checks, but still keeping the promise to check at least within the requested interval. The complexity of book keeping the timeouts is remarkable. But I think it is worth the effort and we should try hard to - have a connectivity state as accurate as possible. Clearly, connectivity checking means that we probing, so being more intelligent about timeout and backoff timers can result in a better connectivity state. The connectivity state is important because we use it for the default-route penaly and the GUI indicates bad connectivity. - be intelligent about avoiding redundant connectivity checks. While we want to check often to get an accurate connectivity state, we also want to minimize the number of HTTP requests, in case the connectivity is established and suppossedly stable. Also, perform connectivity checks in every state of the device. Even if a device is disconnected, it still might have connectivity, for example if the user externally adds an IP address on an unmanaged device. https://bugzilla.gnome.org/show_bug.cgi?id=792240 --- src/devices/nm-device.c | 346 ++++++++++++++++++++++++++++++++++------ src/devices/nm-device.h | 2 + src/main.c | 2 - src/nm-connectivity.c | 33 ++-- src/nm-connectivity.h | 4 +- src/nm-manager.c | 51 +++++- 6 files changed, 355 insertions(+), 83 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 6142908a57..cc3df54ae8 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -164,6 +164,7 @@ struct _NMDeviceConnectivityHandle { gpointer user_data; NMConnectivityCheckHandle *c_handle; guint64 seq; + bool is_periodic:1; }; /*****************************************************************************/ @@ -543,7 +544,19 @@ typedef struct _NMDevicePrivate { NMConnectivity *concheck_mgr; - gulong concheck_periodic_id; + /* if periodic checks are enabled, this is the source id for the next check. */ + guint concheck_p_cur_id; + + /* the currently configured max periodic interval. */ + guint concheck_p_max_interval; + + /* the current interval. If we are probing, the interval might be lower + * then the configured max interval. */ + guint concheck_p_cur_interval; + + /* the timestamp, when we last scheduled the timer concheck_p_cur_id with current interval + * concheck_p_cur_interval. */ + gint64 concheck_p_cur_basetime_ns; NMConnectivityState connectivity_state; @@ -2185,17 +2198,225 @@ nm_device_get_physical_port_id (NMDevice *self) /*****************************************************************************/ -static void -concheck_update_state (NMDevice *self, NMConnectivityState state) +typedef enum { + CONCHECK_SCHEDULE_UPDATE_INTERVAL, + CONCHECK_SCHEDULE_CHECK_EXTERNAL, + CONCHECK_SCHEDULE_CHECK_PERIODIC, + CONCHECK_SCHEDULE_RETURNED_MIN, + CONCHECK_SCHEDULE_RETURNED_BUMP, + CONCHECK_SCHEDULE_RETURNED_MAX, +} ConcheckScheduleMode; + +static NMDeviceConnectivityHandle *concheck_start (NMDevice *self, + NMDeviceConnectivityCallback callback, + gpointer user_data, + gboolean is_periodic); + +static void concheck_periodic_schedule_set (NMDevice *self, + ConcheckScheduleMode mode); + +static gboolean +concheck_periodic_timeout_cb (gpointer user_data) +{ + NMDevice *self = user_data; + + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_CHECK_PERIODIC); + concheck_start (self, NULL, NULL, TRUE); + return G_SOURCE_CONTINUE; +} + +static gboolean +concheck_is_possible (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (state == NM_CONNECTIVITY_ERROR) + if ( !nm_device_is_real (self) + || NM_FLAGS_HAS (priv->unmanaged_flags, NM_UNMANAGED_LOOPBACK)) + return FALSE; + + /* we enable periodic checks for every device state (except UNKNOWN). Especially with + * unmanaged devices, it is interesting to know whether we have connectivity on that device. */ + if (priv->state == NM_DEVICE_STATE_UNKNOWN) + return FALSE; + + return TRUE; +} + +static gboolean +concheck_periodic_schedule_do (NMDevice *self, gint64 interval_ns) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + gboolean periodic_check_disabled = FALSE; + + /* we always cancel whatever was pending. */ + if (nm_clear_g_source (&priv->concheck_p_cur_id)) + periodic_check_disabled = TRUE; + + if (priv->concheck_p_max_interval == 0) { + /* periodic checks are disabled */ + goto out; + } + + nm_assert (interval_ns >= 0); + + if (!concheck_is_possible (self)) + goto out; + + _LOGT (LOGD_CONCHECK, "connectivity: periodic-check: %sscheduled in %u milliseconds (%u seconds interval)", + periodic_check_disabled ? "re-" : "", + (guint) (interval_ns / NM_UTILS_NS_PER_MSEC), + priv->concheck_p_cur_interval); + + nm_assert (priv->concheck_p_cur_interval > 0); + priv->concheck_p_cur_id = g_timeout_add (interval_ns / NM_UTILS_NS_PER_MSEC, + concheck_periodic_timeout_cb, + self); + return TRUE; +out: + if (periodic_check_disabled) + _LOGT (LOGD_CONCHECK, "connectivity: periodic-check: unscheduled"); + return FALSE; +} + +#define CONCHECK_P_PROBE_INTERVAL 1 + +static void +concheck_periodic_schedule_set (NMDevice *self, + ConcheckScheduleMode mode) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + gint64 new_expiry, cur_expiry, tdiff; + gint64 now_ns = 0; + + if (priv->concheck_p_max_interval == 0) { + /* periodic check is disabled. Nothing to do. */ + return; + } + + if (!priv->concheck_p_cur_id) { + /* we currently don't have a timeout scheduled. No need to reschedule + * another one... */ + if (mode == CONCHECK_SCHEDULE_UPDATE_INTERVAL) { + /* ... unless, we are initalizing. In this case, setup the current current + * interval and schedule a perform a check right away. */ + priv->concheck_p_cur_interval = NM_MIN (priv->concheck_p_max_interval, CONCHECK_P_PROBE_INTERVAL); + priv->concheck_p_cur_basetime_ns = nm_utils_get_monotonic_timestamp_ns_cached (&now_ns); + if (concheck_periodic_schedule_do (self, priv->concheck_p_cur_interval * NM_UTILS_NS_PER_SECOND)) + concheck_start (self, NULL, NULL, TRUE); + } + return; + } + + switch (mode) { + case CONCHECK_SCHEDULE_UPDATE_INTERVAL: + /* called with "UPDATE_INTERVAL" and already have a concheck_p_cur_id scheduled. */ + + if (priv->concheck_p_cur_interval <= priv->concheck_p_max_interval) { + /* we currently have a shorter interval set, then what we now have. Either, + * because we are probing, or because the previous max interval was shorter. + * + * Either way, the current timer is set just fine. Nothing to do. */ + return; + } + + cur_expiry = priv->concheck_p_cur_basetime_ns + (priv->concheck_p_max_interval * NM_UTILS_NS_PER_SECOND); + priv->concheck_p_cur_interval = priv->concheck_p_max_interval; + priv->concheck_p_cur_basetime_ns = nm_utils_get_monotonic_timestamp_ns_cached (&now_ns); + if (cur_expiry <= now_ns) { + /* the last timer was scheduled longer ago then the new desired interval. It means, + * we must schedule a timer right away */ + if (concheck_periodic_schedule_do (self, priv->concheck_p_cur_interval * NM_UTILS_NS_PER_SECOND)) { + concheck_start (self, NULL, NULL, TRUE); + } + } else { + /* we only need to reset the timer. */ + concheck_periodic_schedule_do (self, (cur_expiry - now_ns) / NM_UTILS_NS_PER_MSEC); + } return; - /* If the connectivity check is disabled and we obtain a fake - * result, make an optimistic guess. */ - if (state == NM_CONNECTIVITY_FAKE) { + case CONCHECK_SCHEDULE_CHECK_EXTERNAL: + /* a external connectivity check delays our periodic check. We reset the counter. */ + priv->concheck_p_cur_basetime_ns = nm_utils_get_monotonic_timestamp_ns_cached (&now_ns); + concheck_periodic_schedule_do (self, priv->concheck_p_cur_interval * NM_UTILS_NS_PER_SECOND); + return; + + case CONCHECK_SCHEDULE_CHECK_PERIODIC: + /* we schedule a periodic connectivity check now. We just remember the time when + * we did it. There is nothing to reschedule, it's fine already. */ + priv->concheck_p_cur_basetime_ns = nm_utils_get_monotonic_timestamp_ns_cached (&now_ns); + return; + + /* we just got an event that we lost connectivity (that is, concheck returned). We reset + * the interval to min/max or increase the probe interval (bump). */ + case CONCHECK_SCHEDULE_RETURNED_MIN: + priv->concheck_p_cur_interval = NM_MIN (priv->concheck_p_max_interval, CONCHECK_P_PROBE_INTERVAL); + break; + case CONCHECK_SCHEDULE_RETURNED_MAX: + priv->concheck_p_cur_interval = priv->concheck_p_max_interval; + break; + case CONCHECK_SCHEDULE_RETURNED_BUMP: + priv->concheck_p_cur_interval = NM_MIN (priv->concheck_p_cur_interval * 2, priv->concheck_p_max_interval); + break; + } + + /* we are here, because we returned from a connectivity check and adjust the current interval. + * + * But note that we calculate the new timeout based on the time when we scheduled the + * last check, instead of counting from now. The reaons is, that we want that the times + * when we schedule checks be at precise intervals, without including the time it took for + * the connectivity check. */ + new_expiry = priv->concheck_p_cur_basetime_ns + (priv->concheck_p_cur_interval * NM_UTILS_NS_PER_SECOND); + tdiff = NM_MAX (new_expiry - nm_utils_get_monotonic_timestamp_ns_cached (&now_ns), 0); + priv->concheck_p_cur_basetime_ns = now_ns - tdiff; + concheck_periodic_schedule_do (self, tdiff); +} + +void +nm_device_check_connectivity_update_interval (NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + guint new_interval; + + new_interval = nm_connectivity_get_interval (concheck_get_mgr (self)); + + new_interval = NM_MIN (new_interval, 7 *24 * 3600); + + if (new_interval != priv->concheck_p_max_interval) { + _LOGT (LOGD_CONCHECK, "connectivity: periodic-check: set interval to %u seconds", new_interval); + priv->concheck_p_max_interval = new_interval; + } + + if (!new_interval) { + /* this will cancel any potentially pending timeout. */ + concheck_periodic_schedule_do (self, 0); + return; + } + + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_UPDATE_INTERVAL); +} + +static void +concheck_update_state (NMDevice *self, NMConnectivityState state, gboolean is_periodic) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + /* @state is a result of the connectivity check. We only expect a precise + * number of possible values. */ + nm_assert (NM_IN_SET (state, NM_CONNECTIVITY_LIMITED, + NM_CONNECTIVITY_PORTAL, + NM_CONNECTIVITY_FULL, + NM_CONNECTIVITY_FAKE, + NM_CONNECTIVITY_ERROR)); + + if (state == NM_CONNECTIVITY_ERROR) { + /* on error, we don't change the current connectivity state, + * except making UNKNOWN to NONE. */ + state = priv->connectivity_state; + if (state == NM_CONNECTIVITY_UNKNOWN) + state = NM_CONNECTIVITY_NONE; + } else if (state == NM_CONNECTIVITY_FAKE) { + /* If the connectivity check is disabled and we obtain a fake + * result, make an optimistic guess. */ if (priv->state == NM_DEVICE_STATE_ACTIVATED) { if (nm_device_get_best_default_route (self, AF_UNSPEC)) state = NM_CONNECTIVITY_FULL; @@ -2205,10 +2426,32 @@ concheck_update_state (NMDevice *self, NMConnectivityState state) state = NM_CONNECTIVITY_NONE; } - if (priv->connectivity_state == state) + if (priv->connectivity_state == state) { + /* we got a connectivty update, but the state didn't change. If we were probing, + * we bump the probe frequency. */ + if ( is_periodic + && priv->concheck_p_cur_id) + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_RETURNED_BUMP); return; + } + /* we need to update the probe interval before emitting signals. Emitting + * a signal might call back into NMDevice and change the probe settings. + * So, do that first. */ + if (state == NM_CONNECTIVITY_FULL) { + /* we reached full connectivity state. Stop probing by setting the + * interval to the max. */ + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_RETURNED_MAX); + } else if (priv->connectivity_state == NM_CONNECTIVITY_FULL) { + /* we are about to loose connectivity. (re)start probing by setting + * the timeout interval to the min. */ + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_RETURNED_MIN); + } else { + if ( is_periodic + && priv->concheck_p_cur_id) + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_RETURNED_BUMP); + } - _LOGD (LOGD_CONCHECK, "state changed from %s to %s", + _LOGD (LOGD_CONCHECK, "connectivity state changed from %s to %s", nm_connectivity_state_to_string (priv->connectivity_state), nm_connectivity_state_to_string (state)); priv->connectivity_state = state; @@ -2226,34 +2469,6 @@ concheck_update_state (NMDevice *self, NMConnectivityState state) } } -static void -concheck_periodic (NMConnectivity *connectivity, NMDevice *self) -{ - nm_device_check_connectivity (self, NULL, NULL); -} - -static void -concheck_periodic_update (NMDevice *self) -{ - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - - if ( priv->state == NM_DEVICE_STATE_ACTIVATED - && nm_device_get_best_default_route (self, AF_UNSPEC)) { - if (!priv->concheck_periodic_id) { - priv->concheck_periodic_id = g_signal_connect (concheck_get_mgr (self), - NM_CONNECTIVITY_PERIODIC_CHECK, - G_CALLBACK (concheck_periodic), self); - nm_device_check_connectivity (self, NULL, NULL); - } - } else { - if (priv->concheck_periodic_id) { - /* The default route has gone off, trigger a final connectivity check. */ - nm_clear_g_signal_handler (priv->concheck_mgr, &priv->concheck_periodic_id); - nm_device_check_connectivity (self, NULL, NULL); - } - } -} - static void concheck_handle_complete (NMDeviceConnectivityHandle *handle, GError *error) @@ -2264,7 +2479,7 @@ concheck_handle_complete (NMDeviceConnectivityHandle *handle, c_list_unlink (&handle->concheck_lst); if (handle->c_handle) - nm_connectivity_check_cancel (g_steal_pointer (&handle->c_handle)); + nm_connectivity_check_cancel (handle->c_handle); if (handle->callback) { handle->callback (handle->self, @@ -2291,6 +2506,18 @@ concheck_cb (NMConnectivity *connectivity, gboolean handle_is_alive; guint64 seq; + handle = user_data; + nm_assert (handle->c_handle == c_handle); + nm_assert (NM_IS_DEVICE (handle->self)); + + 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, @@ -2301,12 +2528,7 @@ concheck_cb (NMConnectivity *connectivity, /* we keep NMConnectivity instance alive. It cannot be disposing. */ nm_assert (!nm_utils_error_is_cancelled (error, TRUE)); - handle = user_data; - nm_assert (handle->c_handle == c_handle); - handle->c_handle = NULL; - /* keep @self alive, while we invoke callbacks. */ - self = g_object_ref (handle->self); priv = NM_DEVICE_GET_PRIVATE (self); nm_assert (!handle || c_list_contains (&priv->concheck_lst_head, &handle->concheck_lst)); @@ -2314,7 +2536,7 @@ concheck_cb (NMConnectivity *connectivity, seq = handle->seq; /* first update the new state, and emit signals. */ - concheck_update_state (self, state); + concheck_update_state (self, state, handle->is_periodic); handle_is_alive = FALSE; @@ -2361,10 +2583,11 @@ check_handles: concheck_handle_complete (handle, NULL); } -NMDeviceConnectivityHandle * -nm_device_check_connectivity (NMDevice *self, - NMDeviceConnectivityCallback callback, - gpointer user_data) +static NMDeviceConnectivityHandle * +concheck_start (NMDevice *self, + NMDeviceConnectivityCallback callback, + gpointer user_data, + gboolean is_periodic) { static guint64 seq_counter = 0; NMDevicePrivate *priv; @@ -2379,8 +2602,14 @@ nm_device_check_connectivity (NMDevice *self, handle->self = self; handle->callback = callback; handle->user_data = user_data; + handle->is_periodic = is_periodic; + c_list_link_tail (&priv->concheck_lst_head, &handle->concheck_lst); + _LOGT (LOGD_CONCHECK, "connectivity: start check (seq:%llu%s)", + (long long unsigned) handle->seq, + is_periodic ? ", periodic-check" : ""); + handle->c_handle = nm_connectivity_check_start (concheck_get_mgr (self), nm_device_get_ip_iface (self), concheck_cb, @@ -2388,6 +2617,21 @@ nm_device_check_connectivity (NMDevice *self, return handle; } +NMDeviceConnectivityHandle * +nm_device_check_connectivity (NMDevice *self, + NMDeviceConnectivityCallback callback, + gpointer user_data) +{ + NMDeviceConnectivityHandle *handle; + + if (!concheck_is_possible (self)) + return NULL; + + concheck_periodic_schedule_set (self, CONCHECK_SCHEDULE_CHECK_EXTERNAL); + handle = concheck_start (self, callback, user_data, FALSE); + return handle; +} + void nm_device_check_connectivity_cancel (NMDeviceConnectivityHandle *handle) { @@ -10688,8 +10932,6 @@ nm_device_set_ip_config (NMDevice *self, } if (IS_IPv4) { - concheck_periodic_update (self); - if (!nm_device_sys_iface_state_is_external_or_assume (self)) ip4_rp_filter_update (self); } @@ -13544,7 +13786,7 @@ _set_state_full (NMDevice *self, if (ip_config_valid (old_state) && !ip_config_valid (state)) notify_ip_properties (self); - concheck_periodic_update (self); + nm_device_check_connectivity_update_interval (self); /* Dispose of the cached activation request */ if (req) @@ -14711,7 +14953,7 @@ dispose (GObject *object) g_clear_object (&priv->lldp_listener); } - nm_clear_g_signal_handler (priv->concheck_mgr, &priv->concheck_periodic_id); + nm_clear_g_source (&priv->concheck_p_cur_id); G_OBJECT_CLASS (nm_device_parent_class)->dispose (object); diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 24d184657f..fd266d69ab 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -783,6 +783,8 @@ typedef void (*NMDeviceConnectivityCallback) (NMDevice *self, GError *error, gpointer user_data); +void nm_device_check_connectivity_update_interval (NMDevice *self); + NMDeviceConnectivityHandle *nm_device_check_connectivity (NMDevice *self, NMDeviceConnectivityCallback callback, gpointer user_data); diff --git a/src/main.c b/src/main.c index 69902c8437..bd8e26a556 100644 --- a/src/main.c +++ b/src/main.c @@ -402,8 +402,6 @@ main (int argc, char *argv[]) manager)) goto done; - NM_UTILS_KEEP_ALIVE (manager, nm_connectivity_get (), "NMManager-depends-on-NMConnectivity"); - nm_dispatcher_init (); g_signal_connect (manager, NM_MANAGER_CONFIGURE_QUIT, G_CALLBACK (manager_configure_quit), config); diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 039ff71d05..d46b7d2859 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -81,7 +81,7 @@ struct _NMConnectivityCheckHandle { }; enum { - PERIODIC_CHECK, + CONFIG_CHANGED, LAST_SIGNAL }; @@ -99,7 +99,6 @@ typedef struct { struct { CURLM *curl_mhandle; guint curl_timer; - guint periodic_check_id; } concheck; #endif } NMConnectivityPrivate; @@ -539,21 +538,20 @@ nm_connectivity_check_cancel (NMConnectivityCheckHandle *cb_data) gboolean nm_connectivity_check_enabled (NMConnectivity *self) { - NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); + g_return_val_if_fail (NM_IS_CONNECTIVITY (self), FALSE); - return priv->enabled; + return NM_CONNECTIVITY_GET_PRIVATE (self)->enabled; } /*****************************************************************************/ -#if WITH_CONCHECK -static gboolean -periodic_check (gpointer user_data) +guint +nm_connectivity_get_interval (NMConnectivity *self) { - g_signal_emit (NM_CONNECTIVITY (user_data), signals[PERIODIC_CHECK], 0); - return G_SOURCE_CONTINUE; + return nm_connectivity_check_enabled (self) + ? NM_CONNECTIVITY_GET_PRIVATE (self)->interval + : 0; } -#endif static void update_config (NMConnectivity *self, NMConfigData *config_data) @@ -592,6 +590,7 @@ update_config (NMConnectivity *self, NMConfigData *config_data) /* Set the interval. */ interval = nm_config_data_get_connectivity_interval (config_data); + interval = MIN (interval, (7 * 24 * 3600)); if (priv->interval != interval) { priv->interval = interval; changed = TRUE; @@ -622,13 +621,8 @@ update_config (NMConnectivity *self, NMConfigData *config_data) changed = TRUE; } -#if WITH_CONCHECK - if (changed) { - nm_clear_g_source (&priv->concheck.periodic_check_id); - if (nm_connectivity_check_enabled (self)) - priv->concheck.periodic_check_id = g_timeout_add_seconds (priv->interval, periodic_check, self); - } -#endif + if (changed) + g_signal_emit (self, signals[CONFIG_CHANGED], 0); } static void @@ -699,7 +693,6 @@ again: curl_multi_cleanup (priv->concheck.curl_mhandle); curl_global_cleanup (); - nm_clear_g_source (&priv->concheck.periodic_check_id); #endif if (priv->config) { @@ -715,8 +708,8 @@ nm_connectivity_class_init (NMConnectivityClass *klass) { GObjectClass *object_class = G_OBJECT_CLASS (klass); - signals[PERIODIC_CHECK] = - g_signal_new (NM_CONNECTIVITY_PERIODIC_CHECK, + signals[CONFIG_CHANGED] = + g_signal_new (NM_CONNECTIVITY_CONFIG_CHANGED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_FIRST, 0, NULL, NULL, NULL, diff --git a/src/nm-connectivity.h b/src/nm-connectivity.h index 6c16982307..df9295e02b 100644 --- a/src/nm-connectivity.h +++ b/src/nm-connectivity.h @@ -34,7 +34,7 @@ #define NM_IS_CONNECTIVITY_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_CONNECTIVITY)) #define NM_CONNECTIVITY_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_CONNECTIVITY, NMConnectivityClass)) -#define NM_CONNECTIVITY_PERIODIC_CHECK "nm-connectivity-periodic-check" +#define NM_CONNECTIVITY_CONFIG_CHANGED "config-changed" typedef struct _NMConnectivityClass NMConnectivityClass; @@ -46,6 +46,8 @@ const char *nm_connectivity_state_to_string (NMConnectivityState state); gboolean nm_connectivity_check_enabled (NMConnectivity *self); +guint nm_connectivity_get_interval (NMConnectivity *self); + typedef struct _NMConnectivityCheckHandle NMConnectivityCheckHandle; typedef void (*NMConnectivityCheckCallback) (NMConnectivity *self, diff --git a/src/nm-manager.c b/src/nm-manager.c index e98b9fe03f..98de80782d 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -132,10 +132,8 @@ typedef struct { NMState state; NMConfig *config; - NMConnectivityState connectivity_state; - + NMConnectivity *concheck_mgr; NMPolicy *policy; - NMHostnameManager *hostname_manager; struct { @@ -170,6 +168,8 @@ typedef struct { guint devices_inited_id; + NMConnectivityState connectivity_state; + bool startup:1; bool devices_inited:1; @@ -334,6 +334,34 @@ static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark) /*****************************************************************************/ +static void +concheck_config_changed_cb (NMConnectivity *connectivity, + NMManager *self) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + NMDevice *device; + + c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) + nm_device_check_connectivity_update_interval (device); +} + +static NMConnectivity * +concheck_get_mgr (NMManager *self) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + + if (G_UNLIKELY (!priv->concheck_mgr)) { + priv->concheck_mgr = g_object_ref (nm_connectivity_get ()); + g_signal_connect (priv->concheck_mgr, + NM_CONNECTIVITY_CONFIG_CHANGED, + G_CALLBACK (concheck_config_changed_cb), + self); + } + return priv->concheck_mgr; +} + +/*****************************************************************************/ + typedef struct { int ifindex; guint32 aspired_metric; @@ -5527,10 +5555,10 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, data->remaining = 0; c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { - data->remaining++; - nm_device_check_connectivity (device, - device_connectivity_done, - data); + if (nm_device_check_connectivity (device, + device_connectivity_done, + data)) + data->remaining++; } if (data->remaining == 0) { @@ -6647,7 +6675,7 @@ get_property (GObject *object, guint prop_id, break; case PROP_CONNECTIVITY_CHECK_ENABLED: g_value_set_boolean (value, - nm_connectivity_check_enabled (nm_connectivity_get ())); + nm_connectivity_check_enabled (concheck_get_mgr (self))); break; case PROP_PRIMARY_CONNECTION: nm_dbus_utils_g_value_set_object_path (value, priv->primary_connection); @@ -6777,6 +6805,13 @@ dispose (GObject *object) g_clear_pointer (&priv->checkpoint_mgr, nm_checkpoint_manager_free); + if (priv->concheck_mgr) { + g_signal_handlers_disconnect_by_func (priv->concheck_mgr, + G_CALLBACK (concheck_config_changed_cb), + self); + g_clear_object (&priv->concheck_mgr); + } + if (priv->auth_mgr) { g_signal_handlers_disconnect_by_func (priv->auth_mgr, G_CALLBACK (auth_mgr_changed), From 91b1af2839f7b065df283c664142c8e15b993c68 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Apr 2018 15:55:16 +0200 Subject: [PATCH 5/5] connectivity: emit properties-changed about connectivity settings --- src/nm-manager.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 98de80782d..fcebcfd314 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -176,6 +176,8 @@ typedef struct { bool sleeping:1; bool net_enabled:1; + unsigned connectivity_check_enabled_last:2; + guint delete_volatile_connection_idle_id; CList delete_volatile_connection_lst_head; } NMManagerPrivate; @@ -326,6 +328,8 @@ static NMActiveConnection *active_connection_find_first (NMManager *self, const char *uuid, NMActiveConnectionState max_state); +static NMConnectivity *concheck_get_mgr (NMManager *self); + /*****************************************************************************/ static NM_CACHED_QUARK_FCN ("active-connection-add-and-activate", active_connection_add_and_activate_quark) @@ -334,12 +338,34 @@ static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark) /*****************************************************************************/ +static gboolean +concheck_enabled (NMManager *self, gboolean *out_changed) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + guint check_enabled; + + check_enabled = nm_connectivity_check_enabled (concheck_get_mgr (self)) + ? 1 : 2; + if (priv->connectivity_check_enabled_last == check_enabled) + NM_SET_OUT (out_changed, FALSE); + else { + NM_SET_OUT (out_changed, TRUE); + priv->connectivity_check_enabled_last = check_enabled; + } + return check_enabled == 1; +} + static void concheck_config_changed_cb (NMConnectivity *connectivity, NMManager *self) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); NMDevice *device; + gboolean changed; + + concheck_enabled (self, &changed); + if (changed) + _notify (self, PROP_CONNECTIVITY_CHECK_ENABLED); c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) nm_device_check_connectivity_update_interval (device); @@ -849,8 +875,14 @@ active_connection_get_by_path (NMManager *self, const char *path) static void _config_changed_cb (NMConfig *config, NMConfigData *config_data, NMConfigChangeFlags changes, NMConfigData *old_data, NMManager *self) { + g_object_freeze_notify (G_OBJECT (self)); + if (NM_FLAGS_HAS (changes, NM_CONFIG_CHANGE_GLOBAL_DNS_CONFIG)) _notify (self, PROP_GLOBAL_DNS_CONFIGURATION); + if ((!nm_config_data_get_connectivity_uri (config_data)) != (!nm_config_data_get_connectivity_uri (old_data))) + _notify (self, PROP_CONNECTIVITY_CHECK_AVAILABLE); + + g_object_thaw_notify (G_OBJECT (self)); } static void @@ -6674,8 +6706,7 @@ get_property (GObject *object, guint prop_id, g_value_set_boolean (value, nm_config_data_get_connectivity_uri (config_data) != NULL); break; case PROP_CONNECTIVITY_CHECK_ENABLED: - g_value_set_boolean (value, - nm_connectivity_check_enabled (concheck_get_mgr (self))); + g_value_set_boolean (value, concheck_enabled (self, NULL)); break; case PROP_PRIMARY_CONNECTION: nm_dbus_utils_g_value_set_object_path (value, priv->primary_connection);