pacrunner: rework processing of config entries

Fix some issues in nm-pacrunner-manager.c:

 - when adding a configuration through nm_pacrunner_manager_send(), we
   kept an association between the interface name and the pacrunner
   configuration object path, so that the configuration for that
   interface could be removed later. Unfortunately not all
   configurations have an interface associated, so we need a more
   generic way to identify configurations. Introduce a new @tag
   argument that serves as key to match configurations

 - the interface name of the last pushed configuration was stored in
   the manager private config and reused later; this could cause
   issues when there are multiple outstanding D-Bus calls. The
   interface is not needed anymore after the previous point.

 - remove() didn't actually remove the configuration from the list

4 files changed, 153 insertions, 85 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 8fef0d2ee6..530658df9d 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -12120,6 +12120,7 @@ _set_state_full (NMDevice *self,
 		if (priv->proxy_config) {
 			nm_pacrunner_manager_send (priv->pacrunner_manager,
 			                           nm_device_get_ip_iface (self),
+			                           nm_device_get_ip_iface (self),
 			                           priv->proxy_config,
 			                           priv->ip4_config,
 			                           priv->ip6_config);
diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c
index 45d291a551..18fdd229a0 100644
--- a/src/nm-pacrunner-manager.c
+++ b/src/nm-pacrunner-manager.c
@@ -28,24 +28,29 @@
 #include "nm-ip4-config.h"
 #include "nm-ip6-config.h"
 
+static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data);
+
 #define PACRUNNER_DBUS_SERVICE "org.pacrunner"
 #define PACRUNNER_DBUS_INTERFACE "org.pacrunner.Manager"
 #define PACRUNNER_DBUS_PATH "/org/pacrunner/manager"
 
 /*****************************************************************************/
 
-struct remove_data {
-	char *iface;
+typedef struct {
+	char *tag;
+	NMPacrunnerManager *manager;
+	GVariant *args;
 	char *path;
-};
+	guint refcount;
+	bool removed;
+} Config;
 
 typedef struct {
 	char *iface;
 	GDBusProxy *pacrunner;
 	gboolean pacrunner_running;
 	GCancellable *pacrunner_cancellable;
-	GList *args;
-	GList *remove;
+	GList *configs;
 } NMPacrunnerManagerPrivate;
 
 struct _NMPacrunnerManager {
@@ -72,15 +77,42 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP
 
 /*****************************************************************************/
 
+static Config *
+config_new (NMPacrunnerManager *manager, char *tag, GVariant *args)
+{
+	Config *config;
+
+	config = g_slice_new0 (Config);
+	config->manager = manager;
+	config->tag = tag;
+	config->args = args;
+	config->refcount = 1;
+
+	return config;
+}
+
 static void
-remove_data_destroy (struct remove_data *data)
+config_ref (Config *config)
 {
-	g_return_if_fail (data != NULL);
+	g_assert (config);
+	g_assert (config->refcount > 0);
 
-	g_free (data->iface);
-	g_free (data->path);
-	memset (data, 0, sizeof (struct remove_data));
-	g_free (data);
+	config->refcount++;
+}
+
+static void
+config_unref (Config *config)
+{
+	g_assert (config);
+	g_assert (config->refcount > 0);
+
+	if (config->refcount == 1) {
+		g_free (config->tag);
+		g_variant_unref (config->args);
+		g_free (config->path);
+		g_slice_free (Config, config);
+	} else
+		config->refcount--;
 }
 
 static void
@@ -181,63 +213,71 @@ get_ip6_domains (GPtrArray *domains, NMIP6Config *ip6)
 }
 
 static void
-pacrunner_send_done (GObject *source, GAsyncResult *res, gpointer user_data)
+pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 {
-	NMPacrunnerManager *self = NM_PACRUNNER_MANAGER (user_data);
-	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
+	Config *config = user_data;
+	NMPacrunnerManager *self;
+	NMPacrunnerManagerPrivate *priv;
 	gs_free_error GError *error = NULL;
 	gs_unref_variant GVariant *variant = NULL;
 	const char *path = NULL;
-	GList *iter = NULL;
-	gboolean found = FALSE;
 
-	variant = g_dbus_proxy_call_finish (priv->pacrunner, res, &error);
+	g_return_if_fail (!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;
+	}
+
+	self = NM_PACRUNNER_MANAGER (config->manager);
+	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
+
 	if (!variant) {
-		_LOGD ("sending proxy config to pacrunner failed: %s", error->message);
+		_LOGD ("send config for '%s' failed: %s", config->tag, error->message);
 	} else {
-		struct remove_data *data;
 		g_variant_get (variant, "(&o)", &path);
 
-		/* Replace the old path (if any) of proxy config with the new one returned
-		 * from CreateProxyConfiguration() DBus method on pacrunner.
-		 */
-		for (iter = g_list_first (priv->remove); iter; iter = g_list_next (iter)) {
-			struct remove_data *r = iter->data;
-			if (g_strcmp0 (priv->iface, r->iface) == 0) {
-				g_free (r->path);
-				r->path = g_strdup (path);
-				found = TRUE;
-				break;
-			}
-		}
-
-		if (!found) {
-			data = g_malloc0 (sizeof (struct remove_data));
-			data->iface = g_strdup (priv->iface);
-			data->path = g_strdup (path);
-			priv->remove = g_list_append (priv->remove, data);
-			_LOGD ("proxy config sent to pacrunner");
+		config->path = g_strdup (path);
+		_LOGD ("successfully sent config for '%s'", config->tag);
+
+		if (config->removed) {
+			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);
 		}
 	}
+	config_unref (config);
 }
 
 static void
-send_pacrunner_proxy_data (NMPacrunnerManager *self, GVariant *pacrunner_manager_args)
+pacrunner_send_config (NMPacrunnerManager *self, Config *config)
 {
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
-
-	if (!pacrunner_manager_args)
-		return;
+	gs_free char *args_str = NULL;
 
 	if (priv->pacrunner && priv->pacrunner_running) {
+		if (_LOGT_ENABLED ()) {
+			args_str = g_variant_print (config->args, FALSE);
+			_LOGT ("sending proxy config for '%s': %s", config->tag, args_str);
+		}
+
+		config_ref (config);
+		g_clear_pointer (&config->path, g_free);
+
 		g_dbus_proxy_call (priv->pacrunner,
 		                   "CreateProxyConfiguration",
-		                   pacrunner_manager_args,
+		                   config->args,
 		                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
 		                   -1,
-		                   NULL,
-		                  (GAsyncReadyCallback) pacrunner_send_done,
-		                   self);
+		                   priv->pacrunner_cancellable,
+		                   (GAsyncReadyCallback) pacrunner_send_done,
+		                   config);
 	}
 }
 
