firewall: merge branch 'th/firewall-callback-bgo755539'

https://bugzilla.gnome.org/show_bug.cgi?id=755539

11 files changed, 444 insertions, 228 deletions

diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c
index d78313df50..d5bb6c87f0 100644
--- a/src/NetworkManagerUtils.c
+++ b/src/NetworkManagerUtils.c
@@ -110,6 +110,39 @@ _nm_utils_set_testing (NMUtilsTestFlags flags)
 
 /*****************************************************************************/
 
+G_DEFINE_QUARK (nm-utils-error-quark, nm_utils_error)
+
+void
+nm_utils_error_set_cancelled (GError **error,
+                              gboolean is_disposing,
+                              const char *instance_name)
+{
+	if (is_disposing) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING,
+		             "Disposing %s instance",
+		             instance_name && *instance_name ? instance_name : "source");
+	} else {
+		g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
+		                     "Request cancelled");
+	}
+}
+
+gboolean
+nm_utils_error_is_cancelled (GError *error,
+                             gboolean consider_is_disposing)
+{
+	if (error) {
+		if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+			return TRUE;
+		if (   consider_is_disposing
+		    && g_error_matches (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING))
+			return TRUE;
+	}
+	return FALSE;
+}
+
+/*****************************************************************************/
+
 static GSList *_singletons = NULL;
 static gboolean _singletons_shutdown = FALSE;
 
diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h
index df4b35b14c..3af93e5a03 100644
--- a/src/NetworkManagerUtils.h
+++ b/src/NetworkManagerUtils.h
@@ -89,6 +89,31 @@ GETTER (void) \
 
 /*****************************************************************************/
 
+/**
+ * NMUtilsError:
+ * @NM_UTILS_ERROR_UNKNOWN: unknown or unclassified error
+ * @NM_UTILS_ERROR_CANCELLED_DISPOSING: when disposing an object that has
+ *   pending aynchronous operations, the operation is cancelled with this
+ *   error reason. Depending on the usage, this might indicate a bug because
+ *   usually the target object should stay alive as long as there are pending
+ *   operations.
+ */
+typedef enum {
+	NM_UTILS_ERROR_UNKNOWN = 0,                 /*< nick=Unknown >*/
+	NM_UTILS_ERROR_CANCELLED_DISPOSING,         /*< nick=CancelledDisposing >*/
+} NMUtilsError;
+
+#define NM_UTILS_ERROR (nm_utils_error_quark ())
+GQuark nm_utils_error_quark (void);
+
+void nm_utils_error_set_cancelled (GError **error,
+                                   gboolean is_disposing,
+                                   const char *instance_name);
+gboolean nm_utils_error_is_cancelled (GError *error,
+                                      gboolean consider_is_disposing);
+
+/*****************************************************************************/
+
 gboolean nm_ethernet_address_is_valid (gconstpointer addr, gssize len);
 
 in_addr_t nm_utils_ip4_address_clear_host_address (in_addr_t addr, guint8 plen);
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 928481ec12..a1b78ee6be 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -291,7 +291,7 @@ typedef struct {
 	gulong            dnsmasq_state_id;
 
 	/* Firewall */
-	NMFirewallPendingCall fw_call;
+	NMFirewallManagerCallId fw_call;
 
 	/* IPv4LL stuff */
 	sd_ipv4ll *    ipv4ll;
@@ -5527,19 +5527,24 @@ out:
 
 
 static void
-fw_change_zone_cb (GError *error, gpointer user_data)
+fw_change_zone_cb (NMFirewallManager *firewall_manager,
+                   NMFirewallManagerCallId call_id,
+                   GError *error,
+                   gpointer user_data)
 {
-	NMDevice *self;
+	NMDevice *self = user_data;
 	NMDevicePrivate *priv;
 
-	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-		return;
+	g_return_if_fail (NM_IS_DEVICE (self));
 
-	self = NM_DEVICE (user_data);
 	priv = NM_DEVICE_GET_PRIVATE (self);
 
+	g_return_if_fail (priv->fw_call == call_id);
 	priv->fw_call = NULL;
 
+	if (nm_utils_error_is_cancelled (error, FALSE))
+		return;
+
 	if (error) {
 		/* FIXME: fail the device activation? */
 	}
@@ -8310,7 +8315,8 @@ _cancel_activation (NMDevice *self)
 
 	/* Clean up when device was deactivated during call to firewall */
 	if (priv->fw_call) {
-		nm_firewall_manager_cancel_call (nm_firewall_manager_get (), priv->fw_call);
+		nm_firewall_manager_cancel_call (priv->fw_call);
+		g_warn_if_fail (!priv->fw_call);
 		priv->fw_call = NULL;
 	}
 
@@ -8334,6 +8340,8 @@ _cleanup_generic_pre (NMDevice *self, CleanupType cleanup_type)
 	    && !nm_device_uses_assumed_connection (self)) {
 		nm_firewall_manager_remove_from_zone (nm_firewall_manager_get (),
 		                                      nm_device_get_ip_iface (self),
+		                                      NULL,
+		                                      NULL,
 		                                      NULL);
 	}
 
diff --git a/src/nm-activation-request.c b/src/nm-activation-request.c
index 173dc19fd7..e6645bdbc4 100644
--- a/src/nm-activation-request.c
+++ b/src/nm-activation-request.c
@@ -216,18 +216,7 @@ _do_cancel_secrets (NMActRequest *self, GetSecretsInfo *info, gboolean is_dispos
 	if (info->callback) {
 		gs_free_error GError *error = NULL;
 
-		if (is_disposing) {
-			/* Use a different error code. G_IO_ERROR_CANCELLED is only used synchronously
-			 * when the user calls nm_act_request_cancel_secrets(). Disposing the instance
-			 * with pending requests also cancels the requests, but with a different error
-			 * code. */
-			g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED,
-			                     "Disposing NMActRequest instance");
-		} else {
-			g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
-			                     "Request cancelled");
-		}
-
+		nm_utils_error_set_cancelled (&error, is_disposing, "NMActRequest");
 		info->callback (self, info, NULL, error, info->callback_data);
 	}
 
diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c
index 46a011205e..bc519eb199 100644
--- a/src/nm-auth-manager.c
+++ b/src/nm-auth-manager.c
@@ -175,9 +175,8 @@ check_authorization_cb (GDBusProxy *proxy,
 	value = _nm_dbus_proxy_call_finish (proxy, res, G_VARIANT_TYPE ("((bba{ss}))"), &error);
 	if (value == NULL) {
 		if (data->cancellation_id != NULL &&
-		    (!g_dbus_error_is_remote_error (error) &&
-		     error->domain == G_IO_ERROR &&
-		     error->code == G_IO_ERROR_CANCELLED)) {
+		    (   g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)
+		     && !g_dbus_error_is_remote_error (error))) {
 			_LOGD ("call[%u]: CheckAuthorization cancelled", data->call_id);
 			g_dbus_proxy_call (priv->proxy,
 			                   "CancelCheckAuthorization",
diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c
index 93792b8799..8dd8db2dde 100644
--- a/src/nm-firewall-manager.c
+++ b/src/nm-firewall-manager.c
@@ -15,7 +15,7 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *
- * Copyright (C) 2011 Red Hat, Inc.
+ * Copyright (C) 2011 - 2015 Red Hat, Inc.
  */
 
 #include "config.h"
@@ -24,6 +24,7 @@
 
 #include "nm-default.h"
 #include "nm-firewall-manager.h"
+#include "gsystem-local-alloc.h"
 #include "NetworkManagerUtils.h"
 
 #define NM_FIREWALL_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), \
@@ -43,7 +44,7 @@ typedef struct {
 	GDBusProxy *    proxy;
 	gboolean        running;
 
-	GSList         *pending_calls;
+	GHashTable     *pending_calls;
 } NMFirewallManagerPrivate;
 
 enum {
@@ -54,190 +55,350 @@ enum {
 
 static guint signals[LAST_SIGNAL] = { 0 };
 
+NM_DEFINE_SINGLETON_GETTER (NMFirewallManager, nm_firewall_manager_get, NM_TYPE_FIREWALL_MANAGER);
+
 /********************************************************************/
 
-#define PENDING_CALL_DUMMY               ((NMFirewallPendingCall) GUINT_TO_POINTER(1))
-#define PENDING_CALL_FROM_INFO(info)     ((NMFirewallPendingCall) info)
+typedef enum {
+	CB_INFO_OPS_ADD = 1,
+	CB_INFO_OPS_CHANGE,
+	CB_INFO_OPS_REMOVE,
+} CBInfoOpsType;
 
-typedef struct {
+typedef enum {
+	CB_INFO_MODE_IDLE = 1,
+	CB_INFO_MODE_DBUS,
+	CB_INFO_MODE_DBUS_COMPLETED,
+} CBInfoMode;
+
+struct _NMFirewallManagerCallId {
 	NMFirewallManager *self;
+	CBInfoOpsType ops_type;
+	CBInfoMode mode;
 	char *iface;
-	FwAddToZoneFunc callback;
+	NMFirewallManagerAddRemoveCallback callback;
 	gpointer user_data;
-	guint id;
 
-	guint idle_id;
-	GCancellable *cancellable;
-} CBInfo;
-
-static void
-_cb_info_complete_and_free (CBInfo *info,
-                            const char *tag,
-                            const char *debug_error_match,
-                            GError *error)
-{
-	gs_free_error GError *local = NULL;
+	union {
+		struct {
+			GCancellable *cancellable;
+		} dbus;
+		struct {
+			guint id;
+		} idle;
+	};
+};
+typedef struct _NMFirewallManagerCallId CBInfo;
 
-	g_return_if_fail (info != NULL);
-	g_return_if_fail (tag != NULL);
+/********************************************************************/
 
-	/* A cancelled idle call won't set the error; catch that here */
-	if (!error && g_cancellable_is_cancelled (info->cancellable)) {
-		error = local = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_CANCELLED,
-		                                     "Operation was cancelled");
+static const char *
+_ops_type_to_string (CBInfoOpsType ops_type)
+{
+	switch (ops_type) {
+	case CB_INFO_OPS_ADD:    return "add";
+	case CB_INFO_OPS_REMOVE: return "remove";
+	case CB_INFO_OPS_CHANGE: return "change";
+	default: g_return_val_if_reached ("unknown");
 	}
+}
 
-	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
-		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s call cancelled [%u]",
-		            info->iface, tag, info->id);
-	} else if (error) {
-		g_dbus_error_strip_remote_error (error);
-		if (!g_strcmp0 (error->message, debug_error_match)) {
-			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s failed [%u]: %s",
-			            info->iface, tag, info->id, error->message);
-		} else {
-			nm_log_warn (LOGD_FIREWALL, "(%s) firewall zone %s failed [%u]: %s",
-			             info->iface, tag, info->id, error->message);
-		}
-	} else {
-		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s succeeded [%u]",
-		            info->iface, tag, info->id);
-	}
+#define _NMLOG_DOMAIN      LOGD_FIREWALL
+#define _NMLOG_PREFIX_NAME "firewall"
+#define _NMLOG(level, info, ...) \
+    G_STMT_START { \
+        if (nm_logging_enabled ((level), (_NMLOG_DOMAIN))) { \
+            CBInfo *__info = (info); \
+            char __prefix_name[30]; \
+            char __prefix_info[64]; \
+            \
+            _nm_log ((level), (_NMLOG_DOMAIN), 0, \
+                     "%s: %s" _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \
+                     (self) != singleton_instance \
+                        ? ({ \
+                                g_snprintf (__prefix_name, sizeof (__prefix_name), "%s[%p]", ""_NMLOG_PREFIX_NAME, (self)); \
+                                __prefix_name; \
+                           }) \
+                        : _NMLOG_PREFIX_NAME, \
+                     __info \
+                        ? ({ \
+                                g_snprintf (__prefix_info, sizeof (__prefix_info), "[%p,%s%s:%s%s%s]: ", __info, \
+                                            _ops_type_to_string (__info->ops_type), _cb_info_is_idle (__info) ? "*" : "", \
+                                            NM_PRINT_FMT_QUOTE_STRING (__info->iface)); \
+                                __prefix_info; \
+                           }) \
+                        : "" \
+                     _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
+        } \
+    } G_STMT_END
 
-	if (info->callback)
-		info->callback (error, info->user_data);
+/********************************************************************/
 
-	g_free (info->iface);
-	g_object_unref (info->cancellable);
-	g_slice_free (CBInfo, info);
+static gboolean
+_cb_info_is_idle (CBInfo *info)
+{
+	return info->mode == CB_INFO_MODE_IDLE;
 }
 
 static CBInfo *
-_cb_info_create (NMFirewallManager *self, const char *iface, FwAddToZoneFunc callback, gpointer user_data)
+_cb_info_create (NMFirewallManager *self,
+                 CBInfoOpsType ops_type,
+                 const char *iface,
+                 NMFirewallManagerAddRemoveCallback callback,
+                 gpointer user_data)
 {
 	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
-	static guint id = 1;
 	CBInfo *info;
 
 	info = g_slice_new0 (CBInfo);
 	info->self = g_object_ref (self);
-	info->id = id++;
+	info->ops_type = ops_type;
 	info->iface = g_strdup (iface);
-	info->cancellable = g_cancellable_new ();
 	info->callback = callback;
 	info->user_data = user_data;
 
-	priv->pending_calls = g_slist_prepend (priv->pending_calls, info);
+	if (priv->running) {
+		info->mode = CB_INFO_MODE_DBUS;
+		info->dbus.cancellable = g_cancellable_new ();
+	} else
+		info->mode = CB_INFO_MODE_IDLE;
+
+	if (!g_hash_table_add (priv->pending_calls, info))
+		g_return_val_if_reached (NULL);
+
 	return info;
 }
 
+static void
+_cb_info_free (CBInfo *info)
+{
+	if (!_cb_info_is_idle (info))
+		g_object_unref (info->dbus.cancellable);
+	g_free (info->iface);
+	if (info->self)
+		g_object_unref (info->self);
+	g_slice_free (CBInfo, info);
+}
+
+static void
+_cb_info_callback (CBInfo *info,
+                   GError *error)
+{
+	if (info->callback)
+		info->callback (info->self, info, error, info->user_data);
+}
+
+static void
+_cb_info_complete_normal (CBInfo *info, GError *error)
+{
+	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (info->self);
+
+	if (!g_hash_table_remove (priv->pending_calls, info))
+		g_return_if_reached ();
+
+	_cb_info_callback (info, error);
+	_cb_info_free (info);
+}
+
 static gboolean
-add_or_change_idle_cb (gpointer user_data)
+_handle_idle (gpointer user_data)
 {
+	NMFirewallManager *self;
 	CBInfo *info = user_data;
 
-	info->idle_id = 0;
-	_cb_info_complete_and_free (info, "idle call", NULL, NULL);
+	nm_assert (info && NM_IS_FIREWALL_MANAGER (info->self));
+
+	self = info->self;
+
+	_LOGD (info, "complete: fake success");
+
+	_cb_info_complete_normal (info, NULL);
 	return G_SOURCE_REMOVE;
 }
 
 static void
-add_or_change_cb (GObject *proxy, GAsyncResult *result, gpointer user_data)
+_handle_dbus (GObject *proxy, GAsyncResult *result, gpointer user_data)
 {
+	NMFirewallManager *self;
 	CBInfo *info = user_data;
 	gs_free_error GError *error = NULL;
 	gs_unref_variant GVariant *ret = NULL;
 
+	if (info->mode != CB_INFO_MODE_DBUS) {
+		_cb_info_free (info);
+		return;
+	}
+
+	self = info->self;
+
 	ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), result, &error);
-	_cb_info_complete_and_free (info, "add/change", "ZONE_ALREADY_SET", error);
+
+	if (error) {
+		const char *non_error = NULL;
+
+		g_dbus_error_strip_remote_error (error);
+
+		switch (info->ops_type) {
+		case CB_INFO_OPS_ADD:
+		case CB_INFO_OPS_CHANGE:
+			non_error = "ZONE_ALREADY_SET";
+			break;
+		case CB_INFO_OPS_REMOVE:
+			non_error = "UNKNOWN_INTERFACE";
+			break;
+		}
+		if (!g_strcmp0 (error->message, non_error)) {
+			_LOGD (info, "complete: request failed with a non-error (%s)", error->message);
+
+			/* The operation failed with an error reason that we don't want
+			 * to propagate. Instead, signal success. */
+			g_clear_error (&error);
+		}
+		else
+			_LOGW (info, "complete: request failed (%s)", error->message);
+	} else
+		_LOGD (info, "complete: success");
+
+	_cb_info_complete_normal (info, error);
 }
 
-NMFirewallPendingCall
-nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,
-                                        const char *iface,
-                                        const char *zone,
-                                        gboolean add, /* TRUE == add, FALSE == change */
-                                        FwAddToZoneFunc callback,
-                                        gpointer user_data)
+static NMFirewallManagerCallId
+_start_request (NMFirewallManager *self,
+                CBInfoOpsType ops_type,
+                const char *iface,
+                const char *zone,
+                NMFirewallManagerAddRemoveCallback callback,
+                gpointer user_data)
 {
-	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
+	NMFirewallManagerPrivate *priv;
 	CBInfo *info;
+	const char *dbus_method;
+
+	g_return_val_if_fail (NM_IS_FIREWALL_MANAGER (self), NULL);
+	g_return_val_if_fail (iface && *iface, NULL);
+
+	priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
+
+	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),
+	       iface,
+	       NM_PRINT_FMT_QUOTED (zone, "\"", zone, "\"", "default"),
+	       _cb_info_is_idle (info) ? " (not running, simulate success)" : "");
+
+	if (!_cb_info_is_idle (info)) {
+
+		switch (ops_type) {
+		case CB_INFO_OPS_ADD:
+			dbus_method = "addInterface";
+			break;
+		case CB_INFO_OPS_CHANGE:
+			dbus_method = "changeZone";
+			break;
+		case CB_INFO_OPS_REMOVE:
+			dbus_method = "removeInterface";
+			break;
+		default:
+			g_assert_not_reached ();
+		}
 
-	if (priv->running == FALSE) {
-		if (callback) {
-			info = _cb_info_create (self, iface, callback, user_data);
-			info->idle_id = g_idle_add (add_or_change_idle_cb, info);
-			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s -> %s%s%s [%u] (not running, simulate success)", iface, add ? "add" : "change",
-			            zone?"\"":"", zone ? zone : "default", zone?"\"":"", info->id);
-			return PENDING_CALL_FROM_INFO (info);
-		} else {
-			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone add/change skipped (not running)", iface);
-			return PENDING_CALL_DUMMY;
+		g_dbus_proxy_call (priv->proxy,
+		                   dbus_method,
+		                   g_variant_new ("(ss)", zone ? zone : "", iface),
+		                   G_DBUS_CALL_FLAGS_NONE, 10000,
+		                   info->dbus.cancellable,
+		                   _handle_dbus,
+		                   info);
+
+		if (!info->callback) {
+			/* if the user did not provide a callback, the call_id is useless.
+			 * Especially, the user cannot use the call-id to cancel the request,
+			 * because he cannot know whether the request is still pending.
+			 *
+			 * Hence, returning %NULL doesn't mean that the request could not be started
+			 * (the request will always be started). */
+			return NULL;
 		}
-	}
+	} else if (!info->callback) {
+		/* if the user did not provide a callback and firewalld is not running,
+		 * there is no point in scheduling an idle-request to fake success. Just
+		 * return right away. */
+		_LOGD (info, "complete: drop request simulating success");
+		_cb_info_free (info);
+		return NULL;
+	} else
+		info->idle.id = g_idle_add (_handle_idle, info);
 
