proxy: introduce call-id for clearing pacmanager configuration

nm_pacrunner_manager_remove() required a "tag" argument. It was a
bug for callers trying to remove a configuration for a non-existing
tag.

That effectively means, the caller must keep track of whether a certain
"tag" is pending. The caller also must remember the tag -- a tag that he
must choose uniquely in the first place.

Turn that around and have nm_pacrunner_manager_send() return a (non
NULL) call-id. This call-id may later be used to remove the
configuration.

Apparently, previously the tracking of the "tag" was not always correct
and we hit the assertion in nm_pacrunner_manager_remove().

https://bugzilla.redhat.com/show_bug.cgi?id=1444374

src/devices/nm-device.c

@@ -323,7 +323,7 @@ typedef struct _NMDevicePrivate {
 	/* Proxy Configuration */
 	NMProxyConfig *proxy_config;
 	NMPacrunnerManager *pacrunner_manager;
-	bool proxy_config_sent;
+	NMPacrunnerCallId *pacrunner_call_id;
 
 	/* IP4 configuration info */
 	NMIP4Config *   ip4_config;     /* Combined config from VPN, settings, and device */
@@ -8879,24 +8879,33 @@ nm_device_reactivate_ip6_config (NMDevice *self,
 	}
 }
 
+static void
+_pacrunner_manager_send (NMDevice *self)
+{
+	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
+
+	nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
+	                                   &priv->pacrunner_call_id);
+
+	if (!priv->pacrunner_manager)
+		priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ());
+
+	priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager,
+	                                                     nm_device_get_ip_iface (self),
+	                                                     priv->proxy_config,
+	                                                     NULL,
+	                                                     NULL);
+}
+
 static void
 reactivate_proxy_config (NMDevice *self)
 {
 	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
 
-	if (!priv->proxy_config_sent)
+	if (!priv->pacrunner_call_id)
 		return;
-
-	nm_pacrunner_manager_remove (priv->pacrunner_manager,
-	                             nm_device_get_ip_iface (self));
-
 	nm_device_set_proxy_config (self, priv->dhcp4.pac_url);
-	nm_pacrunner_manager_send (priv->pacrunner_manager,
+	_pacrunner_manager_send (self);
-	                           nm_device_get_ip_iface (self),
-	                           nm_device_get_ip_iface (self),
-	                           priv->proxy_config,
-	                           NULL,
-	                           NULL);
 }
 
 static gboolean
@@ -12565,11 +12574,8 @@ _set_state_full (NMDevice *self,
 			}
 		}
 
