fixup! firewall: refactor callback handling in NMFirewallManagerth/firewall-callback-bgo755539

Always take a reference to the manager. The manager should stay alive
as long as there are pending operations.

On shutdown, NMDevices are not destroyed. Thus, if we destroy the
firewall manager with pending operations, their callback will execute
again on the device, which might be unexpected.
With this, we easily leak NMFirewallManager on shutdown.

4 files changed, 24 insertions, 62 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 783d14282c..a1b78ee6be 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -5592,7 +5592,6 @@ nm_device_activate_schedule_stage3_ip_config_start (NMDevice *self)
 	                                                        nm_device_get_ip_iface (self),
 	                                                        zone,
 	                                                        FALSE,
-	                                                        TRUE,
 	                                                        fw_change_zone_cb,
 	                                                        self);
 }
@@ -7940,7 +7939,6 @@ nm_device_update_firewall_zone (NMDevice *self)
 		                                        nm_device_get_ip_iface (self),
 		                                        nm_setting_connection_get_zone (s_con),
 		                                        FALSE, /* change zone */
-		                                        TRUE,
 		                                        NULL,
 		                                        NULL);
 	}
@@ -8343,7 +8341,6 @@ _cleanup_generic_pre (NMDevice *self, CleanupType cleanup_type)
 		nm_firewall_manager_remove_from_zone (nm_firewall_manager_get (),
 		                                      nm_device_get_ip_iface (self),
 		                                      NULL,
-		                                      TRUE,
 		                                      NULL,
 		                                      NULL);
 	}
diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c
index a42ebe3791..8dd8db2dde 100644
--- a/src/nm-firewall-manager.c
+++ b/src/nm-firewall-manager.c
@@ -73,7 +73,6 @@ typedef enum {
 
 struct _NMFirewallManagerCallId {
 	NMFirewallManager *self;
-	gboolean keep_mgr_alive;
 	CBInfoOpsType ops_type;
 	CBInfoMode mode;
 	char *iface;
@@ -145,7 +144,6 @@ static CBInfo *
 _cb_info_create (NMFirewallManager *self,
                  CBInfoOpsType ops_type,
                  const char *iface,
-                 gboolean keep_mgr_alive,
                  NMFirewallManagerAddRemoveCallback callback,
                  gpointer user_data)
 {
@@ -153,16 +151,12 @@ _cb_info_create (NMFirewallManager *self,
 	CBInfo *info;
 
 	info = g_slice_new0 (CBInfo);
-	info->self = self;
-	info->keep_mgr_alive = keep_mgr_alive;
+	info->self = g_object_ref (self);
 	info->ops_type = ops_type;
 	info->iface = g_strdup (iface);
 	info->callback = callback;
 	info->user_data = user_data;
 
-	if (keep_mgr_alive)
-		g_object_ref (self);
-
 	if (priv->running) {
 		info->mode = CB_INFO_MODE_DBUS;
 		info->dbus.cancellable = g_cancellable_new ();
@@ -181,7 +175,7 @@ _cb_info_free (CBInfo *info)
 	if (!_cb_info_is_idle (info))
 		g_object_unref (info->dbus.cancellable);
 	g_free (info->iface);
-	if (info->keep_mgr_alive)
+	if (info->self)
 		g_object_unref (info->self);
 	g_slice_free (CBInfo, info);
 }
@@ -206,31 +200,6 @@ _cb_info_complete_normal (CBInfo *info, GError *error)
 	_cb_info_free (info);
 }
 
-static void
-_cb_info_complete_cancel (CBInfo *info, gboolean is_disposing)
-{
-	NMFirewallManager *self = info->self;
-	gs_free_error GError *error = NULL;
-
-	nm_utils_error_set_cancelled (&error, is_disposing, "NMFirewallManager");
-
-	_LOGD (info, "complete: cancel (%s)", error->message);
-
-	_cb_info_callback (info, error);
-
-	if (_cb_info_is_idle (info)) {
-		g_source_remove (info->idle.id);
-		_cb_info_free (info);
-	} else {
-		info->mode = CB_INFO_MODE_DBUS_COMPLETED;
-		g_cancellable_cancel (info->dbus.cancellable);
-		if (info->keep_mgr_alive) {
-			info->keep_mgr_alive = FALSE;
-			g_object_unref (self);
-		}
-	}
-}
-
 static gboolean
 _handle_idle (gpointer user_data)
 {
@@ -298,7 +267,6 @@ _start_request (NMFirewallManager *self,
                 CBInfoOpsType ops_type,
                 const char *iface,
                 const char *zone,
-                gboolean keep_mgr_alive,
                 NMFirewallManagerAddRemoveCallback callback,
                 gpointer user_data)
 {
@@ -311,7 +279,7 @@ _start_request (NMFirewallManager *self,
 
 	priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
 
-	info = _cb_info_create (self, ops_type, iface, keep_mgr_alive, callback, user_data);
+	info = _cb_info_create (self, ops_type, iface, callback, user_data);
 
 	_LOGD (info, "firewall zone %s %s:%s%s%s%s",
 	       _ops_type_to_string (info->ops_type),
@@ -370,7 +338,6 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,
                                         const char *iface,
                                         const char *zone,
                                         gboolean add, /* TRUE == add, FALSE == change */
-                                        gboolean keep_mgr_alive,
                                         NMFirewallManagerAddRemoveCallback callback,
                                         gpointer user_data)
 {
@@ -378,7 +345,6 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,
 	                       add ? CB_INFO_OPS_ADD : CB_INFO_OPS_CHANGE,
 	                       iface,
 	                       zone,
-	                       keep_mgr_alive,
 	                       callback,
 	                       user_data);
 }
@@ -387,7 +353,6 @@ NMFirewallManagerCallId
 nm_firewall_manager_remove_from_zone (NMFirewallManager *self,
                                       const char *iface,
                                       const char *zone,
-                                      gboolean keep_mgr_alive,
                                       NMFirewallManagerAddRemoveCallback callback,
                                       gpointer user_data)
 {
@@ -395,7 +360,6 @@ nm_firewall_manager_remove_from_zone (NMFirewallManager *self,
 	                       CB_INFO_OPS_REMOVE,
 	                       iface,
 	                       zone,
-	                       keep_mgr_alive,
 	                       callback,
 	                       user_data);
 }
@@ -403,18 +367,34 @@ nm_firewall_manager_remove_from_zone (NMFirewallManager *self,
 void
 nm_firewall_manager_cancel_call (NMFirewallManagerCallId call)
 {
+	NMFirewallManager *self;
 	NMFirewallManagerPrivate *priv;
 	CBInfo *info = call;
+	gs_free_error GError *error = NULL;
 
 	g_return_if_fail (info);
 	g_return_if_fail (NM_IS_FIREWALL_MANAGER (info->self));
 
-	priv = NM_FIREWALL_MANAGER_GET_PRIVATE (info->self);
+	self = info->self;
+	priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
 
 	if (!g_hash_table_remove (priv->pending_calls, info))
 		g_return_if_reached ();
 
-	_cb_info_complete_cancel (info, FALSE);
+	nm_utils_error_set_cancelled (&error, FALSE, "NMFirewallManager");
+
+	_LOGD (info, "complete: cancel (%s)", error->message);
+
+	_cb_info_callback (info, error);
+
+	if (_cb_info_is_idle (info)) {
+		g_source_remove (info->idle.id);
+		_cb_info_free (info);
+	} else {
+		info->mode = CB_INFO_MODE_DBUS_COMPLETED;
+		g_cancellable_cancel (info->dbus.cancellable);
+		g_clear_object (&info->self);
+	}
 }
 
 /*******************************************************************/
@@ -502,22 +482,11 @@ dispose (GObject *object)
 {
 	NMFirewallManager *self = NM_FIREWALL_MANAGER (object);
 	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
-	GHashTableIter iter;
 
 	if (priv->pending_calls) {
-		CBInfo *info;
-
-		/* we don't really expect any pending calls at this point because users
-		 * should keep the firewall manager alive as long as there are pending calls.
-		 * Anyway, cancel them now. */
-cancel_more:
-		g_hash_table_iter_init (&iter, priv->pending_calls);
-		if (g_hash_table_iter_next (&iter, (gpointer *) &info, NULL)) {
-			g_hash_table_iter_remove (&iter);
-			_cb_info_complete_cancel (info, TRUE);
-			/* restart iterating, the user might cancelled another call and modified the hash-table */
-			goto cancel_more;
-		}
+		/* as every pending operation takes a reference to the manager,
+		 * we don't expect pending operations at this point. */
+		g_assert (g_hash_table_size (priv->pending_calls) == 0);
 		g_hash_table_unref (priv->pending_calls);
 		priv->pending_calls = NULL;
 	}
diff --git a/src/nm-firewall-manager.h b/src/nm-firewall-manager.h
index 80970374d2..dfa83b459e 100644
--- a/src/nm-firewall-manager.h
+++ b/src/nm-firewall-manager.h
@@ -67,13 +67,11 @@ NMFirewallManagerCallId nm_firewall_manager_add_or_change_zone (NMFirewallManage
                                                                 const char *iface,
                                                                 const char *zone,
                                                                 gboolean add,
-                                                                gboolean keep_mgr_alive,
                                                                 NMFirewallManagerAddRemoveCallback callback,
                                                                 gpointer user_data);
 NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *mgr,
                                                               const char *iface,
                                                               const char *zone,
-                                                              gboolean keep_mgr_alive,
                                                               NMFirewallManagerAddRemoveCallback callback,
                                                               gpointer user_data);
 
diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c
index 132e2cd9e8..a520dfd43e 100644
--- a/src/vpn-manager/nm-vpn-connection.c
+++ b/src/vpn-manager/nm-vpn-connection.c
@@ -348,7 +348,6 @@ vpn_cleanup (NMVpnConnection *self, NMDevice *parent_dev)
 		nm_firewall_manager_remove_from_zone (nm_firewall_manager_get (),
 		                                      priv->ip_iface,
 		                                      NULL,
-		                                      TRUE,
 		                                      NULL,
 		                                      NULL);
 	}
@@ -1163,7 +1162,6 @@ nm_vpn_connection_config_maybe_complete (NMVpnConnection *self,
 			                                                        priv->ip_iface,
 			                                                        zone,
 			                                                        FALSE,
-			                                                        TRUE,
 			                                                        fw_change_zone_cb,
 			                                                        self);
 			return;