proxy: use CList to track configs in NMPacrunnnerManagerth/c-list

- config->removed can be replaced by c_list_is_empty(&config->lst)

- downgrade some assertions to nm_assert(). Even without the
  assert we crash a few lines later with a NULL pointer access.
  That gives almost the same debuggability and discoverability
  of the bug.

- use exected type signature for GAsyncReadyCallback and avoid
  the cast.

- when the name owner disappears, cancel all asynchronous
  operations. Note how the new pacrunner instance will anyway
  start without configuration, so for all purpose, all pending
  operations are at that moment obsolete.

1 files changed, 95 insertions, 73 deletions

diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c
index 87e0a36437..31f83256f3 100644
--- a/src/nm-pacrunner-manager.c
+++ b/src/nm-pacrunner-manager.c
@@ -27,8 +27,7 @@
 #include "nm-proxy-config.h"
 #include "nm-ip4-config.h"
 #include "nm-ip6-config.h"
-
-static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data);
+#include "nm-utils/c-list.h"
 
 #define PACRUNNER_DBUS_SERVICE "org.pacrunner"
 #define PACRUNNER_DBUS_INTERFACE "org.pacrunner.Manager"
@@ -37,11 +36,15 @@ static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointe
 /*****************************************************************************/
 
 struct _NMPacrunnerCallId {
-	NMPacrunnerManager *manager;
+	CList lst;
+
+	/* this might be a dangling pointer after the async operation
+	 * is cancelled. */
+	NMPacrunnerManager *manager_maybe_dangling;
+
 	GVariant *args;
 	char *path;
 	guint refcount;
-	bool removed;
 };
 
 typedef struct _NMPacrunnerCallId Config;
@@ -49,8 +52,8 @@ typedef struct _NMPacrunnerCallId Config;
 typedef struct {
 	char *iface;
 	GDBusProxy *pacrunner;
-	GCancellable *pacrunner_cancellable;
-	GList *configs;
+	GCancellable *cancellable;
+	CList configs;
 } NMPacrunnerManagerPrivate;
 
 struct _NMPacrunnerManager {
@@ -80,49 +83,59 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP
 	G_STMT_START { \
 		nm_log ((level), _NMLOG_DOMAIN, NULL, NULL, \
 		        "%s%p]: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \
-		        "pacrunner: call[", \
+		        _NMLOG2_PREFIX_NAME": call[", \
 		        (config) \
 		        _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
 	} G_STMT_END
 
 /*****************************************************************************/
 
+static void pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data);
+
+/*****************************************************************************/
+
 static Config *
 config_new (NMPacrunnerManager *manager, GVariant *args)
 {
 	Config *config;
 
 	config = g_slice_new0 (Config);
-	config->manager = manager;
+	config->manager_maybe_dangling = manager;
 	config->args = g_variant_ref_sink (args);
 	config->refcount = 1;
+	c_list_link_tail (&NM_PACRUNNER_MANAGER_GET_PRIVATE (manager)->configs,
+	                  &config->lst);
 
 	return config;
 }
 
-static void
+static Config *
 config_ref (Config *config)
 {
-	g_assert (config);
-	g_assert (config->refcount > 0);
+	nm_assert (config);
+	nm_assert (config->refcount > 0);
 
 	config->refcount++;
+	return config;
 }
 
 static void
 config_unref (Config *config)
 {
-	g_assert (config);
-	g_assert (config->refcount > 0);
+	nm_assert (config);
+	nm_assert (config->refcount > 0);
 
 	if (config->refcount == 1) {
 		g_variant_unref (config->args);
 		g_free (config->path);
+		c_list_unlink (&config->lst);
 		g_slice_free (Config, config);
 	} else
 		config->refcount--;
 }
 
+/*****************************************************************************/
+
 static void
 add_proxy_config (GVariantBuilder *proxy_data, const NMProxyConfig *proxy_config)
 {
@@ -220,8 +233,18 @@ get_ip6_domains (GPtrArray *domains, NMIP6Config *ip6)
 	}
 }
 
+/*****************************************************************************/
+
+static GCancellable *
+_ensure_cancellable (NMPacrunnerManagerPrivate *priv)
+{
+	if (G_UNLIKELY (!priv->cancellable))
+		priv->cancellable = g_cancellable_new ();
+	return priv->cancellable;
+}
+
 static void
-pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
+pacrunner_send_done (GObject *source, GAsyncResult *res, gpointer user_data)
 {
 	Config *config = user_data;
 	NMPacrunnerManager *self;
@@ -230,15 +253,13 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 	gs_unref_variant GVariant *variant = NULL;
 	const char *path = NULL;
 
-	g_return_if_fail (!config->path);
+	nm_assert (!config->path);
 
-	variant = g_dbus_proxy_call_finish (proxy, res, &error);
-	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
-		config_unref (config);
-		return;
-	}
+	variant = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error);
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		goto out;
 
-	self = NM_PACRUNNER_MANAGER (config->manager);
+	self = NM_PACRUNNER_MANAGER (config->manager_maybe_dangling);
 	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
 	if (!variant)
@@ -246,21 +267,23 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 	else {
 		g_variant_get (variant, "(&o)", &path);
 
-		config->path = g_strdup (path);
-		_LOG2D (config, "sent");
-
-		if (config->removed) {
-			config_ref (config);
+		if (c_list_is_empty (&config->lst)) {
+			_LOG2D (config, "sent (%s), but destory it right away", path);
 			g_dbus_proxy_call (priv->pacrunner,
 			                   "DestroyProxyConfiguration",
-			                   g_variant_new ("(o)", config->path),
+			                   g_variant_new ("(o)", path),
 			                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
 			                   -1,
-			                   priv->pacrunner_cancellable,
-			                   (GAsyncReadyCallback) pacrunner_remove_done,
-			                   config);
+			                   _ensure_cancellable (priv),
+			                   pacrunner_remove_done,
+			                   config_ref (config));
+		} else {
+			_LOG2D (config, "sent (%s)", path);
+			config->path = g_strdup (path);
 		}
 	}
+
+out:
 	config_unref (config);
 }
 
@@ -272,17 +295,15 @@ pacrunner_send_config (NMPacrunnerManager *self, Config *config)
 	if (priv->pacrunner) {
 		_LOG2T (config, "sending...");
 
-		config_ref (config);
-		g_clear_pointer (&config->path, g_free);
-
+		nm_assert (!config->path);
 		g_dbus_proxy_call (priv->pacrunner,
 		                   "CreateProxyConfiguration",
 		                   config->args,
 		                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
 		                   -1,
-		                   priv->pacrunner_cancellable,
-		                   (GAsyncReadyCallback) pacrunner_send_done,
-		                   config);
+		                   _ensure_cancellable (priv),
+		                   pacrunner_send_done,
+		                   config_ref (config));
 	}
 }
 
@@ -291,15 +312,18 @@ name_owner_changed (NMPacrunnerManager *self)
 {
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 	gs_free char *owner = NULL;
-	GList *iter = NULL;
+	CList *iter;
 
 	owner = g_dbus_proxy_get_name_owner (priv->pacrunner);
 	if (owner) {
 		_LOGD ("name owner appeared (%s)", owner);
-		for (iter = g_list_first (priv->configs); iter; iter = g_list_next (iter))
-			pacrunner_send_config (self, iter->data);
+		c_list_for_each (iter, &priv->configs)
+			pacrunner_send_config (self, c_list_entry (iter, Config, lst));
 	} else {
 		_LOGD ("name owner disappeared");
+		nm_clear_g_cancellable (&priv->cancellable);
+		c_list_for_each (iter, &priv->configs)
+			nm_clear_g_free (&c_list_entry (iter, Config, lst)->path);
 	}
 }
 
@@ -316,21 +340,19 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data)
 {
 	NMPacrunnerManager *self = user_data;
 	NMPacrunnerManagerPrivate *priv;
-	GError *error = NULL;
+	gs_free_error GError *error = NULL;
 	GDBusProxy *proxy;
 
 	proxy = g_dbus_proxy_new_for_bus_finish (res, &error);
 	if (!proxy) {
 		if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-			_LOGW ("failed to connect to pacrunner via DBus: %s", error->message);
-		g_error_free (error);
+			_LOGE ("failed to create D-Bus proxy for pacrunner: %s", error->message);
 		return;
 	}
 
 	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
 	priv->pacrunner = proxy;
-
 	g_signal_connect (priv->pacrunner, "notify::g-name-owner",
 	                  G_CALLBACK (name_owner_changed_cb), self);
 	name_owner_changed (self);
@@ -420,7 +442,6 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 	}
 
 	config = config_new (self, g_variant_new ("(a{sv})", &proxy_data));
-	priv->configs = g_list_append (priv->configs, config);
 
 	{
 		gs_free char *args_str = NULL;
@@ -439,26 +460,24 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 }
 
 static void
-pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
+pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data)
 {
 	Config *config = user_data;
 	NMPacrunnerManager *self;
 	gs_free_error GError *error = NULL;
 	gs_unref_variant GVariant *ret = NULL;
 
-	ret = g_dbus_proxy_call_finish (proxy, res, &error);
-	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
-		config_unref (config);
-		return;
-	}
-
-	self = NM_PACRUNNER_MANAGER (config->manager);
+	ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error);
+	if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		goto out;
 
+	self = NM_PACRUNNER_MANAGER (config->manager_maybe_dangling);
 	if (!ret)
 		_LOG2D (config, "remove failed: %s", error->message);
 	else
 		_LOG2D (config, "removed");
 
+out:
 	config_unref (config);
 }
 
@@ -472,7 +491,6 @@ nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_i
 {
 	NMPacrunnerManagerPrivate *priv;
 	Config *config;
-	GList *list;
 
 	g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self));
 	g_return_if_fail (call_id);
@@ -482,31 +500,28 @@ nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_i
 
 	_LOG2T (config, "removing...");
 
-	list = g_list_find (priv->configs, config);
-	if (!list)
-		g_return_if_reached ();
+	nm_assert (c_list_contains (&priv->configs, &config->lst));
 
 	if (priv->pacrunner) {
 		if (!config->path) {
-			/* send() failed or is still pending. Mark the item as
+			/* send() failed or is still pending. The item is marked as
 			 * removed, so that we ask pacrunner to drop it when the
 			 * send() completes.
 			 */
-			config->removed = TRUE;
-			config_unref (config);
 		} else {
 			g_dbus_proxy_call (priv->pacrunner,
 			                   "DestroyProxyConfiguration",
 			                   g_variant_new ("(o)", config->path),
 			                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
 			                   -1,
-			                   priv->pacrunner_cancellable,
-			                   (GAsyncReadyCallback) pacrunner_remove_done,
-			                   config);
+			                   _ensure_cancellable (priv),
+			                   pacrunner_remove_done,
+			                   config_ref (config));
 		}
-	} else
-		config_unref (config);
-	priv->configs = g_list_delete_link (priv->configs, list);
+	}
+
+	c_list_unlink_init (&config->lst);
+	config_unref (config);
 }
 
 gboolean
@@ -532,16 +547,15 @@ nm_pacrunner_manager_init (NMPacrunnerManager *self)
 {
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
-	priv->pacrunner_cancellable = g_cancellable_new ();
-
+	c_list_init (&priv->configs);
 	g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM,
 	                          G_DBUS_PROXY_FLAGS_NONE,
 	                          NULL,
 	                          PACRUNNER_DBUS_SERVICE,
 	                          PACRUNNER_DBUS_PATH,
 	                          PACRUNNER_DBUS_INTERFACE,
-	                          priv->pacrunner_cancellable,
-	                          (GAsyncReadyCallback) pacrunner_proxy_cb,
+	                          _ensure_cancellable (priv),
+	                          pacrunner_proxy_cb,
 	                          self);
 }
 
@@ -549,14 +563,22 @@ static void
 dispose (GObject *object)
 {
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE ((NMPacrunnerManager *) object);
+	CList *iter, *safe;
+
+	c_list_for_each_safe (iter, safe, &priv->configs) {
+		c_list_unlink_init (iter);
+		config_unref (c_list_entry (iter, Config, lst));
+	}
+
+	/* we cancel all pending operations. Note that pacrunner automatically
+	 * removes all configuration once NetworkManager disconnects from
+	 * the bus -- which happens soon after we destroy the pacrunner manager.
+	 */
+	nm_clear_g_cancellable (&priv->cancellable);
 
 	g_clear_pointer (&priv->iface, g_free);
-	nm_clear_g_cancellable (&priv->pacrunner_cancellable);
 	g_clear_object (&priv->pacrunner);
 
-	g_list_free_full (priv->configs, (GDestroyNotify) config_unref);
-	priv->configs = NULL;
-
 	G_OBJECT_CLASS (nm_pacrunner_manager_parent_class)->dispose (object);
 }