From b50702775f1b7a56ad81fe9be58294a29e805c8f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 26 Apr 2020 13:59:13 +0200 Subject: [PATCH] device: implement "auth-request" as async operation nm_manager_device_auth_request() GObject signals only complicate the code and are less efficient. Also, NM_DEVICE_AUTH_REQUEST signal really invoked an asynchronous request. Of course, fundamentally emitting a signal *is* the same as calling a method. However, implementing this as signal is really not nice nor best practice. For one, there is a (negligible) overhead emitting a GObject signal. But what is worse, GObject signals are not as strongly typed and make it harder to understand what happens. The signal had the appearance of providing some special decoupling of NMDevice and NMManager. Of course, in practice, they were not more decoupled (both forms are the same in nature), but it was harder to understand how they work together. Add and call a method nm_manager_device_auth_request() instead. This has the notion of invoking an asynchronous method. Also, never invoke the callback synchronously and provide a cancellable. Like every asynchronous operation, it *must* be cancellable, and callers should make sure to provide a mechanism to abort. --- src/devices/nm-device-private.h | 11 +++ src/devices/nm-device.c | 105 +++++++++++++++------------ src/devices/nm-device.h | 7 -- src/devices/wifi/nm-device-iwd.c | 16 ++--- src/devices/wifi/nm-device-wifi.c | 16 ++--- src/nm-manager.c | 115 ++++++++++++++++++++---------- src/nm-manager.h | 12 ++++ src/nm-types.h | 6 ++ 8 files changed, 183 insertions(+), 105 deletions(-) diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index f9bc4125b8..4e260da2f3 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -205,4 +205,15 @@ gboolean nm_device_match_parent_hwaddr (NMDevice *device, NMConnection *connection, gboolean fail_if_no_hwaddr); +/*****************************************************************************/ + +void nm_device_auth_request (NMDevice *self, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data); + #endif /* NM_DEVICE_PRIVATE_H */ diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4a24c40bc5..27b1b600b1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -192,7 +192,6 @@ typedef struct { enum { STATE_CHANGED, AUTOCONNECT_ALLOWED, - AUTH_REQUEST, IP4_CONFIG_CHANGED, IP6_CONFIG_CHANGED, IP6_PREFIX_DELEGATED, @@ -6265,6 +6264,27 @@ dnsmasq_state_changed_cb (NMDnsMasqManager *manager, guint32 status, gpointer us } } +void +nm_device_auth_request (NMDevice *self, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data) +{ + nm_manager_device_auth_request (nm_device_get_manager (self), + self, + context, + connection, + permission, + allow_interaction, + cancellable, + callback, + user_data); +} + /*****************************************************************************/ static void @@ -12303,13 +12323,14 @@ impl_device_reapply (NMDBusObject *obj, } else reapply_data = NULL; - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - nm_device_get_applied_connection (self), - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - reapply_cb, - reapply_data); + nm_device_auth_request (self, + invocation, + nm_device_get_applied_connection (self), + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + reapply_cb, + reapply_data); } /*****************************************************************************/ @@ -12346,13 +12367,14 @@ get_applied_connection_cb (NMDevice *self, if (applied_connection != user_data) { /* The applied connection changed due to a race. Reauthenticate. */ - g_signal_emit (self, signals[AUTH_REQUEST], 0, - context, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); + nm_device_auth_request (self, + context, + applied_connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + get_applied_connection_cb, + applied_connection /* no need take a ref. We will not dereference this pointer. */); return; } @@ -12399,13 +12421,14 @@ impl_device_get_applied_connection (NMDBusObject *obj, return; } - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); + nm_device_auth_request (self, + invocation, + applied_connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + get_applied_connection_cb, + applied_connection /* no need take a ref. We will not dereference this pointer. */); } /*****************************************************************************/ @@ -12573,13 +12596,14 @@ impl_device_disconnect (NMDBusObject *obj, connection = nm_device_get_applied_connection (self); nm_assert (connection); - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - disconnect_cb, - NULL); + nm_device_auth_request (self, + invocation, + connection, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + disconnect_cb, + NULL); } static void @@ -12625,13 +12649,14 @@ impl_device_delete (NMDBusObject *obj, return; } - g_signal_emit (self, signals[AUTH_REQUEST], 0, - invocation, - NULL, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - delete_cb, - NULL); + nm_device_auth_request (self, + invocation, + NULL, + NM_AUTH_PERMISSION_NETWORK_CONTROL, + TRUE, + NULL, + delete_cb, + NULL); } static void @@ -18014,14 +18039,6 @@ nm_device_class_init (NMDeviceClass *klass) autoconnect_allowed_accumulator, NULL, NULL, G_TYPE_BOOLEAN, 0); - signals[AUTH_REQUEST] = - g_signal_new (NM_DEVICE_AUTH_REQUEST, - G_OBJECT_CLASS_TYPE (object_class), - G_SIGNAL_RUN_FIRST, - 0, NULL, NULL, NULL, - /* context, connection, permission, allow_interaction, callback, user_data */ - G_TYPE_NONE, 6, G_TYPE_DBUS_METHOD_INVOCATION, NM_TYPE_CONNECTION, G_TYPE_STRING, G_TYPE_BOOLEAN, G_TYPE_POINTER, G_TYPE_POINTER); - signals[IP4_CONFIG_CHANGED] = g_signal_new (NM_DEVICE_IP4_CONFIG_CHANGED, G_OBJECT_CLASS_TYPE (object_class), diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index c18301b275..8d51c707dd 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -115,7 +115,6 @@ nm_device_state_reason_check (NMDeviceStateReason reason) #define NM_DEVICE_HAS_PENDING_ACTION "has-pending-action" /* Internal only */ /* Internal signals */ -#define NM_DEVICE_AUTH_REQUEST "auth-request" #define NM_DEVICE_IP4_CONFIG_CHANGED "ip4-config-changed" #define NM_DEVICE_IP6_CONFIG_CHANGED "ip6-config-changed" #define NM_DEVICE_IP6_PREFIX_DELEGATED "ip6-prefix-delegated" @@ -455,12 +454,6 @@ typedef struct _NMDeviceClass { } NMDeviceClass; -typedef void (*NMDeviceAuthRequestFunc) (NMDevice *device, - GDBusMethodInvocation *context, - NMAuthSubject *subject, - GError *error, - gpointer user_data); - GType nm_device_get_type (void); struct _NMDedupMultiIndex *nm_device_get_multi_index (NMDevice *self); diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 3dccd1cb94..e2e705fd2e 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1061,14 +1061,14 @@ _nm_device_iwd_request_scan (NMDeviceIwd *self, return; } - g_signal_emit_by_name (device, - NM_DEVICE_AUTH_REQUEST, - invocation, - NULL, - NM_AUTH_PERMISSION_WIFI_SCAN, - TRUE, - dbus_request_scan_cb, - options ? g_variant_ref (options) : NULL); + nm_device_auth_request (device, + invocation, + NULL, + NM_AUTH_PERMISSION_WIFI_SCAN, + TRUE, + NULL, + dbus_request_scan_cb, + nm_g_variant_ref (options)); } static gboolean diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index bf3e6ad27a..8aee27f4ce 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1323,14 +1323,14 @@ _nm_device_wifi_request_scan (NMDeviceWifi *self, return; } - g_signal_emit_by_name (device, - NM_DEVICE_AUTH_REQUEST, - invocation, - NULL, - NM_AUTH_PERMISSION_WIFI_SCAN, - TRUE, - dbus_request_scan_cb, - g_steal_pointer (&ssids)); + nm_device_auth_request (device, + invocation, + NULL, + NM_AUTH_PERMISSION_WIFI_SCAN, + TRUE, + NULL, + dbus_request_scan_cb, + g_steal_pointer (&ssids)); } static gboolean diff --git a/src/nm-manager.c b/src/nm-manager.c index 1f3ba1f661..bbe501259f 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2375,8 +2375,9 @@ device_auth_done_cb (NMAuthChain *chain, gs_free_error GError *error = NULL; NMAuthCallResult result; NMDevice *device; + GCancellable *cancellable; const char *permission; - NMDeviceAuthRequestFunc callback; + NMManagerDeviceAuthRequestFunc callback; NMAuthSubject *subject; nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); @@ -2390,18 +2391,26 @@ device_auth_done_cb (NMAuthChain *chain, device = nm_auth_chain_get_data (chain, "device"); nm_assert (NM_IS_DEVICE (device)); + cancellable = nm_auth_chain_get_cancellable (chain); + nm_assert (!cancellable || G_IS_CANCELLABLE (cancellable)); + result = nm_auth_chain_get_result (chain, permission); subject = nm_auth_chain_get_subject (chain); - if (result != NM_AUTH_CALL_RESULT_YES) { - _LOGD (LOGD_CORE, "%s request failed: not authorized", permission); - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "%s request failed: not authorized", - permission); - } + if ( cancellable + && g_cancellable_set_error_if_cancelled (cancellable, &error)) { + /* pass. */ + } else { + if (result != NM_AUTH_CALL_RESULT_YES) { + _LOGD (LOGD_CORE, "%s request failed: not authorized", permission); + error = g_error_new (NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + "%s request failed: not authorized", + permission); + } - nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); + nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); + } callback (device, context, @@ -2411,28 +2420,53 @@ device_auth_done_cb (NMAuthChain *chain, } static void -device_auth_request_cb (NMDevice *device, - GDBusMethodInvocation *context, - NMConnection *connection, - const char *permission, - gboolean allow_interaction, - NMDeviceAuthRequestFunc callback, - gpointer user_data, - NMManager *self) +_device_auth_done_fail_on_idle (gpointer user_data, GCancellable *cancellable) +{ + gs_unref_object NMManager *self = NULL; + gs_unref_object NMDevice *device = NULL; + gs_unref_object GDBusMethodInvocation *context = NULL; + gs_unref_object NMAuthSubject *subject = NULL; + gs_free_error GError *error_original = NULL; + gs_free_error GError *error_cancelled = NULL; + NMManagerDeviceAuthRequestFunc callback; + gpointer callback_user_data; + + nm_utils_user_data_unpack (&self, &device, &context, &subject, &error_original, &callback, &callback_user_data); + + g_cancellable_set_error_if_cancelled (cancellable, &error_cancelled); + + callback (device, + context, + subject, + error_cancelled ?: error_original, + callback_user_data); +} + +void +nm_manager_device_auth_request (NMManager *self, + NMDevice *device, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *error = NULL; - NMAuthSubject *subject = NULL; + gs_free_error GError *error = NULL; + gs_unref_object NMAuthSubject *subject = NULL; NMAuthChain *chain; char *permission_dup; /* Validate the caller */ subject = nm_dbus_manager_new_auth_subject_from_context (context); if (!subject) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); - goto done; + g_set_error_literal (&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); + goto fail_on_idle; } /* Ensure the subject has permissions for this connection */ @@ -2442,17 +2476,21 @@ device_auth_request_cb (NMDevice *device, NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, &error)) - goto done; + goto fail_on_idle; /* Validate the request */ chain = nm_auth_chain_new_subject (subject, context, device_auth_done_cb, self); if (!chain) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED); - goto done; + g_set_error (&error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED); + goto fail_on_idle; } + if (cancellable) + nm_auth_chain_set_cancellable (chain, cancellable); + permission_dup = g_strdup (permission); c_list_link_tail (&priv->auth_lst_head, nm_auth_chain_parent_lst_list (chain)); @@ -2461,13 +2499,18 @@ device_auth_request_cb (NMDevice *device, nm_auth_chain_set_data (chain, "user-data", user_data, NULL); nm_auth_chain_set_data (chain, "perm", permission_dup /* transfer ownership */, g_free); nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction); + return; -done: - if (error) - callback (device, context, subject, error, user_data); - - g_clear_object (&subject); - g_clear_error (&error); +fail_on_idle: + nm_utils_invoke_on_idle (cancellable, + _device_auth_done_fail_on_idle, + nm_utils_user_data_pack (g_object_ref (self), + g_object_ref (device), + g_object_ref (context), + g_steal_pointer (&subject), + g_steal_pointer (&error), + callback, + user_data)); } static gboolean @@ -3130,10 +3173,6 @@ add_device (NMManager *self, NMDevice *device, GError **error) G_CALLBACK (manager_device_state_changed), self); - g_signal_connect (device, NM_DEVICE_AUTH_REQUEST, - G_CALLBACK (device_auth_request_cb), - self); - g_signal_connect (device, NM_DEVICE_REMOVED, G_CALLBACK (device_removed_cb), self); diff --git a/src/nm-manager.h b/src/nm-manager.h index 5873abd24b..ab08eaa839 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -193,4 +193,16 @@ NMMetered nm_manager_get_metered (NMManager *self); void nm_manager_notify_device_availibility_maybe_changed (NMManager *self); +/*****************************************************************************/ + +void nm_manager_device_auth_request (NMManager *self, + NMDevice *device, + GDBusMethodInvocation *context, + NMConnection *connection, + const char *permission, + gboolean allow_interaction, + GCancellable *cancellable, + NMManagerDeviceAuthRequestFunc callback, + gpointer user_data); + #endif /* __NETWORKMANAGER_MANAGER_H__ */ diff --git a/src/nm-types.h b/src/nm-types.h index d2f6fb4093..d2c999fada 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -40,6 +40,12 @@ typedef struct _NMSleepMonitor NMSleepMonitor; typedef struct _NMLldpListener NMLldpListener; typedef struct _NMConfigDeviceStateData NMConfigDeviceStateData; +typedef void (*NMManagerDeviceAuthRequestFunc) (NMDevice *device, + GDBusMethodInvocation *context, + NMAuthSubject *subject, + GError *error, + gpointer user_data); + struct _NMDedupMultiIndex; typedef struct _NMRefString NMRefString;