-	info = _cb_info_create (self, iface, callback, user_data);
-
-	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s -> %s%s%s [%u]", iface, add ? "add" : "change",
-	                           zone?"\"":"", zone ? zone : "default", zone?"\"":"", info->id);
-	g_dbus_proxy_call (priv->proxy,
-	                   add ? "addInterface" : "changeZone",
-	                   g_variant_new ("(ss)", zone ? zone : "", iface),
-	                   G_DBUS_CALL_FLAGS_NONE, 10000,
-	                   info->cancellable,
-	                   add_or_change_cb, info);
-	return (NMFirewallPendingCall) info;
+	return info;
 }
 
-static void
-remove_cb (GObject *proxy, GAsyncResult *result, gpointer user_data)
+NMFirewallManagerCallId
+nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,
+                                        const char *iface,
+                                        const char *zone,
+                                        gboolean add, /* TRUE == add, FALSE == change */
+                                        NMFirewallManagerAddRemoveCallback callback,
+                                        gpointer user_data)
 {
-	CBInfo *info = user_data;
-	gs_free_error GError *error = NULL;
-	gs_unref_variant GVariant *ret = NULL;
-
-	ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), result, &error);
-	_cb_info_complete_and_free (info, "remove", "UNKNOWN_INTERFACE", error);
+	return _start_request (self,
+	                       add ? CB_INFO_OPS_ADD : CB_INFO_OPS_CHANGE,
+	                       iface,
+	                       zone,
+	                       callback,
+	                       user_data);
 }
 