@@ -259,9 +299,8 @@ name_owner_changed (GObject *object,
 	if (owner) {
 		priv->pacrunner_running = TRUE;
 		_LOGD ("pacrunner appeared as %s", owner);
-		for (iter = g_list_first(priv->args); iter; iter = g_list_next(iter)) {
-			send_pacrunner_proxy_data (self, iter->data);
-		}
+		for (iter = g_list_first (priv->configs); iter; iter = g_list_next (iter))
+			pacrunner_send_config (self, iter->data);
 	} else {
 		priv->pacrunner_running = FALSE;
 		_LOGD ("pacrunner disappeared");
@@ -298,6 +337,7 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data)
  * nm_pacrunner_manager_send:
  * @self: the #NMPacrunnerManager
  * @iface: the iface for the connection or %NULL
+ * @tag: unique configuration identifier
  * @proxy_config: proxy config of the connection
  * @ip4_config: IP4 config of the connection
  * @ip6_config: IP6 config of the connection
@@ -305,6 +345,7 @@ pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data)
 void
 nm_pacrunner_manager_send (NMPacrunnerManager *self,
                            const char *iface,
+                           const char *tag,
                            NMProxyConfig *proxy_config,
                            NMIP4Config *ip4_config,
                            NMIP6Config *ip6_config)
@@ -313,8 +354,8 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 	NMProxyConfigMethod method;
 	NMPacrunnerManagerPrivate *priv;
 	GVariantBuilder proxy_data;