-		if (priv->proxy_config_sent) {
+		nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
-			nm_pacrunner_manager_remove (priv->pacrunner_manager,
+		                                   &priv->pacrunner_call_id);
-			                             nm_device_get_ip_iface (self));
-			priv->proxy_config_sent = FALSE;
-		}
 		break;
 	case NM_DEVICE_STATE_DISCONNECTED:
 		if (   priv->queued_act_request
@@ -12594,15 +12600,8 @@ _set_state_full (NMDevice *self,
 		                           req,
 		                           NULL, NULL, NULL);
 
-		if (priv->proxy_config) {
+		if (priv->proxy_config)
-			nm_pacrunner_manager_send (priv->pacrunner_manager,
+			_pacrunner_manager_send (self);
-			                           nm_device_get_ip_iface (self),
-			                           nm_device_get_ip_iface (self),
-			                           priv->proxy_config,
-			                           NULL,
-			                           NULL);
-			priv->proxy_config_sent = TRUE;
-		}
 		break;
 	case NM_DEVICE_STATE_FAILED:
 		/* Usually upon failure the activation chain is interrupted in
@@ -13608,8 +13607,6 @@ nm_device_init (NMDevice *self)
 	priv->ip6_saved_properties = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free);
 	priv->sys_iface_state = NM_DEVICE_SYS_IFACE_STATE_EXTERNAL;
 
-	priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ());
-
 	priv->default_route.v4_is_assumed = TRUE;
 	priv->default_route.v6_is_assumed = TRUE;
 
@@ -13734,6 +13731,8 @@ dispose (GObject *object)
 
 	dispatcher_cleanup (self);
 
+	nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
+	                                   &priv->pacrunner_call_id);
 	g_clear_object (&priv->pacrunner_manager);
 
 	_cleanup_generic_pre (self, CLEANUP_TYPE_KEEP);

src/nm-pacrunner-manager.c

@@ -36,14 +36,15 @@ static void pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointe
 
 /*****************************************************************************/
 
-typedef struct {
+struct _NMPacrunnerCallId {
-	char *tag;
 	NMPacrunnerManager *manager;
 	GVariant *args;
 	char *path;
 	guint refcount;
 	bool removed;
-} Config;
+};
+
+typedef struct _NMPacrunnerCallId Config;
 
 typedef struct {
 	char *iface;
@@ -77,13 +78,12 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP
 /*****************************************************************************/
 
 static Config *
-config_new (NMPacrunnerManager *manager, char *tag, GVariant *args)
+config_new (NMPacrunnerManager *manager, GVariant *args)
 {
 	Config *config;
 
 	config = g_slice_new0 (Config);
 	config->manager = manager;
-	config->tag = tag;
 	config->args = g_variant_ref_sink (args);
 	config->refcount = 1;
 
@@ -106,7 +106,6 @@ config_unref (Config *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);
@@ -233,12 +232,12 @@ pacrunner_send_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
 	if (!variant) {
-		_LOGD ("send config for '%s' failed: %s", config->tag, error->message);
+		_LOGD ("send config for '%p' failed: %s", config, error->message);
 	} else {
 		g_variant_get (variant, "(&o)", &path);
 
 		config->path = g_strdup (path);
-		_LOGD ("successfully sent config for '%s'", config->tag);
+		_LOGD ("successfully sent config for '%p'", config);
 
 		if (config->removed) {
 			g_dbus_proxy_call (priv->pacrunner,
@@ -262,7 +261,7 @@ pacrunner_send_config (NMPacrunnerManager *self, Config *config)
 	if (priv->pacrunner) {
 		gs_free char *args_str = NULL;
 
-		_LOGT ("sending proxy config for '%s': %s", config->tag,
+		_LOGT ("sending proxy config for '%p': %s", config,
 		       (args_str = g_variant_print (config->args, FALSE)));
 
 		config_ref (config);
@@ -328,15 +327,21 @@ 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 to extract domain info from
  * @ip6_config: IP6 config of the connection to extract domain info from
+ *
+ * Returns: a #NMPacrunnerCallId call id. The function cannot
+ *  fail and always returns a non NULL pointer. The call-id may
+ *  be used to remove the configuration later via nm_pacrunner_manager_remove().
+ *  Note that the call-id does not keep the @self instance alive.
+ *  If you plan to remove the configuration later, you must keep
+ *  the instance alive long enough. You can remove the configuration
+ *  at most once using this call call-id.
  */
-void
+NMPacrunnerCallId *
 nm_pacrunner_manager_send (NMPacrunnerManager *self,
                            const char *iface,
-                           const char *tag,
                            NMProxyConfig *proxy_config,
                            NMIP4Config *ip4_config,
                            NMIP6Config *ip6_config)
@@ -348,8 +353,8 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 	GPtrArray *domains;
 	Config *config;
 
-	g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self));
+	g_return_val_if_fail (NM_IS_PACRUNNER_MANAGER (self), NULL);
-	g_return_if_fail (proxy_config);
+	g_return_val_if_fail (proxy_config, NULL);
 
 	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
@@ -401,8 +406,7 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 		}
 	}
 
-	config = config_new (self, g_strdup (tag),
+	config = config_new (self, g_variant_new ("(a{sv})", &proxy_data));
-	                     g_variant_new ("(a{sv})", &proxy_data));
 	priv->configs = g_list_append (priv->configs, config);
 
 	/* Send if pacrunner is available on bus, otherwise
@@ -410,6 +414,8 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self,
 	 * sent when pacrunner appears.
 	 */
 	pacrunner_send_config (self, config);
+
+	return config;
 }
 
 static void
@@ -429,9 +435,9 @@ pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 	self = NM_PACRUNNER_MANAGER (config->manager);
 
 	if (!ret)
-		_LOGD ("couldn't remove config for '%s': %s", config->tag, error->message);
+		_LOGD ("couldn't remove config for '%p': %s", config, error->message);
 	else
-		_LOGD ("successfully removed config for '%s'", config->tag);
+		_LOGD ("successfully removed config for '%p'", config);
 
 	config_unref (config);
 }
@@ -439,49 +445,64 @@ pacrunner_remove_done (GDBusProxy *proxy, GAsyncResult *res, gpointer user_data)
 /**
  * nm_pacrunner_manager_remove:
  * @self: the #NMPacrunnerManager
- * @iface: the iface for the connection to be removed
+ * @call_id: the call-id obtained from nm_pacrunner_manager_send()
- * from pacrunner
  */
 void
-nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag)
+nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_id)
 {
-	NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
+	NMPacrunnerManagerPrivate *priv;
+	Config *config;
 	GList *list;
 
-	g_return_if_fail (tag);
+	g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self));
+	g_return_if_fail (call_id);
 