-NMFirewallPendingCall
+NMFirewallManagerCallId
 nm_firewall_manager_remove_from_zone (NMFirewallManager *self,
                                       const char *iface,
-                                      const char *zone)
+                                      const char *zone,
+                                      NMFirewallManagerAddRemoveCallback callback,
+                                      gpointer user_data)
 {
-	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
-	CBInfo *info;
-
-	if (priv->running == FALSE) {
-		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove skipped (not running)", iface);
-		return PENDING_CALL_DUMMY;
-	}
-
-	info = _cb_info_create (self, iface, NULL, NULL);
-
-	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove -> %s%s%s [%u]", iface,
-	                           zone?"\"":"", zone ? zone : "*", zone?"\"":"", info->id);
-	g_dbus_proxy_call (priv->proxy,
-	                   "removeInterface",
-	                   g_variant_new ("(ss)", zone ? zone : "", iface),
-	                   G_DBUS_CALL_FLAGS_NONE, 10000,
-	                   info->cancellable,
-	                   remove_cb, info);
-	return (NMFirewallPendingCall) info;
+	return _start_request (self,
+	                       CB_INFO_OPS_REMOVE,
+	                       iface,
+	                       zone,
+	                       callback,
+	                       user_data);
 }
 
 void
-nm_firewall_manager_cancel_call (NMFirewallManager *self, NMFirewallPendingCall call)
+nm_firewall_manager_cancel_call (NMFirewallManagerCallId call)
 {
-	CBInfo *info = (CBInfo *) 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));
+
+	self = info->self;
+	priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
+
+	if (!g_hash_table_remove (priv->pending_calls, info))
+		g_return_if_reached ();
+
+	nm_utils_error_set_cancelled (&error, FALSE, "NMFirewallManager");
+
+	_LOGD (info, "complete: cancel (%s)", error->message);
 