-	GVariant *pacrunner_manager_args;
 	GPtrArray *domains;
+	Config *config;
 
 	g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self));
 	g_return_if_fail (proxy_config);
@@ -366,31 +407,39 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 		g_strfreev (strv);
 	}
 
-	pacrunner_manager_args = g_variant_ref_sink (g_variant_new ("(a{sv})", &proxy_data));
-	priv->args = g_list_append (priv->args, pacrunner_manager_args);
+	config = config_new (self, g_strdup (tag),
+	                     g_variant_ref_sink (g_variant_new ("(a{sv})", &proxy_data)));
+	priv->configs = g_list_append (priv->configs, config);
 
-	/* Send if pacrunner is available on Bus, otherwise
-	 * argument has already been appended above to be
+	/* Send if pacrunner is available on bus, otherwise
+	 * config has already been appended above to be
 	 * sent when pacrunner appears.
 	 */
-	send_pacrunner_proxy_data (self, pacrunner_manager_args);
+	pacrunner_send_config (self, config);
 }
 
 static void
-pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data)
+pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 {
-	/* @self may be a dangling pointer. However, we don't use it as the
-	 * logging macro below does not dereference @self. */
-	NMPacrunnerManager *self = 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 ((GDBusProxy *) source, res, &error);
+	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);
 
 	if (!ret)
-		_LOGD ("Couldn't remove proxy config from pacrunner: %s", error->message);
+		_LOGD ("couldn't remove config for '%s': %s", config->tag, error->message);
 	else
-		_LOGD ("Successfully removed proxy config from pacrunner");
+		_LOGD ("successfully removed config for '%s'", config->tag);
+
+	config_unref (config);
 }
 
 /**
@@ -400,27 +449,44 @@ pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data)
  * from pacrunner
  */
 void
-nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *iface)
+nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag)
 {
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 	GList *list;
 
-	for (list = g_list_first (priv->remove); list; list = g_list_next (list)) {
-		struct remove_data *data = list->data;
-		if (g_strcmp0 (data->iface, iface) == 0) {
-			if (priv->pacrunner  && priv->pacrunner_running && data->path) {
+	g_return_if_fail (tag);
+
+	_LOGT ("removing config for '%s'", tag);
+
+	for (list = g_list_first (priv->configs); list; list = g_list_next (list)) {
+		Config *config = list->data;
+
+		if (nm_streq (config->tag, tag)) {
+			if (priv->pacrunner  && priv->pacrunner_running) {
+				if (!config->path) {
+					/* send() is pending: mark the config as removed
+					 * so that the send() callback will remove it when
+					 * the D-Bus path is known. */
+					config->removed = TRUE;
+					config_unref (config);
+					return;
+				}
 				g_dbus_proxy_call (priv->pacrunner,
 				                   "DestroyProxyConfiguration",
-				                   g_variant_new ("(o)", data->path),
+				                   g_variant_new ("(o)", config->path),
 				                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
 				                   -1,
-				                   NULL,
-				                  (GAsyncReadyCallback) pacrunner_remove_done,
-				                   self);
-			}
-			break;
+				                   priv->pacrunner_cancellable,
+				                   (GAsyncReadyCallback) pacrunner_remove_done,
+				                   config);
+			} else
+				config_unref (config);
+			priv->configs = g_list_delete_link (priv->configs, list);
+			return;
 		}
 	}
+	/* bug, remove() should always match a previous send() for a given tag */
+	g_return_if_reached ();
 }
 
 /*****************************************************************************/
@@ -449,16 +515,11 @@ dispose (GObject *object)
 	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE ((NMPacrunnerManager *) object);
 
 	g_clear_pointer (&priv->iface, g_free);
-
 	nm_clear_g_cancellable (&priv->pacrunner_cancellable);
-
 	g_clear_object (&priv->pacrunner);
 
-	g_list_free_full (priv->args, (GDestroyNotify) g_variant_unref);
-	priv->args = NULL;
-
-	g_list_free_full (priv->remove, (GDestroyNotify) remove_data_destroy);
-	priv->remove = NULL;
+	g_list_free_full (priv->configs, (GDestroyNotify) config_unref);
+	priv->configs = NULL;
 
 	G_OBJECT_CLASS (nm_pacrunner_manager_parent_class)->dispose (object);
 }