-	_LOGT ("removing config for '%s'", tag);
+	config = call_id;
+	priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self);
 
-	for (list = g_list_first (priv->configs); list; list = g_list_next (list)) {
+	_LOGT ("removing config for '%p'", config);
-		Config *config = list->data;
 
-		if (nm_streq (config->tag, tag)) {
+	list = g_list_find (priv->configs, config);
-			if (priv->pacrunner) {
+	if (!list)
-				if (!config->path) {
+		g_return_if_reached ();
-					/* send() failed or is still pending. Mark the item as
+
-					 * removed, so that we ask pacrunner to drop it when the
+	if (priv->pacrunner) {
-					 * send() completes.
+		if (!config->path) {
-					 */
+			/* send() failed or is still pending. Mark the item as
-					config->removed = TRUE;
+			 * removed, so that we ask pacrunner to drop it when the
-					config_unref (config);
+			 * send() completes.
-				} else {
+			 */
-					g_dbus_proxy_call (priv->pacrunner,
+			config->removed = TRUE;
-					                   "DestroyProxyConfiguration",
+			config_unref (config);
-					                   g_variant_new ("(o)", config->path),
+		} else {
-					                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
+			g_dbus_proxy_call (priv->pacrunner,
-					                   -1,
+			                   "DestroyProxyConfiguration",
-					                   priv->pacrunner_cancellable,
+			                   g_variant_new ("(o)", config->path),
-					                   (GAsyncReadyCallback) pacrunner_remove_done,
+			                   G_DBUS_CALL_FLAGS_NO_AUTO_START,
-					                   config);
+			                   -1,
-				}
+			                   priv->pacrunner_cancellable,
-			} else
+			                   (GAsyncReadyCallback) pacrunner_remove_done,
-				config_unref (config);
+			                   config);
-			priv->configs = g_list_delete_link (priv->configs, list);
-			return;
 		}
-	}
+	} else
-	/* bug, remove() should always match a previous send() for a given tag */
+		config_unref (config);
-	g_return_if_reached ();
+	priv->configs = g_list_delete_link (priv->configs, list);
+}
+
+gboolean
+nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self,
+                                   NMPacrunnerCallId **p_call_id)
+{
+	g_return_val_if_fail (p_call_id, FALSE);
+
+	/* if we have no call-id, allow for %NULL */
+	g_return_val_if_fail ((!self && !*p_call_id) || NM_IS_PACRUNNER_MANAGER (self), FALSE);
+
+	if (!*p_call_id)
+		return FALSE;
+	nm_pacrunner_manager_remove (self,
+	                             g_steal_pointer (p_call_id));
+	return TRUE;
 }
 
 /*****************************************************************************/

src/nm-pacrunner-manager.h