-	g_return_if_fail (NM_IS_FIREWALL_MANAGER (self));
+	_cb_info_callback (info, error);
 
-	info->callback = NULL;
-	info->idle_id = 0;
-	g_cancellable_cancel (info->cancellable);
+	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);
+	}
 }
 
+/*******************************************************************/
+
 static void
 set_running (NMFirewallManager *self, gboolean now_running)
 {
@@ -259,25 +420,34 @@ name_owner_changed (GObject    *object,
 
 	owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (object));
 	if (owner) {
-		nm_log_dbg (LOGD_FIREWALL, "firewall started");
+		_LOGD (NULL, "firewall started");
 		set_running (self, TRUE);
 		g_signal_emit (self, signals[STARTED], 0);
 	} else {
-		nm_log_dbg (LOGD_FIREWALL, "firewall stopped");
+		_LOGD (NULL, "firewall stopped");
 		set_running (self, FALSE);
 	}
 }
 
 /*******************************************************************/
 
-NM_DEFINE_SINGLETON_GETTER (NMFirewallManager, nm_firewall_manager_get, NM_TYPE_FIREWALL_MANAGER);
-
 static void
 nm_firewall_manager_init (NMFirewallManager * self)
 {
 	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
+
+	priv->pending_calls = g_hash_table_new (g_direct_hash, g_direct_equal);
+}
+
+static void
+constructed (GObject *object)
+{
+	NMFirewallManager *self = (NMFirewallManager *) object;
+	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
 	gs_free char *owner = NULL;
 
+	G_OBJECT_CLASS (nm_firewall_manager_parent_class)->constructed (object);
+
 	priv->proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM,
 	                                             G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES |
 	                                                 G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS,
@@ -291,14 +461,7 @@ nm_firewall_manager_init (NMFirewallManager * self)
 	                  G_CALLBACK (name_owner_changed), self);
 	owner = g_dbus_proxy_get_name_owner (priv->proxy);
 	priv->running = (owner != NULL);
