From 44f3f18797cf3b0e0d45791939b2f9f023d51230 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Sep 2015 12:44:20 +0200 Subject: core: add NM_UTILS_ERROR --- src/NetworkManagerUtils.c | 33 +++++++++++++++++++++++++++++++++ src/NetworkManagerUtils.h | 25 +++++++++++++++++++++++++ 2 files changed, 58 insertions(+) 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); -- cgit v1.2.1 From 320f454e9f06db6859134daf4e9e567e9b03c8eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Sep 2015 13:14:14 +0200 Subject: core: use NM_UTILS_ERROR_CANCELLED_DISPOSING error reason --- src/nm-activation-request.c | 13 +------------ src/nm-auth-manager.c | 5 ++--- src/settings/nm-agent-manager.c | 20 +++++++++----------- src/settings/nm-secret-agent.c | 18 ++++-------------- src/settings/nm-settings-connection.c | 15 ++------------- 5 files changed, 18 insertions(+), 53 deletions(-) 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/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); -- cgit v1.2.1 From 0d33b73e425a7c8cb465443068f5dcbac6c0a29d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 16:41:14 +0200 Subject: firewall/trivial: rename NMFirewallPendingCall to NMFirewallManagerCallId Matches the naming scheme for other call-ids. --- src/devices/nm-device.c | 2 +- src/nm-firewall-manager.c | 14 +++++++------- src/nm-firewall-manager.h | 24 ++++++++++++------------ src/vpn-manager/nm-vpn-connection.c | 2 +- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 928481ec12..1c5ad3e5bc 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; diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c index 93792b8799..044d48a0d7 100644 --- a/src/nm-firewall-manager.c +++ b/src/nm-firewall-manager.c @@ -56,8 +56,8 @@ static guint signals[LAST_SIGNAL] = { 0 }; /********************************************************************/ -#define PENDING_CALL_DUMMY ((NMFirewallPendingCall) GUINT_TO_POINTER(1)) -#define PENDING_CALL_FROM_INFO(info) ((NMFirewallPendingCall) info) +#define PENDING_CALL_DUMMY ((NMFirewallManagerCallId) GUINT_TO_POINTER(1)) +#define PENDING_CALL_FROM_INFO(info) ((NMFirewallManagerCallId) info) typedef struct { NMFirewallManager *self; @@ -152,7 +152,7 @@ add_or_change_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) _cb_info_complete_and_free (info, "add/change", "ZONE_ALREADY_SET", error); } -NMFirewallPendingCall +NMFirewallManagerCallId nm_firewall_manager_add_or_change_zone (NMFirewallManager *self, const char *iface, const char *zone, @@ -186,7 +186,7 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self, G_DBUS_CALL_FLAGS_NONE, 10000, info->cancellable, add_or_change_cb, info); - return (NMFirewallPendingCall) info; + return (NMFirewallManagerCallId) info; } static void @@ -200,7 +200,7 @@ remove_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) _cb_info_complete_and_free (info, "remove", "UNKNOWN_INTERFACE", error); } -NMFirewallPendingCall +NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *self, const char *iface, const char *zone) @@ -223,11 +223,11 @@ nm_firewall_manager_remove_from_zone (NMFirewallManager *self, G_DBUS_CALL_FLAGS_NONE, 10000, info->cancellable, remove_cb, info); - return (NMFirewallPendingCall) info; + return (NMFirewallManagerCallId) info; } void -nm_firewall_manager_cancel_call (NMFirewallManager *self, NMFirewallPendingCall call) +nm_firewall_manager_cancel_call (NMFirewallManager *self, NMFirewallManagerCallId call) { CBInfo *info = (CBInfo *) call; diff --git a/src/nm-firewall-manager.h b/src/nm-firewall-manager.h index 19d9689788..a37f2fd6ea 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; @@ -60,16 +60,16 @@ NMFirewallManager *nm_firewall_manager_get (void); typedef void (*FwAddToZoneFunc) (GError *error, gpointer user_data); -NMFirewallPendingCall nm_firewall_manager_add_or_change_zone (NMFirewallManager *mgr, +NMFirewallManagerCallId nm_firewall_manager_add_or_change_zone (NMFirewallManager *mgr, + const char *iface, + const char *zone, + gboolean add, + FwAddToZoneFunc callback, + gpointer user_data); +NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *mgr, const char *iface, - const char *zone, - gboolean add, - FwAddToZoneFunc 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); + const char *zone); + +void nm_firewall_manager_cancel_call (NMFirewallManager *mgr, NMFirewallManagerCallId fw_call); #endif /* __NETWORKMANAGER_FIREWALL_MANAGER_H__ */ diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c index 4141d3f7af..2c79de6b0d 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; -- cgit v1.2.1 From d3a8254681cbbab864f1e7d5c6ac87a768b323e4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 16:43:22 +0200 Subject: firewall/trivial: rename FwAddToZoneFunc to NMFirewallManagerAddRemoveCallback Give it a proper NMFirewallManager* prefix to make it clear where the type belongs. Also, we will add a similar callback for nm_firewall_manager_remove_from_zone(), so reflect that in the name. --- src/nm-firewall-manager.c | 6 +++--- src/nm-firewall-manager.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c index 044d48a0d7..1418013f9a 100644 --- a/src/nm-firewall-manager.c +++ b/src/nm-firewall-manager.c @@ -62,7 +62,7 @@ static guint signals[LAST_SIGNAL] = { 0 }; typedef struct { NMFirewallManager *self; char *iface; - FwAddToZoneFunc callback; + NMFirewallManagerAddRemoveCallback callback; gpointer user_data; guint id; @@ -113,7 +113,7 @@ _cb_info_complete_and_free (CBInfo *info, } static CBInfo * -_cb_info_create (NMFirewallManager *self, const char *iface, FwAddToZoneFunc callback, gpointer user_data) +_cb_info_create (NMFirewallManager *self, const char *iface, NMFirewallManagerAddRemoveCallback callback, gpointer user_data) { NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self); static guint id = 1; @@ -157,7 +157,7 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self, const char *iface, const char *zone, gboolean add, /* TRUE == add, FALSE == change */ - FwAddToZoneFunc callback, + NMFirewallManagerAddRemoveCallback callback, gpointer user_data) { NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self); diff --git a/src/nm-firewall-manager.h b/src/nm-firewall-manager.h index a37f2fd6ea..fa83e1bcee 100644 --- a/src/nm-firewall-manager.h +++ b/src/nm-firewall-manager.h @@ -58,13 +58,13 @@ GType nm_firewall_manager_get_type (void); NMFirewallManager *nm_firewall_manager_get (void); -typedef void (*FwAddToZoneFunc) (GError *error, gpointer user_data); +typedef void (*NMFirewallManagerAddRemoveCallback) (GError *error, gpointer user_data); NMFirewallManagerCallId nm_firewall_manager_add_or_change_zone (NMFirewallManager *mgr, const char *iface, const char *zone, gboolean add, - FwAddToZoneFunc callback, + NMFirewallManagerAddRemoveCallback callback, gpointer user_data); NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *mgr, const char *iface, -- cgit v1.2.1 From 5bc4d7f0f95f09939e27e2c056ff49b72de50e6d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 17:05:02 +0200 Subject: firewall: add arguments to NMFirewallManagerAddRemoveCallback We should return the target object and the call_id. --- src/devices/nm-device.c | 5 ++++- src/nm-firewall-manager.c | 2 +- src/nm-firewall-manager.h | 5 ++++- src/vpn-manager/nm-vpn-connection.c | 5 ++++- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 1c5ad3e5bc..43ad89e338 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -5527,7 +5527,10 @@ 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; NMDevicePrivate *priv; diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c index 1418013f9a..e7d8638c9e 100644 --- a/src/nm-firewall-manager.c +++ b/src/nm-firewall-manager.c @@ -105,7 +105,7 @@ _cb_info_complete_and_free (CBInfo *info, } if (info->callback) - info->callback (error, info->user_data); + info->callback (info->self, PENDING_CALL_FROM_INFO (info), error, info->user_data); g_free (info->iface); g_object_unref (info->cancellable); diff --git a/src/nm-firewall-manager.h b/src/nm-firewall-manager.h index fa83e1bcee..92faa360e3 100644 --- a/src/nm-firewall-manager.h +++ b/src/nm-firewall-manager.h @@ -58,7 +58,10 @@ GType nm_firewall_manager_get_type (void); NMFirewallManager *nm_firewall_manager_get (void); -typedef void (*NMFirewallManagerAddRemoveCallback) (GError *error, gpointer user_data); +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, diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c index 2c79de6b0d..60d9c882be 100644 --- a/src/vpn-manager/nm-vpn-connection.c +++ b/src/vpn-manager/nm-vpn-connection.c @@ -1090,7 +1090,10 @@ _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); -- cgit v1.2.1 From 94f888a2628a5e743d5abbb3e6f95c7c83052f09 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 17:18:44 +0200 Subject: firewall: refactor callback handling in NMFirewallManager Refactor NMFirewallManager to have consistent semantics about asynchronous calls. - the callback is always invoked exactly once, but never synchronously when starting the operation. - while cancelling the request, the callback is invoked synchronously with respect to the cancel operation. - you can cancel a request once (and once only). - you cannot cancel the request once the callback is invoked. - when disposing the object with requests pending, the callbacks are invoked synchronously. - Add a callback argument to nm_firewall_manager_remove_from_zone(). Otherwise, the user never knows whether a call is still pending and cancellable (as complete operations cannot be cancelled). In fact, it makes no sense trying to cancel a call if you don't care about when it completes. - get rid of PENDING_CALL_DUMMY. The dummy callback allows cancellation any number of times. We want to catch wrong usage by asserting that an operation is only cancelled once. - nm_firewall_manager_cancel_call() doesn't need the manager argument. Either the call-id is valid (and has a valid pointer to the manager), or there is a bug and it is a dangling pointer. --- src/devices/nm-device.c | 18 +- src/nm-firewall-manager.c | 495 +++++++++++++++++++++++++----------- src/nm-firewall-manager.h | 8 +- src/vpn-manager/nm-vpn-connection.c | 25 +- 4 files changed, 384 insertions(+), 162 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 43ad89e338..783d14282c 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -5532,17 +5532,19 @@ fw_change_zone_cb (NMFirewallManager *firewall_manager, 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? */ } @@ -5590,6 +5592,7 @@ nm_device_activate_schedule_stage3_ip_config_start (NMDevice *self) nm_device_get_ip_iface (self), zone, FALSE, + TRUE, fw_change_zone_cb, self); } @@ -7937,6 +7940,7 @@ 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); } @@ -8313,7 +8317,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; } @@ -8337,6 +8342,9 @@ _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, + TRUE, + NULL, NULL); } diff --git a/src/nm-firewall-manager.c b/src/nm-firewall-manager.c index e7d8638c9e..a42ebe3791 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,370 @@ enum { static guint signals[LAST_SIGNAL] = { 0 }; +NM_DEFINE_SINGLETON_GETTER (NMFirewallManager, nm_firewall_manager_get, NM_TYPE_FIREWALL_MANAGER); + /********************************************************************/ -#define PENDING_CALL_DUMMY ((NMFirewallManagerCallId) GUINT_TO_POINTER(1)) -#define PENDING_CALL_FROM_INFO(info) ((NMFirewallManagerCallId) 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; + gboolean keep_mgr_alive; + CBInfoOpsType ops_type; + CBInfoMode mode; char *iface; 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 (info->self, PENDING_CALL_FROM_INFO (info), 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, NMFirewallManagerAddRemoveCallback callback, gpointer user_data) +_cb_info_create (NMFirewallManager *self, + CBInfoOpsType ops_type, + const char *iface, + gboolean keep_mgr_alive, + 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->self = self; + info->keep_mgr_alive = keep_mgr_alive; + 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 (keep_mgr_alive) + g_object_ref (self); + + 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->keep_mgr_alive) + 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 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 -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); } -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) +static NMFirewallManagerCallId +_start_request (NMFirewallManager *self, + CBInfoOpsType ops_type, + const char *iface, + const char *zone, + gboolean keep_mgr_alive, + 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, keep_mgr_alive, 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 (NMFirewallManagerCallId) 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 */ + gboolean keep_mgr_alive, + 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, + keep_mgr_alive, + callback, + user_data); } NMFirewallManagerCallId nm_firewall_manager_remove_from_zone (NMFirewallManager *self, const char *iface, - const char *zone) + const char *zone, + gboolean keep_mgr_alive, + 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 (NMFirewallManagerCallId) info; + return _start_request (self, + CB_INFO_OPS_REMOVE, + iface, + zone, + keep_mgr_alive, + callback, + user_data); } void -nm_firewall_manager_cancel_call (NMFirewallManager *self, NMFirewallManagerCallId call) +nm_firewall_manager_cancel_call (NMFirewallManagerCallId call) { - CBInfo *info = (CBInfo *) call; + NMFirewallManagerPrivate *priv; + CBInfo *info = call; - g_return_if_fail (NM_IS_FIREWALL_MANAGER (self)); + g_return_if_fail (info); + g_return_if_fail (NM_IS_FIREWALL_MANAGER (info->self)); - info->callback = NULL; - info->idle_id = 0; - g_cancellable_cancel (info->cancellable); + priv = NM_FIREWALL_MANAGER_GET_PRIVATE (info->self); + + if (!g_hash_table_remove (priv->pending_calls, info)) + g_return_if_reached (); + + _cb_info_complete_cancel (info, FALSE); } +/*******************************************************************/ + static void set_running (NMFirewallManager *self, gboolean now_running) { @@ -259,25 +440,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 +481,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 +500,27 @@ get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) static void dispose (GObject *object) { - NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (object); - - g_assert (priv->pending_calls == NULL); + 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; + } + g_hash_table_unref (priv->pending_calls); + priv->pending_calls = NULL; + } g_clear_object (&priv->proxy); @@ -334,25 +535,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 92faa360e3..80970374d2 100644 --- a/src/nm-firewall-manager.h +++ b/src/nm-firewall-manager.h @@ -67,12 +67,16 @@ 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); + const char *zone, + gboolean keep_mgr_alive, + NMFirewallManagerAddRemoveCallback callback, + gpointer user_data); -void nm_firewall_manager_cancel_call (NMFirewallManager *mgr, NMFirewallManagerCallId fw_call); +void nm_firewall_manager_cancel_call (NMFirewallManagerCallId fw_call); #endif /* __NETWORKMANAGER_FIREWALL_MANAGER_H__ */ diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c index 60d9c882be..132e2cd9e8 100644 --- a/src/vpn-manager/nm-vpn-connection.c +++ b/src/vpn-manager/nm-vpn-connection.c @@ -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,14 @@ 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, + TRUE, + NULL, NULL); + } /* Cancel pending firewall call */ fw_call_cleanup (self); @@ -1095,17 +1100,20 @@ fw_change_zone_cb (NMFirewallManager *firewall_manager, 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? } @@ -1155,6 +1163,7 @@ nm_vpn_connection_config_maybe_complete (NMVpnConnection *self, priv->ip_iface, zone, FALSE, + TRUE, fw_change_zone_cb, self); return; -- cgit v1.2.1 From bb7e6c2cd461ea4a1709dedd71f171eb54bab41d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 25 Sep 2015 09:59:16 +0200 Subject: firewall: always take a reference to NMFirewallManager during asynchronous operations Always take a reference to the manager when scheaduling an asynchronous operations. In fact, all callers want to keep the manager alive as long as there are operations in progress. --- src/devices/nm-device.c | 3 -- src/nm-firewall-manager.c | 79 +++++++++++-------------------------- src/nm-firewall-manager.h | 2 - src/vpn-manager/nm-vpn-connection.c | 2 - 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; -- cgit v1.2.1