@@ -15,7 +15,8 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
- * (C) Copyright 2016 Atul Anand <atulhjp@gmail.com>.
+ * Copyright 2016 Atul Anand <atulhjp@gmail.com>.
+ * Copyright 2016 - 2017 Red Hat, Inc.
  */
 
 #ifndef __NETWORKMANAGER_PACRUNNER_MANAGER_H__
@@ -30,17 +31,22 @@
 
 typedef struct _NMPacrunnerManagerClass NMPacrunnerManagerClass;
 
+typedef struct _NMPacrunnerCallId NMPacrunnerCallId;
+
 GType nm_pacrunner_manager_get_type (void);
 
 NMPacrunnerManager *nm_pacrunner_manager_get (void);
 
-void nm_pacrunner_manager_send (NMPacrunnerManager *self,
+NMPacrunnerCallId *nm_pacrunner_manager_send (NMPacrunnerManager *self,
-                                const char *iface,
+                                              const char *iface,
-                                const char *tag,
+                                              NMProxyConfig *proxy_config,
-                                NMProxyConfig *proxy_config,
+                                              NMIP4Config *ip4_config,
-                                NMIP4Config *ip4_config,
+                                              NMIP6Config *ip6_config);
-                                NMIP6Config *ip6_config);
 
-void nm_pacrunner_manager_remove (NMPacrunnerManager *self, const char *tag);
+void nm_pacrunner_manager_remove (NMPacrunnerManager *self,
+                                  NMPacrunnerCallId *call_id);
+
+gboolean nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self,
+                                            NMPacrunnerCallId **p_call_id);
 
 #endif /* __NETWORKMANAGER_PACRUNNER_MANAGER_H__ */

src/vpn/nm-vpn-connection.c

@@ -128,7 +128,8 @@ typedef struct {
 	GVariant *connect_hash;
 	guint connect_timeout;
 	NMProxyConfig *proxy_config;
-	gboolean proxy_config_sent;
+	NMPacrunnerManager *pacrunner_manager;
+	NMPacrunnerCallId *pacrunner_call_id;
 	gboolean has_ip4;
 	NMIP4Config *ip4_config;
 	guint32 ip4_internal_gw;
@@ -559,13 +560,18 @@ _set_vpn_state (NMVpnConnection *self,
 		                        NULL);
 
 		if (priv->proxy_config) {
-			nm_pacrunner_manager_send (nm_pacrunner_manager_get (),
+			nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
-			                           priv->ip_iface,
+			                                   &priv->pacrunner_call_id);
-			                           nm_connection_get_uuid (applied),
+			if (!priv->pacrunner_manager) {
-			                           priv->proxy_config,
+				/* the pending call doesn't keep NMPacrunnerManager alive.
-			                           priv->ip4_config,
+				 * Take a reference to it. */
-			                           priv->ip6_config);
+				priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ());
-			priv->proxy_config_sent = TRUE;
+			}
+			priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager,
+			                                                     priv->ip_iface,
+			                                                     priv->proxy_config,
+			                                                     priv->ip4_config,
+			                                                     priv->ip6_config);
 		}
 		break;
 	case STATE_DEACTIVATING:
@@ -596,11 +602,8 @@ _set_vpn_state (NMVpnConnection *self,
 			}
 		}
 
-		if (priv->proxy_config_sent) {
+		nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
-			nm_pacrunner_manager_remove (nm_pacrunner_manager_get(),
+		                                   &priv->pacrunner_call_id);
-			                             nm_connection_get_uuid (applied));
-			priv->proxy_config_sent = FALSE;
-		}
 		break;
 	case STATE_FAILED:
 	case STATE_DISCONNECTED:
@@ -2649,6 +2652,10 @@ dispose (GObject *object)
 
 	fw_call_cleanup (self);
 
+	nm_pacrunner_manager_remove_clear (priv->pacrunner_manager,
+	                                   &priv->pacrunner_call_id);
+	g_clear_object (&priv->pacrunner_manager);
+
 	G_OBJECT_CLASS (nm_vpn_connection_parent_class)->dispose (object);
 }