-	nm_log_dbg (LOGD_FIREWALL, "firewall %s running", priv->running ? "is" : "is not" );
-
-}
-
-static void
-set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec)
-{
-	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+	_LOGD (NULL, "firewall constructed (%srunning)", priv->running ? "" : "not");
 }
 
 static void
@@ -317,9 +480,16 @@ get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec)
 static void
 dispose (GObject *object)
 {
-	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (object);
+	NMFirewallManager *self = NM_FIREWALL_MANAGER (object);
+	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);
 
-	g_assert (priv->pending_calls == NULL);
+	if (priv->pending_calls) {
+		/* 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;
+	}
 
 	g_clear_object (&priv->proxy);
 
@@ -334,25 +504,25 @@ nm_firewall_manager_class_init (NMFirewallManagerClass *klass)
 
 	g_type_class_add_private (object_class, sizeof (NMFirewallManagerPrivate));
 
+	object_class->constructed = constructed;
 	object_class->get_property = get_property;
-	object_class->set_property = set_property;
 	object_class->dispose = dispose;
 
 	g_object_class_install_property
-		(object_class, PROP_AVAILABLE,
-		 g_param_spec_boolean (NM_FIREWALL_MANAGER_AVAILABLE, "", "",
-		                       FALSE,
-		                       G_PARAM_READABLE |
-		                       G_PARAM_STATIC_STRINGS));
+	    (object_class, PROP_AVAILABLE,
+	     g_param_spec_boolean (NM_FIREWALL_MANAGER_AVAILABLE, "", "",
+	                           FALSE,
+	                           G_PARAM_READABLE |
+	                           G_PARAM_STATIC_STRINGS));
 
 	signals[STARTED] =
-		g_signal_new ("started",
-					  G_OBJECT_CLASS_TYPE (object_class),
-					  G_SIGNAL_RUN_FIRST,
-					  G_STRUCT_OFFSET (NMFirewallManagerClass, started),
-					  NULL, NULL,
-					  g_cclosure_marshal_VOID__VOID,
-					  G_TYPE_NONE, 0);
+	    g_signal_new ("started",
+	                  G_OBJECT_CLASS_TYPE (object_class),
+	                  G_SIGNAL_RUN_FIRST,
+	                  G_STRUCT_OFFSET (NMFirewallManagerClass, started),
+	                  NULL, NULL,
+	                  g_cclosure_marshal_VOID__VOID,
+	                  G_TYPE_NONE, 0);
 
 }
 
diff --git a/src/nm-firewall-manager.h b/src/nm-firewall-manager.h
index 19d9689788..dfa83b459e 100644
--- a/src/nm-firewall-manager.h
+++ b/src/nm-firewall-manager.h
@@ -40,8 +40,8 @@ G_BEGIN_DECLS
 
 #define NM_FIREWALL_MANAGER_AVAILABLE "available"
 
-struct _NMFirewallPendingCall;
-typedef struct _NMFirewallPendingCall *NMFirewallPendingCall;
+struct _NMFirewallManagerCallId;
+typedef struct _NMFirewallManagerCallId *NMFirewallManagerCallId;
 
 typedef struct {
 	GObject parent;
@@ -58,18 +58,23 @@ GType nm_firewall_manager_get_type (void);
 
 NMFirewallManager *nm_firewall_manager_get (void);
 
-typedef void (*FwAddToZoneFunc) (GError *error, gpointer user_data);
-
-NMFirewallPendingCall nm_firewall_manager_add_or_change_zone (NMFirewallManager *mgr,
+typedef void (*NMFirewallManagerAddRemoveCallback) (NMFirewallManager *self,
+                                                    NMFirewallManagerCallId call_id,
+                                                    GError *error,
+                                                    gpointer user_data);
+
+NMFirewallManagerCallId nm_firewall_manager_add_or_change_zone (NMFirewallManager *mgr,
+                                                                const char *iface,
+                                                                const char *zone,
+                                                                gboolean add,
+                                                                NMFirewallManagerAddRemoveCallback callback,
+                                                                gpointer user_data);
+NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *mgr,
                                                               const char *iface,
                                                               const char *zone,
-                                                              gboolean add,
-                                                              FwAddToZoneFunc callback,
+                                                              NMFirewallManagerAddRemoveCallback callback,
                                                               gpointer user_data);
-NMFirewallPendingCall nm_firewall_manager_remove_from_zone (NMFirewallManager *mgr,
-                                                            const char *iface,
-                                                            const char *zone);
 
-void nm_firewall_manager_cancel_call (NMFirewallManager *mgr, NMFirewallPendingCall fw_call);
+void nm_firewall_manager_cancel_call (NMFirewallManagerCallId fw_call);
 
 #endif /* __NETWORKMANAGER_FIREWALL_MANAGER_H__ */
diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c
index c3abaac25c..0c36d286f2 100644
--- a/src/settings/nm-agent-manager.c
+++ b/src/settings/nm-agent-manager.c
@@ -576,19 +576,15 @@ req_complete_release (Request *req,
 }
 
 static void
-req_complete_cancel (Request *req,
-                     GQuark domain,
-                     guint code,
-                     const char *message)
+req_complete_cancel (Request *req, gboolean is_disposing)
 {
-	GError *error = NULL;
+	gs_free_error GError *error = NULL;
 
 	nm_assert (req && req->self);
 	nm_assert (!g_hash_table_contains (NM_AGENT_MANAGER_GET_PRIVATE (req->self)->requests, req));
 
-	g_set_error_literal (&error, domain, code, message);
+	nm_utils_error_set_cancelled (&error, is_disposing, "NMAgentManager");
 	req_complete_release (req, NULL, NULL, NULL, error);
-	g_error_free (error);
 }
 
 static void
@@ -1242,7 +1238,7 @@ nm_agent_manager_cancel_secrets (NMAgentManager *self,
 	                          request_id))
 		g_return_if_reached ();
 