diff --git a/src/nm-pacrunner-manager.h b/src/nm-pacrunner-manager.h
index 99e8511580..4f6ad15857 100644
--- a/src/nm-pacrunner-manager.h
+++ b/src/nm-pacrunner-manager.h
@@ -36,10 +36,11 @@ NMPacrunnerManager *nm_pacrunner_manager_get (void);
 
 void nm_pacrunner_manager_send (NMPacrunnerManager *self,
                                 const char *iface,
+                                const char *tag,
                                 NMProxyConfig *proxy_config,
                                 NMIP4Config *ip4_config,
                                 NMIP6Config *ip6_config);
 
-void nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *iface);
+void nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag);
 
 #endif /* __NETWORKMANAGER_PACRUNNER_MANAGER_H__ */
diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c
index 3d4536e702..41f32f2552 100644
--- a/src/vpn/nm-vpn-connection.c
+++ b/src/vpn/nm-vpn-connection.c
@@ -481,6 +481,7 @@ _set_vpn_state (NMVpnConnection *self,
 	VpnState old_vpn_state;
 	NMVpnConnectionState new_external_state, old_external_state;
 	NMDevice *parent_dev = nm_active_connection_get_device (NM_ACTIVE_CONNECTION (self));
+	NMConnection *applied;
 
 	g_return_if_fail (NM_IS_VPN_CONNECTION (self));
 
@@ -551,13 +552,15 @@ _set_vpn_state (NMVpnConnection *self,
 		}
 		break;
 	case STATE_ACTIVATED:
+		applied = _get_applied_connection (self);
+
 		/* Secrets no longer needed now that we're connected */
 		nm_active_connection_clear_secrets (NM_ACTIVE_CONNECTION (self));
 
 		/* Let dispatcher scripts know we're up and running */
 		nm_dispatcher_call_vpn (DISPATCHER_ACTION_VPN_UP,
 		                        _get_settings_connection (self, FALSE),
-		                        _get_applied_connection (self),
+		                        applied,
 		                        parent_dev,
 		                        priv->ip_iface,
 		                        priv->proxy_config,
@@ -570,16 +573,18 @@ _set_vpn_state (NMVpnConnection *self,
 		if (priv->proxy_config) {
 			nm_pacrunner_manager_send (nm_pacrunner_manager_get (),
 			                           priv->ip_iface,
+			                           nm_connection_get_uuid (applied),
 			                           priv->proxy_config,
 			                           priv->ip4_config,
 			                           priv->ip6_config);
 		}
 		break;
 	case STATE_DEACTIVATING:
+		applied = _get_applied_connection (self);
 		if (quitting) {
 			nm_dispatcher_call_vpn_sync (DISPATCHER_ACTION_VPN_PRE_DOWN,
 			                             _get_settings_connection (self, FALSE),
-			                             _get_applied_connection (self),
+			                             applied,
 			                             parent_dev,
 			                             priv->ip_iface,
 			                             priv->proxy_config,
@@ -588,7 +593,7 @@ _set_vpn_state (NMVpnConnection *self,
 		} else {
 			if (!nm_dispatcher_call_vpn (DISPATCHER_ACTION_VPN_PRE_DOWN,
 			                             _get_settings_connection (self, FALSE),
-			                             _get_applied_connection (self),
+			                             applied,
 			                             parent_dev,
 			                             priv->ip_iface,
 			                             priv->proxy_config,
@@ -603,7 +608,7 @@ _set_vpn_state (NMVpnConnection *self,
 		}
 
 		/* Remove config from PacRunner */
-		nm_pacrunner_manager_remove (nm_pacrunner_manager_get(), priv->ip_iface);
+		nm_pacrunner_manager_remove (nm_pacrunner_manager_get(), nm_connection_get_uuid (applied));
 		break;
 	case STATE_FAILED:
 	case STATE_DISCONNECTED: