connectivity: merge branch 'th/connectivity-busy-loop-fix'

1 files changed, 65 insertions, 24 deletions

diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c
index 51a6970e32..3b779677da 100644
--- a/src/nm-connectivity.c
+++ b/src/nm-connectivity.c
@@ -195,7 +195,7 @@ cb_data_free (NMConnectivityCheckHandle *cb_data,
 		 * 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(). */
+		 * message goes to CURLMSG_DONE in _con_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);
@@ -234,8 +234,8 @@ _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)
+static gboolean
+_con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask)
 {
 	NMConnectivityCheckHandle *cb_data;
 	CURLMsg *msg;
@@ -244,10 +244,13 @@ curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask)
 	long response_code;
 	CURLMcode ret;
 	int running_handles;
+	gboolean success = TRUE;
 
 	ret = curl_multi_socket_action (mhandle, sockfd, ev_bitmask, &running_handles);
-	if (ret != CURLM_OK)
+	if (ret != CURLM_OK) {
 		_LOGD ("connectivity check failed: %d", ret);
+		success = FALSE;
+	}
 
 	while ((msg = curl_multi_info_read (mhandle, &m_left))) {
 
@@ -258,6 +261,7 @@ curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask)
 		eret = curl_easy_getinfo (msg->easy_handle, CURLINFO_PRIVATE, (char **) &cb_data);
 		if (eret != CURLE_OK) {
 			_LOGD ("curl cannot extract cb_data for easy handle, skipping msg");
+			success = FALSE;
 			continue;
 		}
 
@@ -286,16 +290,22 @@ curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask)
 			              "unexpected short response");
 		}
 	}
+
+	/* if we return a failure, we don't know what went wrong. It's likely serious, because
+	 * a failure here is not expected. Return FALSE, so that we stop polling the file descriptor.
+	 * Worst case, this leaves the pending connectivity check unhandled, until our regular
+	 * time-out kicks in. */
+	return success;
 }
 
 static gboolean
-curl_timeout_cb (gpointer user_data)
+_con_curl_timeout_cb (gpointer user_data)
 {
 	gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data));
 	NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self);
 
 	priv->concheck.curl_timer = 0;
-	curl_check_connectivity (priv->concheck.curl_mhandle, CURL_SOCKET_TIMEOUT, 0);
+	_con_curl_check_connectivity (priv->concheck.curl_mhandle, CURL_SOCKET_TIMEOUT, 0);
 	return G_SOURCE_REMOVE;
 }
 
@@ -307,17 +317,34 @@ multi_timer_cb (CURLM *multi, long timeout_ms, void *userdata)
 
 	nm_clear_g_source (&priv->concheck.curl_timer);
 	if (timeout_ms != -1)
-		priv->concheck.curl_timer = g_timeout_add (timeout_ms, curl_timeout_cb, self);
+		priv->concheck.curl_timer = g_timeout_add (timeout_ms, _con_curl_timeout_cb, self);
 	return 0;
 }
 
+typedef struct {
+	NMConnectivity *self;
+	GIOChannel *ch;
+
+	/* this is a very simplistic weak-pointer. If ConCurlSockData gets
+	 * destroyed, it will set *destroy_notify to TRUE.
+	 *
+	 * _con_curl_socketevent_cb() uses this to detect whether it can
+	 * safely access @fdp after _con_curl_check_connectivity(). */
+	gboolean *destroy_notify;
+
+	guint ev;
+} ConCurlSockData;
+
 static gboolean
-curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data)
+_con_curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data)
 {
-	gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data));
+	ConCurlSockData *fdp = user_data;
+	gs_unref_object NMConnectivity *self = g_object_ref (fdp->self);
 	NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self);
 	int fd = g_io_channel_unix_get_fd (ch);
 	int action = 0;
+	gboolean fdp_destroyed = FALSE;
+	gboolean success;
 
 	if (condition & G_IO_IN)
 		action |= CURL_CSELECT_IN;
@@ -326,35 +353,49 @@ curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data)
 	if (condition & G_IO_ERR)
 		action |= CURL_CSELECT_ERR;
 
-	curl_check_connectivity (priv->concheck.curl_mhandle, fd, action);
-	return G_SOURCE_CONTINUE;
-}
+	nm_assert (!fdp->destroy_notify);
+	fdp->destroy_notify = &fdp_destroyed;
 
-typedef struct {
-	GIOChannel *ch;
-	guint ev;
-} CurlSockData;
+	success = _con_curl_check_connectivity (priv->concheck.curl_mhandle, fd, action);
+
+	if (fdp_destroyed) {
+		/* hups. fdp got invalidated during _con_curl_check_connectivity(). That's fine,
+		 * just don't touch it. */
+	} else {
+		nm_assert (fdp->destroy_notify == &fdp_destroyed);
+		fdp->destroy_notify = NULL;
+		if (!success)
+			fdp->ev = 0;
+	}
+
+	return success ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE;
+}
 
 static int
-multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void *socketp)
+multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, void *socketp)
 {
 	NMConnectivity *self = NM_CONNECTIVITY (userdata);
 	NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self);
-	CurlSockData *fdp = socketp;
+	ConCurlSockData *fdp = socketp;
 	GIOCondition condition = 0;
 
+	(void) _NM_ENSURE_TYPE (int, fd);
+
 	if (what == CURL_POLL_REMOVE) {
 		if (fdp) {
-			curl_multi_assign (priv->concheck.curl_mhandle, s, NULL);
+			if (fdp->destroy_notify)
+				*fdp->destroy_notify = TRUE;
+			curl_multi_assign (priv->concheck.curl_mhandle, fd, NULL);
 			nm_clear_g_source (&fdp->ev);
 			g_io_channel_unref (fdp->ch);
-			g_slice_free (CurlSockData, fdp);
+			g_slice_free (ConCurlSockData, fdp);
 		}
 	} else {
 		if (!fdp) {
-			fdp = g_slice_new0 (CurlSockData);
-			fdp->ch = g_io_channel_unix_new (s);
-			curl_multi_assign (priv->concheck.curl_mhandle, s, fdp);
+			fdp = g_slice_new0 (ConCurlSockData);
+			fdp->self = self;
+			fdp->ch = g_io_channel_unix_new (fd);
+			curl_multi_assign (priv->concheck.curl_mhandle, fd, fdp);
 		} else
 			nm_clear_g_source (&fdp->ev);
 
@@ -366,7 +407,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void
 			condition = G_IO_IN | G_IO_OUT;
 
 		if (condition)
-			fdp->ev = g_io_add_watch (fdp->ch, condition, curl_socketevent_cb, self);
+			fdp->ev = g_io_add_watch (fdp->ch, condition, _con_curl_socketevent_cb, fdp);
 	}
 
 	return CURLM_OK;