-	req_complete_cancel (request_id, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Request cancelled");
+	req_complete_cancel (request_id, FALSE);
 }
 
 /*************************************************************/
@@ -1571,12 +1567,14 @@ dispose (GObject *object)
 		GHashTableIter iter;
 		Request *req;
 
+cancel_more:
 		g_hash_table_iter_init (&iter, priv->requests);
-		while (g_hash_table_iter_next (&iter, (gpointer *) &req, NULL)) {
+		if (g_hash_table_iter_next (&iter, (gpointer *) &req, NULL)) {
 			g_hash_table_iter_remove (&iter);
-			req_complete_cancel (req, G_IO_ERROR, G_IO_ERROR_FAILED, "Disposing NMAgentManagerClass instance");
+			req_complete_cancel (req, TRUE);
+			goto cancel_more;
 		}
-		g_hash_table_destroy (priv->requests);
+		g_hash_table_unref (priv->requests);
 		priv->requests = NULL;
 	}
 
diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c
index 1032027c39..29c0f55b4d 100644
--- a/src/settings/nm-secret-agent.c
+++ b/src/settings/nm-secret-agent.c
@@ -29,6 +29,7 @@
 #include "nm-bus-manager.h"
 #include "nm-auth-subject.h"
 #include "nm-simple-connection.h"
+#include "NetworkManagerUtils.h"
 
 #include "nmdbus-secret-agent.h"
 
@@ -428,23 +429,12 @@ do_cancel_secrets (NMSecretAgent *self, Request *r, gboolean disposing)
 	 * Only clear r->cancellable to indicate that the request was cancelled. */
 
 	if (callback) {
-		GError *error = NULL;
-
-		if (disposing) {
-			/* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously
-			 * when the user calls nm_act_request_cancel_secrets().
-			 * When the user disposes the instance, we also invoke the callback synchronously,
-			 * but with a different error-reason. */
-			g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED,
-		                         "Disposing NMSecretAgent instance");
-		} else {
-			g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
-			                     "Request cancelled");
-		}
+		gs_free_error GError *error = NULL;
+
+		nm_utils_error_set_cancelled (&error, disposing, "NMSecretAgent");
 		/* @r might be a dangling pointer at this point. However, that is no problem
 		 * to pass it as (opaque) call_id. */
 		callback (self, r, NULL, error, callback_data);
-		g_error_free (error);
 	}
 }
 
diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c
index 9ab0bc6b30..29162c7984 100644
--- a/src/settings/nm-settings-connection.c
+++ b/src/settings/nm-settings-connection.c
@@ -1270,10 +1270,9 @@ schedule_dummy:
 static void
 _get_secrets_cancel (NMSettingsConnection *self,
                      GetSecretsInfo *info,
-                     gboolean shutdown)
+                     gboolean is_disposing)
 {
 	NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self);
-
 	gs_free_error GError *error = NULL;
 
 	if (!g_slist_find (priv->get_secret_requests, info))
@@ -1286,17 +1285,7 @@ _get_secrets_cancel (NMSettingsConnection *self,
 	else
 		g_source_remove (info->t.idle.id);
 
-	if (shutdown) {
-		/* Use a different error code. G_IO_ERROR_CANCELLED is only used synchronously
-		 * when the user calls nm_act_request_cancel_secrets(). Disposing the instance
-		 * with pending requests also cancels the requests, but with a different error
-		 * code. */
-		g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED,
-		                     "Disposing NMActRequest instance");
-	} else {
-		g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
-		                     "Request cancelled");
-	}
+	nm_utils_error_set_cancelled (&error, is_disposing, "NMSettingsConnection");
 
 	_get_secrets_info_callback (info, NULL, NULL, error);
 
diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c
index 4141d3f7af..a520dfd43e 100644
--- a/src/vpn-manager/nm-vpn-connection.c
+++ b/src/vpn-manager/nm-vpn-connection.c
@@ -91,7 +91,7 @@ typedef struct {
 	NMVpnServiceState service_state;
 
 	/* Firewall */
-	NMFirewallPendingCall fw_call;
+	NMFirewallManagerCallId fw_call;
 
 	NMDefaultRouteManager *default_route_manager;
 	NMRouteManager *route_manager;
@@ -323,7 +323,8 @@ fw_call_cleanup (NMVpnConnection *self)
 	NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
 
 	if (priv->fw_call) {
-		nm_firewall_manager_cancel_call (nm_firewall_manager_get (), priv->fw_call);
+		nm_firewall_manager_cancel_call (priv->fw_call);
+		g_warn_if_fail (!priv->fw_call);
 		priv->fw_call = NULL;
 	}
 }
@@ -343,10 +344,13 @@ vpn_cleanup (NMVpnConnection *self, NMDevice *parent_dev)
 	nm_device_set_vpn6_config (parent_dev, NULL);
 
 	/* Remove zone from firewall */
-	if (priv->ip_iface)
+	if (priv->ip_iface) {
 		nm_firewall_manager_remove_from_zone (nm_firewall_manager_get (),
 		                                      priv->ip_iface,
+		                                      NULL,
+		                                      NULL,
 		                                      NULL);
+	}
 	/* Cancel pending firewall call */
 	fw_call_cleanup (self);
 
@@ -1090,19 +1094,25 @@ _cleanup_failed_config (NMVpnConnection *self)
 }
 
 static void
-fw_change_zone_cb (GError *error, gpointer user_data)
+fw_change_zone_cb (NMFirewallManager *firewall_manager,
+                   NMFirewallManagerCallId call_id,
+                   GError *error,
+                   gpointer user_data)
 {
-	NMVpnConnection *self = NM_VPN_CONNECTION (user_data);
-	NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
+	NMVpnConnection *self = user_data;
+	NMVpnConnectionPrivate *priv;
 
-	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-		return;
+	g_return_if_fail (NM_IS_VPN_CONNECTION (self));
+
+	priv = NM_VPN_CONNECTION_GET_PRIVATE (self);
+	g_return_if_fail (priv->fw_call == call_id);
 
 	priv->fw_call = NULL;
 
+	if (nm_utils_error_is_cancelled (error, FALSE))
+		return;
+
 	if (error) {
-		_LOGW ("VPN connection: setting firewall zone failed: '%s'",
-		       error->message);
 		// FIXME: fail the activation?
 	}