From 24b50625bd1d7f0210703089016817b8ead04682 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:40:11 +0100 Subject: [PATCH 01/22] shared: add NM_UTILS_USEC_PER_SEC macro --- shared/nm-glib-aux/nm-shared-utils.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index ad461d9d00..0b550ad051 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1198,8 +1198,9 @@ _nm_utils_strv_equal (char **strv1, char **strv2) /*****************************************************************************/ #define NM_UTILS_NSEC_PER_SEC ((gint64) 1000000000) -#define NM_UTILS_NSEC_PER_MSEC ((gint64) 1000000) +#define NM_UTILS_USEC_PER_SEC ((gint64) 1000000) #define NM_UTILS_MSEC_PER_SEC ((gint64) 1000) +#define NM_UTILS_NSEC_PER_MSEC ((gint64) 1000000) static inline gint64 NM_UTILS_NSEC_TO_MSEC_CEIL (gint64 nsec) From 6e7e18c86f711d79a02dde032c334e2005e8aec1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 Jan 2020 12:45:54 +0100 Subject: [PATCH 02/22] shared: add nm_g_main_context_is_thread_default() util --- shared/nm-glib-aux/nm-shared-utils.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 0b550ad051..e7934cf163 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1026,6 +1026,25 @@ nm_g_main_context_push_thread_default (GMainContext *context) return context; } +static inline gboolean +nm_g_main_context_is_thread_default (GMainContext *context) +{ + GMainContext *cur_context; + + cur_context = g_main_context_get_thread_default (); + if (cur_context == context) + return TRUE; + + if (G_UNLIKELY (!cur_context)) + cur_context = g_main_context_default (); + else if (G_UNLIKELY (!context)) + context = g_main_context_default (); + else + return FALSE; + + return (cur_context == context); +} + static inline GMainContext * nm_g_main_context_push_thread_default_if_necessary (GMainContext *context) { From 64c53a2afa77c174a4a12012865ad7379fc6d4be Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 Jan 2020 17:50:28 +0100 Subject: [PATCH 03/22] libnm: add define for disabling NMClient debug logging For printf debugging (when you recompile the source) it can be useful to have one switch to disable logging of NMClient. For example, this is useful with $ LIBNM_CLIENT_DEBUG=trace nmcli agent secret --- libnm/nm-libnm-utils.h | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index ec9f23ba8e..fd32b7c393 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -31,6 +31,8 @@ gboolean nm_utils_g_param_spec_is_default (const GParamSpec *pspec); /*****************************************************************************/ typedef enum { + _NML_DBUS_LOG_LEVEL_NONE = 0x00, + _NML_DBUS_LOG_LEVEL_INITIALIZED = 0x01, _NML_DBUS_LOG_LEVEL_TRACE = 0x02, @@ -83,6 +85,7 @@ nml_dbus_log_enabled (NMLDBusLogLevel level) int l; nm_assert (NM_IN_SET (level, NML_DBUS_LOG_LEVEL_ANY, + _NML_DBUS_LOG_LEVEL_NONE, NML_DBUS_LOG_LEVEL_TRACE, NML_DBUS_LOG_LEVEL_DEBUG, NML_DBUS_LOG_LEVEL_WARN, @@ -104,7 +107,8 @@ void _nml_dbus_log (NMLDBusLogLevel level, #define NML_DBUS_LOG(level, ...) \ G_STMT_START { \ - G_STATIC_ASSERT ( (level) == NML_DBUS_LOG_LEVEL_TRACE \ + G_STATIC_ASSERT ( (level) == _NML_DBUS_LOG_LEVEL_NONE \ + || (level) == NML_DBUS_LOG_LEVEL_TRACE \ || (level) == NML_DBUS_LOG_LEVEL_DEBUG \ || (level) == NML_DBUS_LOG_LEVEL_WARN \ || (level) == NML_DBUS_LOG_LEVEL_ERROR); \ @@ -119,8 +123,19 @@ void _nml_dbus_log (NMLDBusLogLevel level, #define NML_DBUS_LOG_W(...) NML_DBUS_LOG (NML_DBUS_LOG_LEVEL_WARN, __VA_ARGS__) #define NML_DBUS_LOG_E(...) NML_DBUS_LOG (NML_DBUS_LOG_LEVEL_ERROR, __VA_ARGS__) +/* _NML_NMCLIENT_LOG_LEVEL_COERCE is only for printf debugging. You can disable client logging by + * mapping the requested log level to a different one (or disable it altogether). + * That's useful for example if you are interested in *other* trace logging messages from + * libnm and don't want to get flooded by NMClient's trace messages. */ +#define _NML_NMCLIENT_LOG_LEVEL_COERCE(level) \ + /* for example, change condition below to suppress messages from NMClient. */ \ + (( TRUE \ + || ((level) != NML_DBUS_LOG_LEVEL_TRACE)) \ + ? (level) \ + : _NML_DBUS_LOG_LEVEL_NONE) + #define NML_NMCLIENT_LOG(level, self, ...) \ - NML_DBUS_LOG ((level), \ + NML_DBUS_LOG (_NML_NMCLIENT_LOG_LEVEL_COERCE (level), \ "nmclient["NM_HASH_OBFUSCATE_PTR_FMT"]: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ NM_HASH_OBFUSCATE_PTR (self) \ _NM_UTILS_MACRO_REST (__VA_ARGS__)) From 2c4f57be193e936af07aa043b250971292d64ae5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:09:37 +0100 Subject: [PATCH 04/22] libnm: log message when NMClient gets disposed This is useful as a last message to know when the instance is gone for good. --- libnm/nm-client.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 66a36adee3..7cb2cc5aaf 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -7682,6 +7682,8 @@ dispose (GObject *object) priv->nm.capabilities_len = 0; nm_clear_g_free (&priv->nm.capabilities_arr); + + NML_NMCLIENT_LOG_D (self, "disposed"); } const NMLDBusMetaIface _nml_dbus_meta_iface_nm_agentmanager = NML_DBUS_META_IFACE_INIT ( From 8db556372294a606e6537488e469dcf83b63b410 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:49:31 +0100 Subject: [PATCH 05/22] libnm: fix logging message about device's state change signal The device instance might already be removed from the cache. At that point, _nm_object_get_client(self) returns %NULL. Use the correct NMClient instance. --- libnm/nm-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnm/nm-device.c b/libnm/nm-device.c index 77b7da6b09..f2701cb7e5 100644 --- a/libnm/nm-device.c +++ b/libnm/nm-device.c @@ -158,7 +158,7 @@ _notify_event_state_changed (NMClient *client, gs_unref_object NMDevice *self = notify_event->user_data; NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - NML_NMCLIENT_LOG_T (_nm_object_get_client (self), + NML_NMCLIENT_LOG_T (client, "[%s] emit Device's StateChanged signal %u -> %u, reason: %u", _nm_object_get_path (self), (guint) priv->old_state, From 71fb823a43e6f0c84377c2b831a870c003c7ae84 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:09:03 +0100 Subject: [PATCH 06/22] shared/tests: add nmtst_context_busy_watcher_wait() helper --- shared/nm-utils/nm-test-utils.h | 68 +++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 495aa0b35c..3a83e5c0ea 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1040,6 +1040,74 @@ _nmtst_main_context_iterate_until_timeout (gpointer user_data) /*****************************************************************************/ +typedef struct { + GMainLoop *_main_loop; + union { + GSList *_list; + const void *const is_waiting; + }; +} NMTstContextBusyWatcherData; + +static inline void +_nmtst_context_busy_watcher_add_cb (gpointer data, + GObject *where_the_object_was) +{ + NMTstContextBusyWatcherData *watcher_data = data; + GSList *l; + + g_assert (watcher_data); + + l = g_slist_find (watcher_data->_list, where_the_object_was); + g_assert (l); + + watcher_data->_list = g_slist_delete_link (watcher_data->_list, l); + if (!watcher_data->_list) + g_main_loop_quit (watcher_data->_main_loop); +} + +static inline void +nmtst_context_busy_watcher_add (NMTstContextBusyWatcherData *watcher_data, + GObject *object) +{ + g_assert (watcher_data); + g_assert (G_IS_OBJECT (object)); + + if (!watcher_data->_main_loop) { + watcher_data->_main_loop = g_main_loop_new (g_main_context_get_thread_default (), + FALSE); + g_assert (!watcher_data->_list); + } else { + g_assert ( g_main_loop_get_context (watcher_data->_main_loop) + == (g_main_context_get_thread_default () ?: g_main_context_default ())); + } + + g_object_weak_ref (object, + _nmtst_context_busy_watcher_add_cb, + watcher_data); + watcher_data->_list = g_slist_prepend (watcher_data->_list, object); +} + +static inline void +nmtst_context_busy_watcher_wait (NMTstContextBusyWatcherData *watcher_data) +{ + g_assert (watcher_data); + + if (!watcher_data->_main_loop) { + g_assert (!watcher_data->_list); + return; + } + + if (watcher_data->_list) { + if (!nmtst_main_loop_run (watcher_data->_main_loop, 5000)) + g_error ("timeout running mainloop waiting for GObject to destruct"); + } + + g_assert (!watcher_data->_list); + nm_clear_pointer (&watcher_data->_main_loop, g_main_loop_unref); +} + +/*****************************************************************************/ + static inline const char * nmtst_get_sudo_cmd (void) { From 0008c6c801f51cca47deb444e6137d7908f2b692 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:41:14 +0100 Subject: [PATCH 07/22] shared/tests: add nmtst_g_source_set_boolean_true() helper --- shared/nm-utils/nm-test-utils.h | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 3a83e5c0ea..ef363abaee 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -967,6 +967,26 @@ nmtst_rand_perm_gslist (GRand *rand, GSList *list) /*****************************************************************************/ +static inline gboolean +nmtst_g_source_assert_not_called (gpointer user_data) +{ + g_assert_not_reached (); + return G_SOURCE_CONTINUE; +} + +static inline gboolean +nmtst_g_source_set_boolean_true (gpointer user_data) +{ + gboolean *ptr = user_data; + + g_assert (ptr); + g_assert (!*ptr); + *ptr = TRUE; + return G_SOURCE_CONTINUE; +} + +/*****************************************************************************/ + static inline gboolean _nmtst_main_loop_run_timeout (gpointer user_data) { @@ -2391,13 +2411,4 @@ nmtst_keyfile_get_num_keys (GKeyFile *keyfile, /*****************************************************************************/ -static inline gboolean -nmtst_g_source_assert_not_called (gpointer user_data) -{ - g_assert_not_reached (); - return G_SOURCE_CONTINUE; -} - -/*****************************************************************************/ - #endif /* __NM_TEST_UTILS_H__ */ From c4690eeeff8cf1b5c799e8b2272ce8559f207541 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:41:14 +0100 Subject: [PATCH 08/22] shared/tests: add nmtst_main_context_assert_no_dispatch() helper --- shared/nm-utils/nm-test-utils.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index ef363abaee..6d7084c8d5 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1060,6 +1060,26 @@ _nmtst_main_context_iterate_until_timeout (gpointer user_data) /*****************************************************************************/ +static inline void +nmtst_main_context_assert_no_dispatch (GMainContext *context, + guint timeout_msec) +{ + nm_auto_destroy_and_unref_gsource GSource *source = NULL; + gboolean timeout_hit = FALSE; + + source = g_timeout_source_new (timeout_msec); + g_source_set_callback (source, nmtst_g_source_set_boolean_true, &timeout_hit, NULL); + g_source_attach (source, context); + + while (g_main_context_iteration (context, TRUE)) { + if (timeout_hit) + return; + g_assert_not_reached (); + } +} + +/*****************************************************************************/ + typedef struct { GMainLoop *_main_loop; union { From eceaa39a1efd33e3bdf14fe04624245d591f6582 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 08:40:19 +0100 Subject: [PATCH 09/22] shared/tests: use nmtst_g_source_set_boolean_true() in nmtst_main_context_iterate_until() --- shared/nm-utils/nm-test-utils.h | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 6d7084c8d5..38eddd975b 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1030,16 +1030,6 @@ _nmtst_main_loop_quit_on_notify (GObject *object, GParamSpec *pspec, gpointer us } #define nmtst_main_loop_quit_on_notify ((GCallback) _nmtst_main_loop_quit_on_notify) -static inline gboolean -_nmtst_main_context_iterate_until_timeout (gpointer user_data) -{ - gboolean *p_had_pointer = user_data; - - g_assert (!*p_had_pointer); - *p_had_pointer = TRUE; - return G_SOURCE_CONTINUE; -} - #define nmtst_main_context_iterate_until(context, timeout_msec, condition) \ G_STMT_START { \ nm_auto_destroy_and_unref_gsource GSource *_source = NULL; \ @@ -1047,7 +1037,7 @@ _nmtst_main_context_iterate_until_timeout (gpointer user_data) gboolean _had_timeout = FALSE; \ \ _source = g_timeout_source_new (timeout_msec); \ - g_source_set_callback (_source, _nmtst_main_context_iterate_until_timeout, &_had_timeout, NULL); \ + g_source_set_callback (_source, nmtst_g_source_set_boolean_true, &_had_timeout, NULL); \ g_source_attach (_source, _context); \ \ while (TRUE) { \ From 90bb46c8ee9a25eaf541914f3e719285fbacb9e9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 08:57:47 +0100 Subject: [PATCH 10/22] shared/tests/trivial: rename nmtst_main_context_iterate_until() to nmtst_main_context_iterate_until_assert() nmtst_main_context_iterate_until*() iterates until the condition is satisfied. If that doesn't happen within timeout, it fails an assertion. Rename the function to make that clearer. --- libnm-core/tests/test-general.c | 2 +- libnm/tests/test-remote-settings-client.c | 12 ++++++------ shared/nm-utils/nm-test-utils.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 1bdaf5bd62..9ea3079e83 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -8270,7 +8270,7 @@ test_integrate_maincontext (gconstpointer test_data) g_source_set_callback (idle_source_1, _test_integrate_maincontext_cb_idle1, &count, NULL); g_source_attach (idle_source_1, c2); - nmtst_main_context_iterate_until (c1, 2000, count == 5); + nmtst_main_context_iterate_until_assert (c1, 2000, count == 5); } if (TEST_IDX == 2) { diff --git a/libnm/tests/test-remote-settings-client.c b/libnm/tests/test-remote-settings-client.c index 429eecb235..f8771a988e 100644 --- a/libnm/tests/test-remote-settings-client.c +++ b/libnm/tests/test-remote-settings-client.c @@ -62,7 +62,7 @@ test_add_connection (void) add_cb, &done); - nmtst_main_context_iterate_until (NULL, 5000, done); + nmtst_main_context_iterate_until_assert (NULL, 5000, done); g_assert (gl.remote != NULL); @@ -151,7 +151,7 @@ test_make_invisible (void) set_visible_cb, NULL); /* Wait for the connection to be removed */ - nmtst_main_context_iterate_until (NULL, 5000, visible_changed && connection_removed); + nmtst_main_context_iterate_until_assert (NULL, 5000, visible_changed && connection_removed); g_signal_handlers_disconnect_by_func (gl.remote, G_CALLBACK (visible_changed_cb), &visible_changed); g_signal_handlers_disconnect_by_func (gl.client, G_CALLBACK (connection_removed_cb), &connection_removed); @@ -225,7 +225,7 @@ test_make_visible (void) set_visible_cb, NULL); /* Wait for the settings service to announce the connection again */ - nmtst_main_context_iterate_until (NULL, 5000, new); + nmtst_main_context_iterate_until_assert (NULL, 5000, new); /* Ensure the new connection is the same as the one we made visible again */ g_assert (new == gl.remote); @@ -313,7 +313,7 @@ test_remove_connection (void) NULL, deleted_cb, NULL); - nmtst_main_context_iterate_until (NULL, 5000, done && !gl.remote); + nmtst_main_context_iterate_until_assert (NULL, 5000, done && !gl.remote); /* Ensure NMClient no longer has the connection */ conns = nm_client_get_connections (gl.client); @@ -378,7 +378,7 @@ test_add_remove_connection (void) add_remove_cb, &done); - nmtst_main_context_iterate_until (NULL, 5000, done); + nmtst_main_context_iterate_until_assert (NULL, 5000, done); } /*****************************************************************************/ @@ -417,7 +417,7 @@ test_add_bad_connection (void) &done); g_clear_object (&connection); - nmtst_main_context_iterate_until (NULL, 5000, done); + nmtst_main_context_iterate_until_assert (NULL, 5000, done); g_assert (gl.remote == NULL); } diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 38eddd975b..86dc9f9e56 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1030,7 +1030,7 @@ _nmtst_main_loop_quit_on_notify (GObject *object, GParamSpec *pspec, gpointer us } #define nmtst_main_loop_quit_on_notify ((GCallback) _nmtst_main_loop_quit_on_notify) -#define nmtst_main_context_iterate_until(context, timeout_msec, condition) \ +#define nmtst_main_context_iterate_until_assert(context, timeout_msec, condition) \ G_STMT_START { \ nm_auto_destroy_and_unref_gsource GSource *_source = NULL; \ GMainContext *_context = (context); \ From f2baa10bb8b0f7d4a00312bf6fa55f9bab027a14 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 08:40:19 +0100 Subject: [PATCH 11/22] shared/tests: add nmtst_main_context_iterate_until() helper Like nmtst_main_context_iterate_until_assert(), but allows to run into timeout. --- shared/nm-utils/nm-test-utils.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 86dc9f9e56..a3938e0121 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1030,8 +1030,8 @@ _nmtst_main_loop_quit_on_notify (GObject *object, GParamSpec *pspec, gpointer us } #define nmtst_main_loop_quit_on_notify ((GCallback) _nmtst_main_loop_quit_on_notify) -#define nmtst_main_context_iterate_until_assert(context, timeout_msec, condition) \ - G_STMT_START { \ +#define nmtst_main_context_iterate_until(context, timeout_msec, condition) \ + ({ \ nm_auto_destroy_and_unref_gsource GSource *_source = NULL; \ GMainContext *_context = (context); \ gboolean _had_timeout = FALSE; \ @@ -1044,8 +1044,17 @@ _nmtst_main_loop_quit_on_notify (GObject *object, GParamSpec *pspec, gpointer us if (condition) \ break; \ g_main_context_iteration (_context, TRUE); \ - g_assert (!_had_timeout && #condition); \ + if (_had_timeout) \ + break; \ } \ + \ + !_had_timeout; \ + }) + +#define nmtst_main_context_iterate_until_assert(context, timeout_msec, condition) \ + G_STMT_START { \ + if (!nmtst_main_context_iterate_until (context, timeout_msec, condition)) \ + g_assert (FALSE && #condition); \ } G_STMT_END /*****************************************************************************/ From 6acdc42e04598d530d66bb6753c0d89b7969d82e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 08:34:20 +0100 Subject: [PATCH 12/22] libnm/tests: extend nmtstc_client_new() to create other GObject types nmtstc_client_new() exists to test creating a GInitiable/GAsyncInitiable in different GMainContext combinations. This is not only useful for NMClient but will also be useful for NMSecretAgentOld. Add nmtstc_context_object_new() to allow for that. Also, allow passing parameters when creating the object. The resulting nmtstc_context_object_new() is relatively complex. But this is only testing code, that aims to construct the respective GObject instance in various manners (randomly using the sync or async initialization). It is complex, but delivers at testing various code paths of the underlying code. The API that it provides however is simple. Also drop _nmtstc_client_new_extra_context() to create the instance with a different context. For one, this requires that the internal context is integrated as long as the context-busy-watcher exists. That was not handled correctly. Also, creating a NMClient instance with a different context than the current thread default at construct time has implications to the test later. The tests don't want this variant, and don't handle them properly. So drop this. --- shared/nm-test-libnm-utils.h | 16 ++- shared/nm-test-utils-impl.c | 202 ++++++++++++++++++++--------------- 2 files changed, 131 insertions(+), 87 deletions(-) diff --git a/shared/nm-test-libnm-utils.h b/shared/nm-test-libnm-utils.h index 63e3b4c72c..06dbc72d71 100644 --- a/shared/nm-test-libnm-utils.h +++ b/shared/nm-test-libnm-utils.h @@ -67,4 +67,18 @@ void nmtstc_service_update_connection_variant (NMTstcServiceInfo *sinfo, GVariant *connection, gboolean verify_connection); -NMClient *nmtstc_client_new (gboolean allow_iterate_main_context); +gpointer nmtstc_context_object_new_valist (GType gtype, + gboolean allow_iterate_main_context, + const char *first_property_name, + va_list var_args); + +gpointer nmtstc_context_object_new (GType gtype, + gboolean allow_iterate_main_context, + const char *first_property_name, + ...); + +static inline NMClient * +nmtstc_client_new (gboolean allow_iterate_main_context) +{ + return nmtstc_context_object_new (NM_TYPE_CLIENT, allow_iterate_main_context, NULL); +} diff --git a/shared/nm-test-utils-impl.c b/shared/nm-test-utils-impl.c index 43176f4c7e..0494638957 100644 --- a/shared/nm-test-utils-impl.c +++ b/shared/nm-test-utils-impl.c @@ -410,35 +410,47 @@ nmtstc_service_update_connection_variant (NMTstcServiceInfo *sinfo, /*****************************************************************************/ typedef struct { + GType gtype; GMainLoop *loop; - NMClient *client; -} NMTstcClientNewData; + GObject *obj; + bool call_nm_client_new_async:1; +} NMTstcObjNewData; static void -_nmtstc_client_new_cb (GObject *source_object, - GAsyncResult *res, - gpointer user_data) +_context_object_new_do_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) { - NMTstcClientNewData *d = user_data; + NMTstcObjNewData *d = user_data; gs_free_error GError *error = NULL; - g_assert (!d->client); + g_assert (!d->obj); - d->client = nm_client_new_finish (res, - nmtst_get_rand_bool () ? &error : NULL); + if (d->call_nm_client_new_async) { + d->obj = G_OBJECT (nm_client_new_finish (res, + nmtst_get_rand_bool () ? &error : NULL)); + } else { + d->obj = g_async_initable_new_finish (G_ASYNC_INITABLE (source_object), + res, + nmtst_get_rand_bool () ? &error : NULL); + } - nmtst_assert_success (NM_IS_CLIENT (d->client), error); + nmtst_assert_success (G_IS_OBJECT (d->obj), error); + g_assert (G_OBJECT_TYPE (d->obj) == d->gtype); g_main_loop_quit (d->loop); } -static NMClient * -_nmtstc_client_new (gboolean sync) +static GObject * +_context_object_new_do (GType gtype, + gboolean sync, + const gchar *first_property_name, + va_list var_args) { gs_free_error GError *error = NULL; - NMClient *client; + GObject *obj; - /* Create a NMClient instance synchronously, and arbitrarily use either + /* Create a GObject instance synchronously, and arbitrarily use either * the sync or async constructor. * * Note that the sync and async construct differ in one important aspect: @@ -456,125 +468,128 @@ _nmtstc_client_new (gboolean sync) g_source_attach (source, g_main_context_get_thread_default ()); } - if (nmtst_get_rand_bool ()) { + if ( gtype != NM_TYPE_CLIENT + || first_property_name + || nmtst_get_rand_bool ()) { gboolean success; - client = g_object_new (NM_TYPE_CLIENT, NULL); - g_assert (NM_IS_CLIENT (client)); + if ( first_property_name + || nmtst_get_rand_bool ()) + obj = g_object_new_valist (gtype, first_property_name, var_args); + else + obj = g_object_new (gtype, NULL); - success = g_initable_init (G_INITABLE (client), + success = g_initable_init (G_INITABLE (obj), NULL, nmtst_get_rand_bool () ? &error : NULL); nmtst_assert_success (success, error); } else { - client = nm_client_new (NULL, - nmtst_get_rand_bool () ? &error : NULL); + obj = G_OBJECT (nm_client_new (NULL, + nmtst_get_rand_bool () ? &error : NULL)); } } else { nm_auto_unref_gmainloop GMainLoop *loop = NULL; - NMTstcClientNewData d = { .loop = NULL, }; + NMTstcObjNewData d = { + .gtype = gtype, + .loop = NULL, + }; + gs_unref_object GObject *obj2 = NULL; loop = g_main_loop_new (g_main_context_get_thread_default (), FALSE); - d.loop = loop; - nm_client_new_async (NULL, - _nmtstc_client_new_cb, - &d); + + if ( gtype != NM_TYPE_CLIENT + || first_property_name + || nmtst_get_rand_bool ()) { + if ( first_property_name + || nmtst_get_rand_bool ()) + obj2 = g_object_new_valist (gtype, first_property_name, var_args); + else + obj2 = g_object_new (gtype, NULL); + + g_async_initable_init_async (G_ASYNC_INITABLE (obj2), + G_PRIORITY_DEFAULT, + NULL, + _context_object_new_do_cb, + &d); + } else { + d.call_nm_client_new_async = TRUE; + nm_client_new_async (NULL, + _context_object_new_do_cb, + &d); + } g_main_loop_run (loop); - g_assert (NM_IS_CLIENT (d.client)); - client = d.client; + obj = d.obj; + g_assert (!obj2 || obj == obj2); } - nmtst_assert_success (NM_IS_CLIENT (client), error); - return client; + nmtst_assert_success (G_IS_OBJECT (obj), error); + g_assert (G_OBJECT_TYPE (obj) == gtype); + return obj; } typedef struct { + GType gtype; + const char *first_property_name; + va_list var_args; GMainLoop *loop; - NMClient *client; + GObject *obj; bool sync; } NewSyncInsideDispatchedData; static gboolean -_nmtstc_client_new_inside_loop_do (gpointer user_data) +_context_object_new_inside_loop_do (gpointer user_data) { NewSyncInsideDispatchedData *d = user_data; g_assert (d->loop); - g_assert (!d->client); + g_assert (!d->obj); - d->client = nmtstc_client_new (d->sync); + d->obj = nmtstc_context_object_new_valist (d->gtype, d->sync, d->first_property_name, d->var_args); g_main_loop_quit (d->loop); return G_SOURCE_CONTINUE; } -static NMClient * -_nmtstc_client_new_inside_loop (gboolean sync) +static GObject * +_context_object_new_inside_loop (GType gtype, + gboolean sync, + const char *first_property_name, + va_list var_args) { GMainContext *context = g_main_context_get_thread_default (); nm_auto_unref_gmainloop GMainLoop *loop = g_main_loop_new (context, FALSE); NewSyncInsideDispatchedData d = { - .sync = sync, - .loop = loop, + .gtype = gtype, + .first_property_name = first_property_name, + .sync = sync, + .loop = loop, }; nm_auto_destroy_and_unref_gsource GSource *source = NULL; + va_copy (d.var_args, var_args); + source = g_idle_source_new (); - g_source_set_callback (source, _nmtstc_client_new_inside_loop_do, &d, NULL); + g_source_set_callback (source, _context_object_new_inside_loop_do, &d, NULL); g_source_attach (source, context); g_main_loop_run (loop); - g_assert (NM_IS_CLIENT (d.client)); - return d.client; + + va_end (d.var_args); + + g_assert (G_IS_OBJECT (d.obj)); + g_assert (G_OBJECT_TYPE (d.obj) == gtype); + return d.obj; } -static NMClient * -_nmtstc_client_new_extra_context (void) -{ - GMainContext *inner_context; - NMClient *client; - GSource *source; - guint key_idx; - - inner_context = g_main_context_new (); - g_main_context_push_thread_default (inner_context); - - client = nmtstc_client_new (TRUE); - - source = nm_utils_g_main_context_create_integrate_source (inner_context); - - g_main_context_pop_thread_default (inner_context); - g_main_context_unref (inner_context); - - g_source_attach (source, g_main_context_get_thread_default ()); - - for (key_idx = 0; TRUE; key_idx++) { - char s[100]; - - /* nmtstc_client_new() may call _nmtstc_client_new_extra_context() repeatedly. We - * need to attach the source to a previously unused key. */ - nm_sprintf_buf (s, "nm-test-extra-context-%u", key_idx); - if (!g_object_get_data (G_OBJECT (client), s)) { - g_object_set_data_full (G_OBJECT (client), - s, - source, - (GDestroyNotify) nm_g_source_destroy_and_unref); - break; - } - } - - return client; -} - -NMClient * -nmtstc_client_new (gboolean allow_iterate_main_context) +gpointer +nmtstc_context_object_new_valist (GType gtype, + gboolean allow_iterate_main_context, + const char *first_property_name, + va_list var_args) { gboolean inside_loop; gboolean sync; - if (nmtst_get_rand_uint32 () % 5 == 0) - return _nmtstc_client_new_extra_context (); - if (!allow_iterate_main_context) { sync = TRUE; inside_loop = FALSE; @@ -587,11 +602,26 @@ nmtstc_client_new (gboolean allow_iterate_main_context) } if (inside_loop) { - /* Create the client on an idle handler of the current context. + /* Create the obj on an idle handler of the current context. * In practice, it should make no difference, which this check * tries to prove. */ - return _nmtstc_client_new_inside_loop (sync); + return _context_object_new_inside_loop (gtype, sync, first_property_name, var_args); } - return _nmtstc_client_new (sync); + return _context_object_new_do (gtype, sync, first_property_name, var_args); +} + +gpointer +nmtstc_context_object_new (GType gtype, + gboolean allow_iterate_main_context, + const char *first_property_name, + ...) +{ + GObject *obj; + va_list var_args; + + va_start (var_args, first_property_name); + obj = nmtstc_context_object_new_valist (gtype, allow_iterate_main_context, first_property_name, var_args); + va_end (var_args); + return obj; } From 15888fc3a8a0ed1a7b508dafd1269c127340565c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 16:04:09 +0100 Subject: [PATCH 13/22] libnm/tests: cleanup add_device_common() test helper - use NMClient's GMainContext instead of the default main context. - add some more assertions. - use cleanup attribute to free resources. --- shared/nm-test-utils-impl.c | 72 +++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 31 deletions(-) diff --git a/shared/nm-test-utils-impl.c b/shared/nm-test-utils-impl.c index 0494638957..0ef6591f32 100644 --- a/shared/nm-test-utils-impl.c +++ b/shared/nm-test-utils-impl.c @@ -216,7 +216,7 @@ again_wait: typedef struct { GMainLoop *loop; const char *ifname; - char *path; + const char *path; NMDevice *device; } AddDeviceInfo; @@ -227,21 +227,17 @@ device_added_cb (NMClient *client, { AddDeviceInfo *info = user_data; - g_assert (device); + g_assert (info); + g_assert (!info->device); + + g_assert (NM_IS_DEVICE (device)); g_assert_cmpstr (nm_object_get_path (NM_OBJECT (device)), ==, info->path); g_assert_cmpstr (nm_device_get_iface (device), ==, info->ifname); - info->device = device; + info->device = g_object_ref (device); g_main_loop_quit (info->loop); } -static gboolean -timeout (gpointer user_data) -{ - g_assert_not_reached (); - return G_SOURCE_REMOVE; -} - static GVariant * call_add_wired_device (GDBusProxy *proxy, const char *ifname, const char *hwaddr, const char **subchannels, GError **error) @@ -275,39 +271,53 @@ call_add_device (GDBusProxy *proxy, const char *method, const char *ifname, GErr } static NMDevice * -add_device_common (NMTstcServiceInfo *sinfo, NMClient *client, - const char *method, const char *ifname, - const char *hwaddr, const char **subchannels) +add_device_common (NMTstcServiceInfo *sinfo, + NMClient *client, + const char *method, + const char *ifname, + const char *hwaddr, + const char **subchannels) { + nm_auto_unref_gmainloop GMainLoop *loop = NULL; + gs_unref_variant GVariant *ret = NULL; + gs_free_error GError *error = NULL; AddDeviceInfo info; - GError *error = NULL; - GVariant *ret; - guint timeout_id; - if (g_strcmp0 (method, "AddWiredDevice") == 0) + g_assert (sinfo); + g_assert (NM_IS_CLIENT (client)); + + if (nm_streq0 (method, "AddWiredDevice")) ret = call_add_wired_device (sinfo->proxy, ifname, hwaddr, subchannels, &error); else ret = call_add_device (sinfo->proxy, method, ifname, &error); - g_assert_no_error (error); - g_assert (ret); + nmtst_assert_success (ret, error); g_assert_cmpstr (g_variant_get_type_string (ret), ==, "(o)"); - g_variant_get (ret, "(o)", &info.path); - g_variant_unref (ret); - /* Wait for libnm to find the device */ - info.ifname = ifname; - info.loop = g_main_loop_new (NULL, FALSE); - g_signal_connect (client, "device-added", - G_CALLBACK (device_added_cb), &info); - timeout_id = g_timeout_add_seconds (5, timeout, NULL); - g_main_loop_run (info.loop); + /* Wait for NMClient to find the device */ + + loop = g_main_loop_new (nm_client_get_main_context (client), FALSE); + + info = (AddDeviceInfo) { + .ifname = ifname, + .loop = loop, + }; + g_variant_get (ret, "(&o)", &info.path); + + g_signal_connect (client, + NM_CLIENT_DEVICE_ADDED, + G_CALLBACK (device_added_cb), + &info); + + if (!nmtst_main_loop_run (loop, 5000)) + g_assert_not_reached (); - g_source_remove (timeout_id); g_signal_handlers_disconnect_by_func (client, device_added_cb, &info); - g_free (info.path); - g_main_loop_unref (info.loop); + g_assert (NM_IS_DEVICE (info.device)); + + g_assert (info.device == nm_client_get_device_by_path (client, nm_object_get_path (NM_OBJECT (info.device)))); + g_object_unref (info.device); return info.device; } From 18512274ea7b253452eb38a6df214b1473aceac9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 07:32:36 +0100 Subject: [PATCH 14/22] libnm/secret-agent/tests: cleanup test code --- libnm/tests/test-secret-agent.c | 112 +++++++++++++++----------------- 1 file changed, 52 insertions(+), 60 deletions(-) diff --git a/libnm/tests/test-secret-agent.c b/libnm/tests/test-secret-agent.c index fef6071df9..317098c853 100644 --- a/libnm/tests/test-secret-agent.c +++ b/libnm/tests/test-secret-agent.c @@ -16,22 +16,17 @@ enum { SECRET_REQUESTED, - - LAST_SIGNAL + LAST_SIGNAL, }; static guint signals[LAST_SIGNAL] = { 0 }; -typedef NMSecretAgentOld TestSecretAgent; +typedef NMSecretAgentOld TestSecretAgent; typedef NMSecretAgentOldClass TestSecretAgentClass; GType test_secret_agent_get_type (void); -G_DEFINE_TYPE (TestSecretAgent, test_secret_agent, NM_TYPE_SECRET_AGENT_OLD) -static void -test_secret_agent_init (TestSecretAgent *agent) -{ -} +G_DEFINE_TYPE (TestSecretAgent, test_secret_agent, NM_TYPE_SECRET_AGENT_OLD) static void test_secret_agent_get_secrets (NMSecretAgentOld *agent, @@ -122,27 +117,8 @@ test_secret_agent_delete_secrets (NMSecretAgentOld *agent, } static void -test_secret_agent_class_init (TestSecretAgentClass *klass) +test_secret_agent_init (TestSecretAgent *agent) { - GObjectClass *object_class = G_OBJECT_CLASS (klass); - NMSecretAgentOldClass *agent_class = NM_SECRET_AGENT_OLD_CLASS (klass); - - agent_class->get_secrets = test_secret_agent_get_secrets; - agent_class->cancel_get_secrets = test_secret_agent_cancel_get_secrets; - agent_class->save_secrets = test_secret_agent_save_secrets; - agent_class->delete_secrets = test_secret_agent_delete_secrets; - - signals[SECRET_REQUESTED] = - g_signal_new ("secret-requested", - G_OBJECT_CLASS_TYPE (object_class), - G_SIGNAL_RUN_LAST, - 0, NULL, NULL, NULL, - G_TYPE_STRING, 4, - NM_TYPE_CONNECTION, - G_TYPE_STRING, - G_TYPE_STRING, - G_TYPE_STRING); - } static NMSecretAgentOld * @@ -151,15 +127,39 @@ test_secret_agent_new (void) NMSecretAgentOld *agent; GError *error = NULL; - agent = g_initable_new (test_secret_agent_get_type (), NULL, &error, + agent = g_initable_new (test_secret_agent_get_type (), + NULL, + &error, NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", NM_SECRET_AGENT_OLD_AUTO_REGISTER, FALSE, NULL); - g_assert_no_error (error); - + nmtst_assert_success (agent, error); return agent; } +static void +test_secret_agent_class_init (TestSecretAgentClass *klass) +{ + GObjectClass *object_class = G_OBJECT_CLASS (klass); + NMSecretAgentOldClass *agent_class = NM_SECRET_AGENT_OLD_CLASS (klass); + + agent_class->get_secrets = test_secret_agent_get_secrets; + agent_class->cancel_get_secrets = test_secret_agent_cancel_get_secrets; + agent_class->save_secrets = test_secret_agent_save_secrets; + agent_class->delete_secrets = test_secret_agent_delete_secrets; + + signals[SECRET_REQUESTED] = + g_signal_new ("secret-requested", + G_OBJECT_CLASS_TYPE (object_class), + G_SIGNAL_RUN_LAST, + 0, NULL, NULL, NULL, + G_TYPE_STRING, 4, + NM_TYPE_CONNECTION, + G_TYPE_STRING, + G_TYPE_STRING, + G_TYPE_STRING); +} + /*****************************************************************************/ typedef struct { @@ -171,7 +171,7 @@ typedef struct { NMConnection *connection; GMainLoop *loop; - guint timeout_id; + GSource *timeout_source; char *ifname; char *con_id; @@ -179,12 +179,6 @@ typedef struct { int secrets_requested; } TestSecretAgentData; -static gboolean -timeout_assert (gpointer user_data) -{ - g_assert_not_reached (); -} - static void connection_added_cb (GObject *s, GAsyncResult *result, @@ -221,14 +215,15 @@ register_cb (GObject *object, GAsyncResult *result, gpointer user_data) static void test_setup (TestSecretAgentData *sadata, gconstpointer test_data) { - static int counter = 0; + static int static_counter = 0; + const int counter = static_counter++; const char *agent_notes = test_data; NMConnection *connection; NMSettingConnection *s_con; NMSettingWireless *s_wireless; GBytes *ssid; NMSetting *s_wsec; - GError *error = NULL; + gs_free_error GError *error = NULL; sadata->sinfo = nmtstc_service_init (); if (!sadata->sinfo) @@ -239,15 +234,18 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data) sadata->client = nmtstc_client_new (TRUE); sadata->loop = g_main_loop_new (NULL, FALSE); - sadata->timeout_id = g_timeout_add_seconds (5, timeout_assert, NULL); + + sadata->timeout_source = g_timeout_source_new_seconds (5); + g_source_set_callback (sadata->timeout_source, nmtst_g_source_assert_not_called, NULL, NULL); + g_source_attach (sadata->timeout_source, NULL); sadata->ifname = g_strdup_printf ("wlan%d", counter); sadata->con_id = g_strdup_printf ("%s-%d", TEST_CON_ID_PREFIX, counter); - counter++; - /* Create the device */ - sadata->device = nmtstc_service_add_device (sadata->sinfo, sadata->client, - "AddWifiDevice", sadata->ifname); + sadata->device = nmtstc_service_add_device (sadata->sinfo, + sadata->client, + "AddWifiDevice", + sadata->ifname); /* Create the connection */ connection = nmtst_create_minimal_connection (sadata->con_id, NULL, NM_SETTING_WIRELESS_SETTING_NAME, &s_con); @@ -325,11 +323,14 @@ test_cleanup (TestSecretAgentData *sadata, gconstpointer test_data) nmtstc_service_cleanup (sadata->sinfo); - g_source_remove (sadata->timeout_id); + nm_clear_g_source_inst (&sadata->timeout_source); + g_main_loop_unref (sadata->loop); g_free (sadata->ifname); g_free (sadata->con_id); + + *sadata = (TestSecretAgentData) { }; } /*****************************************************************************/ @@ -639,25 +640,16 @@ NMTST_DEFINE (); int main (int argc, char **argv) { - int ret; - g_setenv ("LIBNM_USE_SESSION_BUS", "1", TRUE); nmtst_init (&argc, &argv, TRUE); - g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, NULL, - test_setup, test_secret_agent_none, test_cleanup); - g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "sync", - test_setup, test_secret_agent_no_secrets, test_cleanup); - g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "async", - test_setup, test_secret_agent_cancel, test_cleanup); - g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "async", - test_setup, test_secret_agent_good, test_cleanup); + g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, NULL, test_setup, test_secret_agent_none, test_cleanup); + g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "sync", test_setup, test_secret_agent_no_secrets, test_cleanup); + g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "async", test_setup, test_secret_agent_cancel, test_cleanup); + g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "async", test_setup, test_secret_agent_good, test_cleanup); g_test_add_func ("/libnm/secret-agent/nm-not-running", test_secret_agent_nm_not_running); g_test_add_func ("/libnm/secret-agent/auto-register", test_secret_agent_auto_register); - ret = g_test_run (); - - return ret; + return g_test_run (); } - From 8dc760d2c2edeeed550de3289d4519ff27e13cfa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 10:04:38 +0100 Subject: [PATCH 15/22] libnm/secret-agent/tests: test async/sync initialization of NMSecretAgentOld Use nmtstc_context_object_new() to create the NMSecretAgentOld. This randomly uses sync or async initialization, and checks whether the main context gets iterated. --- libnm/tests/test-secret-agent.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/libnm/tests/test-secret-agent.c b/libnm/tests/test-secret-agent.c index 317098c853..a6d745932f 100644 --- a/libnm/tests/test-secret-agent.c +++ b/libnm/tests/test-secret-agent.c @@ -124,17 +124,11 @@ test_secret_agent_init (TestSecretAgent *agent) static NMSecretAgentOld * test_secret_agent_new (void) { - NMSecretAgentOld *agent; - GError *error = NULL; - - agent = g_initable_new (test_secret_agent_get_type (), - NULL, - &error, - NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", - NM_SECRET_AGENT_OLD_AUTO_REGISTER, FALSE, - NULL); - nmtst_assert_success (agent, error); - return agent; + return nmtstc_context_object_new (test_secret_agent_get_type (), + TRUE, + NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", + NM_SECRET_AGENT_OLD_AUTO_REGISTER, FALSE, + NULL); } static void From 13d050a3b788adf9cac81f114d09572e9dd5ee1c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 3 Jan 2020 13:17:56 +0100 Subject: [PATCH 16/22] libnm/secret-agent/tests: iterate main context during test cleanup The test only uses one GMainContext (the g_main_context_get_default() singleton. Between tests, ensure that we iterate the main context long enough, so that no more sources from the previous test are queued. Otherwise, there is an ugly dependency between tests and the order in which they run. --- libnm/tests/test-secret-agent.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/libnm/tests/test-secret-agent.c b/libnm/tests/test-secret-agent.c index a6d745932f..26df6f3c6e 100644 --- a/libnm/tests/test-secret-agent.c +++ b/libnm/tests/test-secret-agent.c @@ -223,10 +223,13 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data) if (!sadata->sinfo) return; - g_assert (g_main_context_get_thread_default () == NULL); + g_assert (nm_g_main_context_is_thread_default (NULL)); sadata->client = nmtstc_client_new (TRUE); + g_assert (nm_g_main_context_is_thread_default (NULL)); + g_assert (nm_g_main_context_is_thread_default (nm_client_get_main_context (sadata->client))); + sadata->loop = g_main_loop_new (NULL, FALSE); sadata->timeout_source = g_timeout_source_new_seconds (5); @@ -290,10 +293,18 @@ test_cleanup (TestSecretAgentData *sadata, gconstpointer test_data) { GVariant *ret; GError *error = NULL; + NMTstContextBusyWatcherData watcher_data = { }; + + g_assert (nm_g_main_context_is_thread_default (NULL)); if (!sadata->sinfo) return; + g_assert (nm_g_main_context_is_thread_default (nm_client_get_main_context (sadata->client))); + + nmtst_context_busy_watcher_add (&watcher_data, + nm_client_get_context_busy_watcher (sadata->client)); + if (sadata->agent) { if (nm_secret_agent_old_get_registered (sadata->agent)) { nm_secret_agent_old_unregister (sadata->agent, NULL, &error); @@ -325,6 +336,13 @@ test_cleanup (TestSecretAgentData *sadata, gconstpointer test_data) g_free (sadata->con_id); *sadata = (TestSecretAgentData) { }; + + nmtst_context_busy_watcher_wait (&watcher_data); + + while (g_main_context_iteration (NULL, FALSE)) { + } + + nmtst_main_context_assert_no_dispatch (NULL, nmtst_get_rand_uint32 () % 500); } /*****************************************************************************/ From 50bda649b177dcb19525a0c9feb9858fda511474 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 Dec 2019 13:59:15 +0100 Subject: [PATCH 17/22] libnm: expose nm_context_busy_watcher_integrate_source() as internal API for reuse This will also be useful for NMSecretAgentOld. The mechanics how NMClient handles the GMainContext and the context-busy-watcher apply really to every GObject that uses GDBusConnection and registers to signals. At least, as long as the API provides no shutdown/stop method, because that means shutdown/stop happens when unreferencing the instance, at which point pending operations get cancelled (but they cannot complete right away due to the nature of GTask and g_dbus_connection_call()). If there is a shutdown/stop API, then all pending operations could keep the instance alive, and the instance would sticks around (and keeps the GMainContext busy) until shutdown is completed. Basically, then the instance could be the context-busy-watcher itself. But in existing API which does not require the user to explicitly shutdown, that is not a feasible (backward compatible) addition. But the context-busy-watcher object is. --- libnm/nm-client.c | 38 +++++++++++++++++++++++--------------- libnm/nm-libnm-utils.h | 8 ++++++++ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 7cb2cc5aaf..47be50e7cd 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -60,7 +60,7 @@ /*****************************************************************************/ -static NM_CACHED_QUARK_FCN ("nm-client-context-busy-watcher", nm_client_context_busy_watcher_quark) +NM_CACHED_QUARK_FCN ("nm-context-busy-watcher", nm_context_busy_watcher_quark) static void _context_busy_watcher_attach_integration_source_cb (gpointer data, @@ -69,10 +69,21 @@ _context_busy_watcher_attach_integration_source_cb (gpointer data, nm_g_source_destroy_and_unref (data); } -static void -_context_busy_watcher_attach_integration_source (GObject *context_busy_watcher, - GSource *source_take) +void +nm_context_busy_watcher_integrate_source (GMainContext *outer_context, + GMainContext *inner_context, + GObject *context_busy_watcher) { + GSource *source; + + nm_assert (outer_context); + nm_assert (inner_context); + nm_assert (outer_context != inner_context); + nm_assert (G_IS_OBJECT (context_busy_watcher)); + + source = nm_utils_g_main_context_create_integrate_source (inner_context); + g_source_attach (source, outer_context); + /* The problem is... * * NMClient is associated with a GMainContext, just like its underlying GDBusConnection @@ -114,7 +125,7 @@ _context_busy_watcher_attach_integration_source (GObject *context_busy_watcher, g_object_weak_ref (context_busy_watcher, _context_busy_watcher_attach_integration_source_cb, - source_take); + source); } /*****************************************************************************/ @@ -1008,7 +1019,7 @@ nm_client_get_context_busy_watcher (NMClient *self) g_return_val_if_fail (NM_IS_CLIENT (self), NULL); w = NM_CLIENT_GET_PRIVATE (self)->context_busy_watcher; - return g_object_get_qdata (w, nm_client_context_busy_watcher_quark ()) + return g_object_get_qdata (w, nm_context_busy_watcher_quark ()) ?: w; } @@ -6816,7 +6827,7 @@ name_owner_changed (NMClient *self, old_context_busy_watcher = g_steal_pointer (&priv->context_busy_watcher); priv->context_busy_watcher = g_object_ref (g_object_get_qdata (old_context_busy_watcher, - nm_client_context_busy_watcher_quark ())); + nm_context_busy_watcher_quark ())); g_main_context_ref (priv->main_context); g_main_context_unref (priv->dbus_context); @@ -7379,12 +7390,12 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) * to resync and drop the inner context. That means, requests made against the inner * context have a different lifetime. Hence, we create a separate tracking * object. This "wraps" the outer context-busy-watcher and references it, so - * that the work together. Grep for nm_client_context_busy_watcher_quark() to + * that the work together. Grep for nm_context_busy_watcher_quark() to * see how this works. */ parent_context_busy_watcher = g_steal_pointer (&priv->context_busy_watcher); priv->context_busy_watcher = g_object_new (G_TYPE_OBJECT, NULL); g_object_set_qdata_full (priv->context_busy_watcher, - nm_client_context_busy_watcher_quark (), + nm_context_busy_watcher_quark (), parent_context_busy_watcher, g_object_unref); @@ -7403,12 +7414,9 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) g_main_context_pop_thread_default (dbus_context); if (priv->main_context != priv->dbus_context) { - GSource *source; - - source = nm_utils_g_main_context_create_integrate_source (priv->dbus_context); - g_source_attach (source, priv->main_context); - _context_busy_watcher_attach_integration_source (priv->context_busy_watcher, - g_steal_pointer (&source)); + nm_context_busy_watcher_integrate_source (priv->main_context, + priv->dbus_context, + priv->context_busy_watcher); } g_main_context_unref (dbus_context); diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index fd32b7c393..c8f0d29095 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -175,6 +175,14 @@ _nml_coerce_property_strv_not_null (char **strv) /*****************************************************************************/ +GQuark nm_context_busy_watcher_quark (void); + +void nm_context_busy_watcher_integrate_source (GMainContext *outer_context, + GMainContext *inner_context, + GObject *context_busy_watcher); + +/*****************************************************************************/ + typedef struct _NMLDBusObject NMLDBusObject; typedef struct _NMLDBusObjWatcher NMLDBusObjWatcher; typedef struct _NMLDBusMetaIface NMLDBusMetaIface; From 718e524a7f8018336536531b8bc949d9aba68b1a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 Dec 2019 14:38:29 +0100 Subject: [PATCH 18/22] libnm: move InitData to nm-libnm-utils.h for sharing It can be reused. --- libnm/nm-client.c | 50 ++++++++++++++---------------------------- libnm/nm-libnm-utils.h | 25 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 47be50e7cd..b89f0e1900 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -186,22 +186,6 @@ typedef struct { /*****************************************************************************/ -typedef struct { - GCancellable *cancellable; - GSource *cancel_on_idle_source; - gulong cancelled_id; - union { - struct { - GTask *task; - } async; - struct { - GMainLoop *main_loop; - GError **error_location; - } sync; - } data; - bool is_sync:1; -} InitData; - NM_GOBJECT_PROPERTIES_DEFINE (NMClient, PROP_DBUS_CONNECTION, PROP_DBUS_NAME_OWNER, @@ -274,7 +258,7 @@ typedef struct { GMainContext *dbus_context; GObject *context_busy_watcher; GDBusConnection *dbus_connection; - InitData *init_data; + NMLInitData *init_data; GHashTable *dbus_objects; CList obj_changed_lst_head; GCancellable *name_owner_get_cancellable; @@ -420,15 +404,15 @@ NM_CACHED_QUARK_FCN ("nm-client-error-quark", nm_client_error_quark) /*****************************************************************************/ -static InitData * -_init_data_new_sync (GCancellable *cancellable, - GMainLoop *main_loop, - GError **error_location) +NMLInitData * +nml_init_data_new_sync (GCancellable *cancellable, + GMainLoop *main_loop, + GError **error_location) { - InitData *init_data; + NMLInitData *init_data; - init_data = g_slice_new (InitData); - *init_data = (InitData) { + init_data = g_slice_new (NMLInitData); + *init_data = (NMLInitData) { .cancellable = nm_g_object_ref (cancellable), .is_sync = TRUE, .data.sync = { @@ -439,14 +423,14 @@ _init_data_new_sync (GCancellable *cancellable, return init_data; } -static InitData * -_init_data_new_async (GCancellable *cancellable, - GTask *task_take) +NMLInitData * +nml_init_data_new_async (GCancellable *cancellable, + GTask *task_take) { - InitData *init_data; + NMLInitData *init_data; - init_data = g_slice_new (InitData); - *init_data = (InitData) { + init_data = g_slice_new (NMLInitData); + *init_data = (NMLInitData) { .cancellable = nm_g_object_ref (cancellable), .is_sync = FALSE, .data.async = { @@ -6968,7 +6952,7 @@ _init_start_complete (NMClient *self, GError *error_take) { NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (self); - InitData *init_data; + NMLInitData *init_data; init_data = g_steal_pointer (&priv->init_data); @@ -7403,7 +7387,7 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) main_loop = g_main_loop_new (dbus_context, FALSE); - priv->init_data = _init_data_new_sync (cancellable, main_loop, &local_error); + priv->init_data = nml_init_data_new_sync (cancellable, main_loop, &local_error); _init_start (self); @@ -7456,7 +7440,7 @@ init_async (GAsyncInitable *initable, task = nm_g_task_new (self, cancellable, init_async, callback, user_data); g_task_set_priority (task, io_priority); - priv->init_data = _init_data_new_async (cancellable, g_steal_pointer (&task)); + priv->init_data = nml_init_data_new_async (cancellable, g_steal_pointer (&task)); _init_start (self); } diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index c8f0d29095..2228612a9a 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -183,6 +183,31 @@ void nm_context_busy_watcher_integrate_source (GMainContext *outer_context, /*****************************************************************************/ +typedef struct { + GCancellable *cancellable; + GSource *cancel_on_idle_source; + gulong cancelled_id; + union { + struct { + GTask *task; + } async; + struct { + GMainLoop *main_loop; + GError **error_location; + } sync; + } data; + bool is_sync:1; +} NMLInitData; + +NMLInitData *nml_init_data_new_sync (GCancellable *cancellable, + GMainLoop *main_loop, + GError **error_location); + +NMLInitData *nml_init_data_new_async (GCancellable *cancellable, + GTask *task_take); + +/*****************************************************************************/ + typedef struct _NMLDBusObject NMLDBusObject; typedef struct _NMLDBusObjWatcher NMLDBusObjWatcher; typedef struct _NMLDBusMetaIface NMLDBusMetaIface; From 587c35b1f4cb4e8bf57c15db749bdac51c8505bc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 18 Dec 2019 17:12:45 +0100 Subject: [PATCH 19/22] libnm: factor out nml_init_data_return() for reuse --- libnm/nm-client.c | 47 ++++++++++++++++++++++++------------------ libnm/nm-libnm-utils.h | 3 +++ 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index b89f0e1900..70fff47b92 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -440,6 +440,30 @@ nml_init_data_new_async (GCancellable *cancellable, return init_data; } +void +nml_init_data_return (NMLInitData *init_data, + GError *error_take) +{ + nm_assert (init_data); + + nm_clear_pointer (&init_data->cancel_on_idle_source, nm_g_source_destroy_and_unref); + nm_clear_g_signal_handler (init_data->cancellable, &init_data->cancelled_id); + + if (init_data->is_sync) { + if (error_take) + g_propagate_error (init_data->data.sync.error_location, error_take); + g_main_loop_quit (init_data->data.sync.main_loop); + } else { + if (error_take) + g_task_return_error (init_data->data.async.task, error_take); + else + g_task_return_boolean (init_data->data.async.task, TRUE); + g_object_unref (init_data->data.async.task); + } + nm_g_object_unref (init_data->cancellable); + nm_g_slice_free (init_data); +} + /*****************************************************************************/ GError * @@ -6952,30 +6976,13 @@ _init_start_complete (NMClient *self, GError *error_take) { NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE (self); - NMLInitData *init_data; - - init_data = g_steal_pointer (&priv->init_data); NML_NMCLIENT_LOG_D (self, "%s init complete with %s%s%s", - init_data->is_sync ? "sync" : "async", + priv->init_data->is_sync ? "sync" : "async", NM_PRINT_FMT_QUOTED (error_take, "error: ", error_take->message, "", "success")); - nm_clear_pointer (&init_data->cancel_on_idle_source, nm_g_source_destroy_and_unref); - nm_clear_g_signal_handler (init_data->cancellable, &init_data->cancelled_id); - - if (init_data->is_sync) { - if (error_take) - g_propagate_error (init_data->data.sync.error_location, error_take); - g_main_loop_quit (init_data->data.sync.main_loop); - } else { - if (error_take) - g_task_return_error (init_data->data.async.task, error_take); - else - g_task_return_boolean (init_data->data.async.task, TRUE); - g_object_unref (init_data->data.async.task); - } - nm_g_object_unref (init_data->cancellable); - nm_g_slice_free (init_data); + nml_init_data_return (g_steal_pointer (&priv->init_data), + error_take); } static void diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index 2228612a9a..c28e8a622b 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -206,6 +206,9 @@ NMLInitData *nml_init_data_new_sync (GCancellable *cancellable, NMLInitData *nml_init_data_new_async (GCancellable *cancellable, GTask *task_take); +void nml_init_data_return (NMLInitData *init_data, + GError *error_take); + /*****************************************************************************/ typedef struct _NMLDBusObject NMLDBusObject; From 0382e54d8d0c8515f5a0ccb2f9e92ccb787bb7c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 20 Jan 2020 11:09:32 +0100 Subject: [PATCH 20/22] libnm: expose nml_cleanup_context_busy_watcher_on_idle() helper for reuse This can be used by NMSecretAgentOld. --- libnm/nm-client.c | 112 +++++++++++++++++++++++------------------ libnm/nm-libnm-utils.h | 3 ++ 2 files changed, 67 insertions(+), 48 deletions(-) diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 70fff47b92..b530d7d671 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -6971,6 +6971,68 @@ name_owner_get_call (NMClient *self) /*****************************************************************************/ +static inline gboolean +_nml_cleanup_context_busy_watcher_on_idle_cb (gpointer user_data) +{ + nm_auto_unref_gmaincontext GMainContext *context = NULL; + gs_unref_object GObject *context_busy_watcher = NULL; + + nm_utils_user_data_unpack (user_data, &context, &context_busy_watcher); + + nm_assert (context); + nm_assert (G_IS_OBJECT (context_busy_watcher)); + return G_SOURCE_REMOVE; +} + +void +nml_cleanup_context_busy_watcher_on_idle (GObject *context_busy_watcher_take, + GMainContext *context) +{ + gs_unref_object GObject *context_busy_watcher = g_steal_pointer (&context_busy_watcher_take); + GSource *cleanup_source; + + nm_assert (G_IS_OBJECT (context_busy_watcher)); + nm_assert (context); + + /* Technically, we cancelled all pending actions (and these actions + * (GTask) keep the context_busy_watcher object alive). Also, we passed + * no destroy notify to g_dbus_connection_signal_subscribe(). + * That means, there should be no other unaccounted GSource'es left. + * + * However, we really need to be sure that the context_busy_watcher's + * lifetime matches the time that the context is busy. That is especially + * important with synchronous initialization, where the context-busy-watcher + * keeps the inner GMainContext integrated in the caller's. + * We must not g_source_destroy() that integration too early. + * + * So to be really sure all this is given, always schedule one last + * cleanup idle action with low priority. This should be the last + * thing related to this instance that keeps the context busy. + * + * Note that we could also *not* take a reference on @context + * and unref @context_busy_watcher via the GDestroyNotify. That would + * allow for the context to be wrapped up early, and when the last user + * gives up the reference to the context, the destroy notify could complete + * without even invoke the idle handler. However, that destroy notify may + * not be called in the right thread. So, we want to be sure that we unref + * the context-busy-watcher in the right context. Hence, we always take an + * additional reference and always cleanup in the idle handler. This means: + * the user *MUST* always keep iterating the context after NMClient got destroyed. + * But that is not a severe limitation, because the user anyway must be prepared + * to do that. That is because in many cases it is necessary anyway (and the user + * wouldn't know a priory when not). This way, it is just always necessary. */ + + cleanup_source = nm_g_idle_source_new (G_PRIORITY_LOW + 10, + _nml_cleanup_context_busy_watcher_on_idle_cb, + nm_utils_user_data_pack (g_main_context_ref (context), + g_steal_pointer (&context_busy_watcher)), + NULL); + g_source_attach (cleanup_source, context); + g_source_unref (cleanup_source); +} + +/*****************************************************************************/ + static void _init_start_complete (NMClient *self, GError *error_take) @@ -7568,18 +7630,6 @@ constructed (GObject *object) NML_NMCLIENT_LOG_D (self, "new NMClient instance"); } -static inline gboolean -_dispose_cleanup_context_busy_watcher_cb (gpointer user_data) -{ - nm_auto_unref_gmaincontext GMainContext *context = NULL; - gs_unref_object GObject *context_busy_watcher = NULL; - - nm_utils_user_data_unpack (user_data, &context, &context_busy_watcher); - - nm_assert (G_IS_OBJECT (context_busy_watcher)); - return G_SOURCE_REMOVE; -} - static void dispose (GObject *object) { @@ -7630,42 +7680,8 @@ dispose (GObject *object) if ( priv->context_busy_watcher && priv->dbus_context) { - GSource *cleanup_source; - - /* Technically, we cancelled all pending actions (and these actions - * (GTask) keep the context_busy_watcher object alive). Also, we passed - * no destroy notify to g_dbus_connection_signal_subscribe(). - * That means, there should be no other unaccounted GSource'es left. - * - * However, we really need to be sure that the context_busy_watcher's - * lifetime matches the time that the context is busy. That is especially - * important with synchronous initialization, where the context-busy-watcher - * keeps the inner GMainContext integrated in the caller's. - * We must not g_source_destroy() that integration too early. - * - * So to be really sure all this is given, always schedule one last - * cleanup idle action with low priority. This should be the last - * thing related to this instance that keeps the context busy. - * - * Note that we could also *not* take a reference on priv->dbus_context - * and unref priv->context_busy_watcher via the GDestroyNotify. That would - * allow for the context to be wrapped up early, and when the last user - * gives up the reference to the context, the destroy notify could complete - * without even invoke the idle handler. However, that destroy notify may - * not be called in the right thread. So, we want to be sure that we unref - * the context-busy-watcher in the right context. Hence, we always take an - * additional reference and always cleanup in the idle handler. This means: - * the user *MUST* always keep iterating the context after NMClient got destroyed. - * But that is not a severe limitation, because the user anyway must be prepared - * to do that. That is because in many cases it is necessary anyway (and the user - * wouldn't know a priory when not). This way, it is just always necessary. */ - cleanup_source = nm_g_idle_source_new (G_PRIORITY_LOW + 10, - _dispose_cleanup_context_busy_watcher_cb, - nm_utils_user_data_pack (g_main_context_ref (priv->dbus_context), - g_steal_pointer (&priv->context_busy_watcher)), - NULL); - g_source_attach (cleanup_source, priv->dbus_context); - g_source_unref (cleanup_source); + nml_cleanup_context_busy_watcher_on_idle (g_steal_pointer (&priv->context_busy_watcher), + priv->dbus_context); } nm_clear_pointer (&priv->dbus_context, g_main_context_unref); diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index c28e8a622b..f2affc5f62 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -209,6 +209,9 @@ NMLInitData *nml_init_data_new_async (GCancellable *cancellable, void nml_init_data_return (NMLInitData *init_data, GError *error_take); +void nml_cleanup_context_busy_watcher_on_idle (GObject *context_busy_watcher_take, + GMainContext *context); + /*****************************************************************************/ typedef struct _NMLDBusObject NMLDBusObject; From 2c30c1a04f823d00f6826be06de6fe9409d50ccc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 Dec 2019 13:19:49 +0100 Subject: [PATCH 21/22] libnm/secret-agent: add dbus-connection and main-context properties to NMSecretAgentOld The NMSecretAgentOld is build very much around a GDBusConnection, and GDBusConnection is build around GMainContext. That means, a NMSecretAgentOld instance is strongly related to these two. That is because NMSecretAgentOld will register to signals on D-Bus, using GDBusConnection. Hence, that GDBusConnection instance and the calling GMainContext becomes central to the NMSecretAgentOld instance. Also, the GMainContext is the way to synchronize access to the NMSecretAgentOld. Used properly, this allows using the API in multi threaded context. Expose these two in the public API. Since NMSecretAgentOld is part of libnm and supposed to provide a flexible API, this is just useful to have. Also, allow to provide a GDBusConnection as construct-only property. This way, the instance can be used independent of g_bus_get() and the user has full control. There is no setter for the GMainContext, because it just takes the g_main_context_get_thread_default() instance at the time of construction. --- libnm/libnm.ver | 2 + libnm/nm-secret-agent-old.c | 173 +++++++++++++++++++++++++++--------- libnm/nm-secret-agent-old.h | 7 ++ 3 files changed, 139 insertions(+), 43 deletions(-) diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 3ff469b0c5..292dbcf826 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1667,6 +1667,8 @@ global: nm_device_vrf_get_table; nm_device_vrf_get_type; nm_object_get_client; + nm_secret_agent_old_get_dbus_connection; + nm_secret_agent_old_get_main_context; nm_setting_vrf_get_table; nm_setting_vrf_get_type; nm_setting_vrf_new; diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 03aa87140f..11cca56ff9 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -35,10 +35,12 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMSecretAgentOld, PROP_AUTO_REGISTER, PROP_REGISTERED, PROP_CAPABILITIES, + PROP_DBUS_CONNECTION, ); typedef struct { - GDBusConnection *bus; + GDBusConnection *dbus_connection; + GMainContext *main_context; NMDBusAgentManager *manager_proxy; NMDBusSecretAgent *dbus_secret_agent; @@ -84,6 +86,44 @@ static void _register_call_cb (GObject *proxy, /*****************************************************************************/ +/** + * nm_secret_agent_old_get_dbus_connection: + * @self: the #NMSecretAgentOld instance + * + * Returns: (transfer none): the #GDBusConnection used by the secret agent. + * You may either set this as construct property %NM_SECRET_AGENT_OLD_DBUS_CONNECTION, + * or it will automatically set during initialization. + * + * Since: 1.24 + */ +GDBusConnection * +nm_secret_agent_old_get_dbus_connection (NMSecretAgentOld *self) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), NULL); + + return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->dbus_connection; +} + +/** + * nm_secret_agent_old_get_main_context: + * @self: the #NMSecretAgentOld instance + * + * Returns: (transfer none): the #GMainContext instance associate with the + * instance. This is the g_main_context_get_thread_default() at the time + * when creating the instance. + * + * Since: 1.24 + */ +GMainContext * +nm_secret_agent_old_get_main_context (NMSecretAgentOld *self) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), NULL); + + return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->main_context; +} + +/*****************************************************************************/ + static void _internal_unregister (NMSecretAgentOld *self) { @@ -198,7 +238,7 @@ verify_sender (NMSecretAgentOld *self, return TRUE; /* Check the UID of the sender */ - ret = g_dbus_connection_call_sync (priv->bus, + ret = g_dbus_connection_call_sync (priv->dbus_connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, @@ -524,7 +564,7 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, g_return_val_if_fail (priv->registered == FALSE, FALSE); g_return_val_if_fail (priv->registering_timeout_msec == 0, FALSE); - g_return_val_if_fail (priv->bus != NULL, FALSE); + g_return_val_if_fail (priv->dbus_connection != NULL, FALSE); g_return_val_if_fail (priv->manager_proxy != NULL, FALSE); /* Also make sure the subclass can actually respond to secrets requests */ @@ -540,7 +580,7 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, /* Export our secret agent interface before registering with the manager */ if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent), - priv->bus, + priv->dbus_connection, NM_DBUS_PATH_SECRET_AGENT, error)) return FALSE; @@ -749,7 +789,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, g_return_if_fail (priv->registered == FALSE); g_return_if_fail (priv->registering_timeout_msec == 0); - g_return_if_fail (priv->bus != NULL); + g_return_if_fail (priv->dbus_connection != NULL); g_return_if_fail (priv->manager_proxy != NULL); /* Also make sure the subclass can actually respond to secrets requests */ @@ -768,7 +808,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, /* Export our secret agent interface before registering with the manager */ if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent), - priv->bus, + priv->dbus_connection, NM_DBUS_PATH_SECRET_AGENT, &error)) { _LOGT ("register: failed to export D-Bus service: %s", error->message); @@ -834,7 +874,7 @@ nm_secret_agent_old_unregister (NMSecretAgentOld *self, priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_val_if_fail (priv->bus != NULL, FALSE); + g_return_val_if_fail (priv->dbus_connection != NULL, FALSE); g_return_val_if_fail (priv->manager_proxy != NULL, FALSE); priv->suppress_auto = TRUE; @@ -892,7 +932,7 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_if_fail (priv->bus != NULL); + g_return_if_fail (priv->dbus_connection != NULL); g_return_if_fail (priv->manager_proxy != NULL); task = nm_g_task_new (self, cancellable, nm_secret_agent_old_unregister_async, callback, user_data); @@ -1128,6 +1168,22 @@ init_async_got_proxy (GObject *object, GAsyncResult *result, gpointer user_data) g_steal_pointer (&task); } +static void +init_async_start (NMSecretAgentOld *self, GTask *task_take) +{ + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + nmdbus_agent_manager_proxy_new (priv->dbus_connection, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES + | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, + NM_DBUS_SERVICE, + NM_DBUS_PATH_AGENT_MANAGER, + g_task_get_cancellable (task_take), + init_async_got_proxy, + task_take); + g_steal_pointer (&task_take); +} + static void init_async_got_bus (GObject *initable, GAsyncResult *result, gpointer user_data) { @@ -1136,21 +1192,40 @@ init_async_got_bus (GObject *initable, GAsyncResult *result, gpointer user_data) NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); GError *error = NULL; - priv->bus = g_bus_get_finish (result, &error); - if (!priv->bus) { + priv->dbus_connection = g_bus_get_finish (result, &error); + if (!priv->dbus_connection) { g_task_return_error (task, error); return; } - nmdbus_agent_manager_proxy_new (priv->bus, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES - | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, - NM_DBUS_SERVICE, - NM_DBUS_PATH_AGENT_MANAGER, - g_task_get_cancellable (task), - init_async_got_proxy, - task); - g_steal_pointer (&task); + init_async_start (self, g_steal_pointer (&task)); + + _notify (self, PROP_DBUS_CONNECTION); +} + +static void +init_async (GAsyncInitable *initable, int io_priority, + GCancellable *cancellable, GAsyncReadyCallback callback, + gpointer user_data) +{ + NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (initable); + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + GTask *task; + + _LOGT ("init-async starting..."); + + task = g_task_new (self, cancellable, callback, user_data); + g_task_set_priority (task, io_priority); + + if (!priv->dbus_connection) { + g_bus_get (_nm_dbus_bus_type (), + cancellable, + init_async_got_bus, + task); + return; + } + + init_async_start (self, task); } /*****************************************************************************/ @@ -1164,6 +1239,9 @@ get_property (GObject *object, NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (object); switch (prop_id) { + case PROP_DBUS_CONNECTION: + g_value_set_object (value, priv->dbus_connection); + break; case PROP_IDENTIFIER: g_value_set_string (value, priv->identifier); break; @@ -1192,6 +1270,10 @@ set_property (GObject *object, const char *identifier; switch (prop_id) { + case PROP_DBUS_CONNECTION: + /* construct-only */ + priv->dbus_connection = g_value_dup_object (value); + break; case PROP_IDENTIFIER: identifier = g_value_get_string (value); @@ -1222,6 +1304,9 @@ nm_secret_agent_old_init (NMSecretAgentOld *self) _LOGT ("create new instance"); c_list_init (&priv->gsi_lst_head); + + priv->main_context = g_main_context_ref_thread_default (); + priv->dbus_secret_agent = nmdbus_secret_agent_skeleton_new (); _nm_dbus_bind_properties (self, priv->dbus_secret_agent); _nm_dbus_bind_methods (self, priv->dbus_secret_agent, @@ -1240,11 +1325,14 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) _LOGT ("init-sync"); - priv->bus = g_bus_get_sync (_nm_dbus_bus_type (), cancellable, error); - if (!priv->bus) - return FALSE; + if (!priv->dbus_connection) { + priv->dbus_connection = g_bus_get_sync (_nm_dbus_bus_type (), cancellable, error); + if (!priv->dbus_connection) + return FALSE; + _notify (self, PROP_DBUS_CONNECTION); + } - priv->manager_proxy = nmdbus_agent_manager_proxy_new_sync (priv->bus, + priv->manager_proxy = nmdbus_agent_manager_proxy_new_sync (priv->dbus_connection, G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, NM_DBUS_SERVICE, @@ -1262,25 +1350,6 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error) return TRUE; } -static void -init_async (GAsyncInitable *initable, int io_priority, - GCancellable *cancellable, GAsyncReadyCallback callback, - gpointer user_data) -{ - NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (initable); - GTask *task; - - _LOGT ("init-async starting..."); - - task = g_task_new (self, cancellable, callback, user_data); - g_task_set_priority (task, io_priority); - - g_bus_get (_nm_dbus_bus_type (), - cancellable, - init_async_got_bus, - task); -} - static void dispose (GObject *object) { @@ -1307,9 +1376,11 @@ dispose (GObject *object) } g_clear_object (&priv->manager_proxy); - g_clear_object (&priv->bus); G_OBJECT_CLASS (nm_secret_agent_old_parent_class)->dispose (object); + + g_clear_object (&priv->dbus_connection); + nm_clear_pointer (&priv->main_context, g_main_context_unref); } static void @@ -1323,6 +1394,22 @@ nm_secret_agent_old_class_init (NMSecretAgentOldClass *class) object_class->get_property = get_property; object_class->set_property = set_property; + /** + * NMSecretAgentOld:dbus-connection: + * + * The #GDBusConnection used by the instance. You may either set this + * as construct-only property, or otherwise #NMSecretAgentOld will choose + * a connection via g_bus_get() during initialization. + * + * Since: 1.24 + **/ + obj_properties[PROP_DBUS_CONNECTION] = + g_param_spec_object (NM_SECRET_AGENT_OLD_DBUS_CONNECTION, "", "", + G_TYPE_DBUS_CONNECTION, + G_PARAM_READWRITE | + G_PARAM_CONSTRUCT_ONLY | + G_PARAM_STATIC_STRINGS); + /** * NMSecretAgentOld:identifier: * diff --git a/libnm/nm-secret-agent-old.h b/libnm/nm-secret-agent-old.h index af0dabe729..2f71fc1987 100644 --- a/libnm/nm-secret-agent-old.h +++ b/libnm/nm-secret-agent-old.h @@ -21,6 +21,7 @@ G_BEGIN_DECLS #define NM_SECRET_AGENT_OLD_AUTO_REGISTER "auto-register" #define NM_SECRET_AGENT_OLD_REGISTERED "registered" #define NM_SECRET_AGENT_OLD_CAPABILITIES "capabilities" +#define NM_SECRET_AGENT_OLD_DBUS_CONNECTION "dbus-connection" /** * NMSecretAgentOld: @@ -179,6 +180,12 @@ typedef struct { GType nm_secret_agent_old_get_type (void); +NM_AVAILABLE_IN_1_24 +GDBusConnection *nm_secret_agent_old_get_dbus_connection (NMSecretAgentOld *self); + +NM_AVAILABLE_IN_1_24 +GMainContext *nm_secret_agent_old_get_main_context (NMSecretAgentOld *self); + gboolean nm_secret_agent_old_register (NMSecretAgentOld *self, GCancellable *cancellable, GError **error); From c1ec82909988add353acad4917a8545a9d3d7488 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 Dec 2019 13:26:50 +0100 Subject: [PATCH 22/22] libnm/secret-agent: rework NMSecretAgentOld Note that the name "NMSecretAgentOld" comes from when libnm was forked from libnm-glib. There was a plan to rework the secret agent API and replace it by a better one. That didn't happen (yet), instead our one and only agent implementation is still lacking. Don't add a new API, instead try to improve the existing one, without breaking existing users. Just get over the fact that the name "NMSecretAgentOld" is ugly. Also note how nm-applet uses NMSecretAgentOld. It subtypes a class AppletAgent. The constructor applet_agent_new() is calling the synchronous g_initable_init() initialization with auto-register enabled. As it was, g_initable_init() would call nm_secret_agent_old_register(), and if the "Register" call failed, initialization failed for good. There are even unit tests that test this behavior. This is bad behavior. It means, when you start nm-applet without NetworkManager running, it will fail to create the AppletAgent instance. It would hence be the responsibility of the applet to recover from this situation (e.g. by retrying after timeout or watching the D-Bus name owner). Of course, nm-applet doesn't do that and won't recover from such a failure. NMSecretAgentOld must try hard not to fail and recover automatically. The user of the API is not interested in implementing the registration, unregistration and retry handling. Instead, it should just work best effort and transparently to the user of the API. Differences: - no longer use gdbus-codegen generate bindings. Use GDBusConnection directly instead. These generated proxies complicate the code by introducing an additional, stateful layer. - properly handle GMainContext and synchronous initialization by using an internal GMainContext. With this NMSecretAgentOld can be used in a multi threaded context with separate GMainContext. This does not mean that the object itself became thread safe, but that the GMainContext gives the means to coordinate multi-threaded access. - there are no more blocking calls except g_initiable_init() which iterates an internal GMainContext until initialization completes. - obtaining the Unix user ID with "GetConnectionUnixUser" to authenticate the server is now done asynchronously and only once per name-owner. - NMSecretAgentOld will now register/export the Agent D-Bus object already during initialization and stay registered as long as the instance is alive. This is because usually registering a D-Bus object would not fail, unless the D-Bus path is already taken. Such an error would mean that another agent is registered for the same GDBusConnection, that likely would be a bug in the caller. Hence, such an issue is truly non-recoverable and should be reported early to the user. There is a change in behavior compared to before, where previously the D-Bus object would only be registered while the instance is enabled. This makes a difference if the user intended to keep the NMSecretAgentOld instance around in an unregistered state. Note that nm_secret_agent_old_destroy() was added to really unregister the D-Bus object. A destroyed instance can no longer be registered. - the API no longer fully exposes the current registration state. The user either enables or disables the agent. Then, in the background NMSecretAgentOld will register, and serve requests as they come. It will also always automatically re-register and it can de-facto no longer fail. That is, there might be a failure to register, or the NetworkManager peer might not be authenticated (non-root) or there might be some other error, or NetworkManager might not be running. But such errors are not exposed to the user. The instance is just not able to provide the secrets in those cases, but it may recover if the problem can be resolved. - In particular, it makes no sense that nm_secret_agent_old_register*() fails, returns an error, or waits until registration is complete. This API is now only to enable/disable the agent. It is idempotent and won't fail (there is a catch, see next point). In particular, nm_secret_agent_old_unregister*() cannot fail anymore. - However, with the previous point there is a problem/race. When you create a NMSecretAgentOld instance and immediately afterwards activate a profile, then you want to be sure that the registration is complete first. Otherwise, NetworkManager might fail the activation because no secret agent registered yet. A partial solution for this is that g_initiable_init()/g_async_initable_init_async() will block until registration is complete (or with or without success). That means, if NetworkManager is running, initializing the NMSecretAgentOld will wait until registration is complete (or failed). However, that does not solve the race if NetworkManager was not running when creating the instance. To solve that race, the user may call nm_secret_agent_old_register_async() and wait for the command to finish before starting activating. While async registration no longer fails (in the sense of leaving the agent permanently disconnected), it will try to ensure that we are successfully registered and ready to serve requests. By using this API correctly, a race can be avoided and the user can know that the instance is now ready to serve request. --- libnm/libnm.ver | 4 + libnm/nm-secret-agent-old.c | 1885 ++++++++++++++++++++----------- libnm/nm-secret-agent-old.h | 53 +- libnm/tests/test-secret-agent.c | 195 +++- po/POTFILES.in | 1 + 5 files changed, 1393 insertions(+), 745 deletions(-) diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 292dbcf826..457b55a798 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1667,7 +1667,11 @@ global: nm_device_vrf_get_table; nm_device_vrf_get_type; nm_object_get_client; + nm_secret_agent_old_destroy; + nm_secret_agent_old_enable; + nm_secret_agent_old_get_context_busy_watcher; nm_secret_agent_old_get_dbus_connection; + nm_secret_agent_old_get_dbus_name_owner; nm_secret_agent_old_get_main_context; nm_setting_vrf_get_table; nm_setting_vrf_get_type; diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 11cca56ff9..eb653327d0 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -12,22 +12,22 @@ #include "nm-dbus-helpers.h" #include "nm-dbus-interface.h" #include "nm-enum-types.h" +#include "nm-glib-aux/nm-c-list.h" #include "nm-glib-aux/nm-dbus-aux.h" #include "nm-glib-aux/nm-time-utils.h" #include "nm-simple-connection.h" -#include "introspection/org.freedesktop.NetworkManager.SecretAgent.h" -#include "introspection/org.freedesktop.NetworkManager.AgentManager.h" - -#define REGISTER_RETRY_TIMEOUT_MSEC 2000 +#define REGISTER_RETRY_TIMEOUT_MSEC 3000 +#define _CALL_REGISTER_TIMEOUT_MSEC 15000 /*****************************************************************************/ typedef struct { - char *path; + char *connection_path; char *setting_name; GDBusMethodInvocation *context; CList gsi_lst; + bool is_cancelling:1; } GetSecretsInfo; NM_GOBJECT_PROPERTIES_DEFINE (NMSecretAgentOld, @@ -41,23 +41,55 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMSecretAgentOld, typedef struct { GDBusConnection *dbus_connection; GMainContext *main_context; - NMDBusAgentManager *manager_proxy; - NMDBusSecretAgent *dbus_secret_agent; + GMainContext *dbus_context; + GObject *context_busy_watcher; + GCancellable *name_owner_cancellable; + GCancellable *registering_cancellable; + GSource *registering_retry_source; + + NMLInitData *init_data; - /* GetSecretsInfo structs of in-flight GetSecrets requests */ CList gsi_lst_head; + CList pending_tasks_register_lst_head; + char *identifier; - NMSecretAgentCapabilities capabilities; + NMRefString *name_owner_curr; + NMRefString *name_owner_next; gint64 registering_timeout_msec; - guint registering_try_count; - bool registered:1; + guint name_owner_changed_id; + + guint exported_id; + + guint capabilities; + + guint8 registering_try_count; + + guint8 register_state_change_reenter:2; + bool session_bus:1; + bool auto_register:1; - bool suppress_auto:1; + + bool is_registered:1; + + bool is_enabled:1; + + bool registration_force_unregister:1; + + /* This is true, if we either are in the process of RegisterWithCapabilities() or + * are already successfully registered. + * + * This is only TRUE, if the name owner was authenticated to run as root user. + * + * It also means, we should follow up with an Unregister() call during shutdown. */ + bool registered_against_server:1; + + bool is_initialized:1; + bool is_destroyed:1; } NMSecretAgentOldPrivate; static void nm_secret_agent_old_initable_iface_init (GInitableIface *iface); @@ -68,7 +100,7 @@ G_DEFINE_ABSTRACT_TYPE_WITH_CODE (NMSecretAgentOld, nm_secret_agent_old, G_TYPE_ G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, nm_secret_agent_old_async_initable_iface_init); ) -#define NM_SECRET_AGENT_OLD_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_SECRET_AGENT_OLD, NMSecretAgentOldPrivate)) +#define NM_SECRET_AGENT_OLD_GET_PRIVATE(self) (G_TYPE_INSTANCE_GET_PRIVATE ((self), NM_TYPE_SECRET_AGENT_OLD, NMSecretAgentOldPrivate)) /*****************************************************************************/ @@ -80,9 +112,55 @@ G_DEFINE_ABSTRACT_TYPE_WITH_CODE (NMSecretAgentOld, nm_secret_agent_old, G_TYPE_ /*****************************************************************************/ -static void _register_call_cb (GObject *proxy, - GAsyncResult *result, - gpointer user_data); +static const GDBusInterfaceInfo interface_info = NM_DEFINE_GDBUS_INTERFACE_INFO_INIT ( + NM_DBUS_INTERFACE_SECRET_AGENT, + .methods = NM_DEFINE_GDBUS_METHOD_INFOS ( + NM_DEFINE_GDBUS_METHOD_INFO ( + "GetSecrets", + .in_args = NM_DEFINE_GDBUS_ARG_INFOS ( + NM_DEFINE_GDBUS_ARG_INFO ("connection", "a{sa{sv}}"), + NM_DEFINE_GDBUS_ARG_INFO ("connection_path", "o"), + NM_DEFINE_GDBUS_ARG_INFO ("setting_name", "s"), + NM_DEFINE_GDBUS_ARG_INFO ("hints", "as"), + NM_DEFINE_GDBUS_ARG_INFO ("flags", "u"), + ), + .out_args = NM_DEFINE_GDBUS_ARG_INFOS ( + NM_DEFINE_GDBUS_ARG_INFO ("secrets", "a{sa{sv}}"), + ), + ), + NM_DEFINE_GDBUS_METHOD_INFO ( + "CancelGetSecrets", + .in_args = NM_DEFINE_GDBUS_ARG_INFOS ( + NM_DEFINE_GDBUS_ARG_INFO ("connection_path", "o"), + NM_DEFINE_GDBUS_ARG_INFO ("setting_name", "s"), + ), + ), + NM_DEFINE_GDBUS_METHOD_INFO ( + "SaveSecrets", + .in_args = NM_DEFINE_GDBUS_ARG_INFOS ( + NM_DEFINE_GDBUS_ARG_INFO ("connection", "a{sa{sv}}"), + NM_DEFINE_GDBUS_ARG_INFO ("connection_path", "o"), + ), + ), + NM_DEFINE_GDBUS_METHOD_INFO ( + "DeleteSecrets", + .in_args = NM_DEFINE_GDBUS_ARG_INFOS ( + NM_DEFINE_GDBUS_ARG_INFO ("connection", "a{sa{sv}}"), + NM_DEFINE_GDBUS_ARG_INFO ("connection_path", "o"), + ), + ), + ), +); + +/*****************************************************************************/ + +static void _register_state_change (NMSecretAgentOld *self); + +static void _register_dbus_call (NMSecretAgentOld *self); + +static void _init_complete (NMSecretAgentOld *self, GError *error_take); + +static void _register_state_complete (NMSecretAgentOld *self); /*****************************************************************************/ @@ -122,156 +200,201 @@ nm_secret_agent_old_get_main_context (NMSecretAgentOld *self) return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->main_context; } +/** + * nm_secret_agent_old_get_context_busy_watcher: + * @self: the #NMSecretAgentOld instance + * + * Returns a #GObject that stays alive as long as there are pending + * requests in the #GDBusConnection. Such requests keep the #GMainContext + * alive, and thus you may want to keep iterating the context as long + * until a weak reference indicates that this object is gone. This is + * useful because even when you destroy the instance right away (and all + * the internally pending requests get cancelled), any pending g_dbus_connection_call() + * requests will still invoke the result on the #GMainContext. Hence, this + * allows you to know how long you must iterate the context to know + * that all remains are cleaned up. + * + * Returns: (transfer none): a #GObject that you may register a weak pointer + * to know that the #GMainContext is still kept busy by @self. + * + * Since: 1.24 + */ +GObject * +nm_secret_agent_old_get_context_busy_watcher (NMSecretAgentOld *self) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), NULL); + + return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->context_busy_watcher; +} + +/** + * nm_secret_agent_old_get_dbus_name_owner: + * @self: the #NMSecretAgentOld instance + * + * Returns: the current D-Bus name owner. While this property + * is set while registering, it really only makes sense when + * the nm_secret_agent_old_get_registered() indicates that + * registration is successfull. + * + * Since: 1.24 + */ +const char * +nm_secret_agent_old_get_dbus_name_owner (NMSecretAgentOld *self) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), NULL); + + return nm_ref_string_get_str (NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->name_owner_curr); +} + +/** + * nm_secret_agent_old_get_registered: + * @self: a #NMSecretAgentOld + * + * Note that the secret agent transparently registers and re-registers + * as the D-Bus name owner appears. Hence, this property is not really + * useful. Also, to be graceful against races during registration, the + * instance will already accept requests while being in the process of + * registering. + * If you need to avoid races and want to wait until @self is registered, + * call nm_secret_agent_old_register_async(). If that function completes + * with success, you know the instance is registered. + * + * Returns: a %TRUE if the agent is registered, %FALSE if it is not. + **/ +gboolean +nm_secret_agent_old_get_registered (NMSecretAgentOld *self) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); + + return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->is_registered; +} + /*****************************************************************************/ static void -_internal_unregister (NMSecretAgentOld *self) -{ - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - - if (priv->registered) { - g_dbus_interface_skeleton_unexport (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent)); - priv->registered = FALSE; - priv->registering_timeout_msec = 0; - _notify (self, PROP_REGISTERED); - } -} - -static void -get_secrets_info_free (GetSecretsInfo *info) +get_secret_info_free (GetSecretsInfo *info) { nm_assert (info); + nm_assert (!info->context); c_list_unlink_stale (&info->gsi_lst); - - g_free (info->path); + g_free (info->connection_path); g_free (info->setting_name); - g_slice_free (GetSecretsInfo, info); -} - -static gboolean -should_auto_register (NMSecretAgentOld *self) -{ - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - - return ( priv->auto_register - && !priv->suppress_auto - && !priv->registered - && priv->registering_timeout_msec == 0); + nm_g_slice_free (info); } static void -name_owner_changed (GObject *proxy, - GParamSpec *pspec, - gpointer user_data) +get_secret_info_complete_and_free (GetSecretsInfo *info, + GVariant *secrets, + GError *error) { - NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (user_data); - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - gs_free char *owner = NULL; - GetSecretsInfo *info; - - owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (proxy)); - - _LOGT ("name owner changed: %s%s%s", NM_PRINT_FMT_QUOTE_STRING (owner)); - - if (owner) { - if (should_auto_register (self)) - nm_secret_agent_old_register_async (self, NULL, NULL, NULL); + if (error) { + if (secrets) + nm_g_variant_unref_floating (secrets); + g_dbus_method_invocation_return_gerror (g_steal_pointer (&info->context), error); } else { - while ((info = c_list_first_entry (&priv->gsi_lst_head, GetSecretsInfo, gsi_lst))) { - c_list_unlink (&info->gsi_lst); - NM_SECRET_AGENT_OLD_GET_CLASS (self)->cancel_get_secrets (self, - info->path, - info->setting_name); - } - - _internal_unregister (self); + g_dbus_method_invocation_return_value (g_steal_pointer (&info->context), + g_variant_new ("(@a{sa{sv}})", secrets)); } + get_secret_info_free (info); } -static gboolean -verify_sender (NMSecretAgentOld *self, - GDBusMethodInvocation *context, - GError **error) +static void +get_secret_info_complete_and_free_error (GetSecretsInfo *info, + GQuark error_domain, + int error_code, + const char *error_message) +{ + g_dbus_method_invocation_return_error_literal (g_steal_pointer (&info->context), error_domain, error_code, error_message); + get_secret_info_free (info); +} + +/*****************************************************************************/ + +static void +_dbus_connection_call_cb (GObject *source, + GAsyncResult *result, + gpointer user_data) +{ + gs_unref_object GObject *context_busy_watcher = NULL; + GAsyncReadyCallback callback; + gpointer callback_user_data; + + nm_utils_user_data_unpack (user_data, &context_busy_watcher, &callback, &callback_user_data); + callback (source, result, callback_user_data); +} + +static void +_dbus_connection_call (NMSecretAgentOld *self, + const char *bus_name, + const char *object_path, + const char *interface_name, + const char *method_name, + GVariant *parameters, + const GVariantType *reply_type, + GDBusCallFlags flags, + int timeout_msec, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - gs_free char *owner = NULL; - const char *sender; - guint32 sender_uid; - gs_unref_variant GVariant *ret = NULL; - gs_free_error GError *local = NULL; - g_return_val_if_fail (context != NULL, FALSE); + nm_assert (nm_g_main_context_is_thread_default (priv->dbus_context)); - /* Verify that the sender is the same as NetworkManager's bus name owner. */ + g_dbus_connection_call (priv->dbus_connection, + bus_name, + object_path, + interface_name, + method_name, + parameters, + reply_type, + flags, + timeout_msec, + cancellable, + callback + ? _dbus_connection_call_cb + : NULL, + callback + ? nm_utils_user_data_pack (g_object_ref (priv->context_busy_watcher), callback, user_data) + : NULL); +} - owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (priv->manager_proxy)); - if (!owner) { - g_set_error_literal (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, - "NetworkManager bus name owner unknown."); - return FALSE; +/*****************************************************************************/ + +static GetSecretsInfo * +find_get_secrets_info (NMSecretAgentOldPrivate *priv, + const char *connection_path, + const char *setting_name) +{ + GetSecretsInfo *info; + + c_list_for_each_entry (info, &priv->gsi_lst_head, gsi_lst) { + if ( nm_streq (connection_path, info->connection_path) + && nm_streq (setting_name, info->setting_name)) + return info; } + return NULL; +} - sender = g_dbus_method_invocation_get_sender (context); - if (!sender) { - g_set_error_literal (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, - "Failed to get request sender."); - return FALSE; - } +static void +_cancel_get_secret_request (NMSecretAgentOld *self, + GetSecretsInfo *info, + const char *message) +{ + c_list_unlink (&info->gsi_lst); + info->is_cancelling = TRUE; - if (!nm_streq (sender, owner)) { - g_set_error_literal (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, - "Request sender does not match NetworkManager bus name owner."); - return FALSE; - } + _LOGT ("cancel get-secrets request \"%s\", \"%s\": %s", info->connection_path, info->setting_name, message); - /* If we're connected to the session bus, then this must be a test program, - * so skip the UID check. - */ - if (priv->session_bus) - return TRUE; + NM_SECRET_AGENT_OLD_GET_CLASS (self)->cancel_get_secrets (self, + info->connection_path, + info->setting_name); - /* Check the UID of the sender */ - ret = g_dbus_connection_call_sync (priv->dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - "GetConnectionUnixUser", - g_variant_new ("(s)", sender), - G_VARIANT_TYPE ("(u)"), - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, &local); - if (!ret) { - gs_free char *remote_error = NULL; - - remote_error = g_dbus_error_get_remote_error (local); - g_dbus_error_strip_remote_error (local); - g_set_error (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, - "Failed to request unix user: (%s) %s.", - remote_error ?: "", - local->message); - return FALSE; - } - g_variant_get (ret, "(u)", &sender_uid); - - /* We only accept requests from NM, which always runs as root */ - if (sender_uid != 0) { - g_set_error_literal (error, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, - "Request sender is not root."); - return FALSE; - } - - return TRUE; + get_secret_info_complete_and_free_error (info, + NM_SECRET_AGENT_ERROR, + NM_SECRET_AGENT_ERROR_AGENT_CANCELED, + message); } static gboolean @@ -285,15 +408,7 @@ verify_request (NMSecretAgentOld *self, gs_unref_object NMConnection *connection = NULL; gs_free_error GError *local = NULL; - if (!verify_sender (self, context, error)) - return FALSE; - - /* No connection? If the sender verified, then we allow the request */ - if (connection_dict == NULL) - return TRUE; - - /* If we have a connection dictionary, we require a path too */ - if (connection_path == NULL) { + if (!nm_dbus_path_not_empty (connection_path)) { g_set_error_literal (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_INVALID_CONNECTION, @@ -301,7 +416,6 @@ verify_request (NMSecretAgentOld *self, return FALSE; } - /* Make sure the given connection is valid */ connection = _nm_simple_connection_new_from_dbus (connection_dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &local); if (!connection) { g_set_error (error, @@ -325,102 +439,99 @@ get_secrets_cb (NMSecretAgentOld *self, { GetSecretsInfo *info = user_data; - if (error) - g_dbus_method_invocation_return_gerror (info->context, error); - else { - g_variant_take_ref (secrets); - g_dbus_method_invocation_return_value (info->context, - g_variant_new ("(@a{sa{sv}})", secrets)); + if (info->is_cancelling) { + if (secrets) + nm_g_variant_unref_floating (secrets); + return; } - get_secrets_info_free (info); + _LOGT ("request: get-secrets request \"%s\", \"%s\" complete with %s%s%s", + info->connection_path, + info->setting_name, + NM_PRINT_FMT_QUOTED (error, "error: ", error->message, "", "success")); + + get_secret_info_complete_and_free (info, secrets, error); } static void -impl_secret_agent_old_get_secrets (NMSecretAgentOld *self, - GDBusMethodInvocation *context, - GVariant *connection_dict, - const char *connection_path, - const char *setting_name, - const char * const *hints, - guint flags, - gpointer user_data) +impl_get_secrets (NMSecretAgentOld *self, + GVariant *parameters, + GDBusMethodInvocation *context) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); GError *error = NULL; gs_unref_object NMConnection *connection = NULL; GetSecretsInfo *info; + gs_unref_variant GVariant *arg_connection = NULL; + const char *arg_connection_path; + const char *arg_setting_name; + gs_free const char **arg_hints = NULL; + guint32 arg_flags; - /* Make sure the request comes from NetworkManager and is valid */ - if (!verify_request (self, context, connection_dict, connection_path, &connection, &error)) { + g_variant_get (parameters, + "(@a{sa{sv}}&o&s^a&su)", + &arg_connection, + &arg_connection_path, + &arg_setting_name, + &arg_hints, + &arg_flags); + + if (!verify_request (self, context, arg_connection, arg_connection_path, &connection, &error)) { g_dbus_method_invocation_take_error (context, error); return; } + _LOGT ("request: get-secrets(\"%s\", \"%s\")", arg_connection_path, arg_setting_name); + + info = find_get_secrets_info (priv, arg_connection_path, arg_setting_name); + if (info) + _cancel_get_secret_request (self, info, "Request aborted due to new request"); + info = g_slice_new (GetSecretsInfo); *info = (GetSecretsInfo) { - .path = g_strdup (connection_path), - .setting_name = g_strdup (setting_name), - .context = context, + .context = context, + .connection_path = g_strdup (arg_connection_path), + .setting_name = g_strdup (arg_setting_name), }; c_list_link_tail (&priv->gsi_lst_head, &info->gsi_lst); NM_SECRET_AGENT_OLD_GET_CLASS (self)->get_secrets (self, connection, - connection_path, - setting_name, - (const char **) hints, - flags, + info->connection_path, + info->setting_name, + arg_hints, + arg_flags, get_secrets_cb, info); } -static GetSecretsInfo * -find_get_secrets_info (NMSecretAgentOldPrivate *priv, - const char *path, - const char *setting_name) -{ - GetSecretsInfo *info; - - c_list_for_each_entry (info, &priv->gsi_lst_head, gsi_lst) { - if ( nm_streq0 (path, info->path) - && nm_streq0 (setting_name, info->setting_name)) - return info; - } - return NULL; -} - static void -impl_secret_agent_old_cancel_get_secrets (NMSecretAgentOld *self, - GDBusMethodInvocation *context, - const char *connection_path, - const char *setting_name, - gpointer user_data) +impl_cancel_get_secrets (NMSecretAgentOld *self, + GVariant *parameters, + GDBusMethodInvocation *context) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GError *error = NULL; GetSecretsInfo *info; + const char *arg_connection_path; + const char *arg_setting_name; - /* Make sure the request comes from NetworkManager and is valid */ - if (!verify_request (self, context, NULL, NULL, NULL, &error)) { - g_dbus_method_invocation_take_error (context, error); - return; - } + g_variant_get (parameters, + "(&o&s)", + &arg_connection_path, + &arg_setting_name); - info = find_get_secrets_info (priv, connection_path, setting_name); + info = find_get_secrets_info (priv, arg_connection_path, arg_setting_name); if (!info) { - g_dbus_method_invocation_return_error (context, - NM_SECRET_AGENT_ERROR, - NM_SECRET_AGENT_ERROR_FAILED, - "No secrets request in progress for this connection."); + g_dbus_method_invocation_return_error_literal (context, + NM_SECRET_AGENT_ERROR, + NM_SECRET_AGENT_ERROR_FAILED, + "No secrets request in progress for this connection."); return; } - c_list_unlink (&info->gsi_lst); - - NM_SECRET_AGENT_OLD_GET_CLASS (self)->cancel_get_secrets (self, - info->path, - info->setting_name); + _cancel_get_secret_request (self, + info, + "Request cancelled by NetworkManager"); g_dbus_method_invocation_return_value (context, NULL); } @@ -440,24 +551,30 @@ save_secrets_cb (NMSecretAgentOld *self, } static void -impl_secret_agent_old_save_secrets (NMSecretAgentOld *self, - GDBusMethodInvocation *context, - GVariant *connection_dict, - const char *connection_path, - gpointer user_data) +impl_save_secrets (NMSecretAgentOld *self, + GVariant *parameters, + GDBusMethodInvocation *context) { gs_unref_object NMConnection *connection = NULL; + gs_unref_variant GVariant *arg_connection = NULL; + const char *arg_connection_path; GError *error = NULL; - /* Make sure the request comes from NetworkManager and is valid */ - if (!verify_request (self, context, connection_dict, connection_path, &connection, &error)) { + g_variant_get (parameters, + "(@a{sa{sv}}&o)", + &arg_connection, + &arg_connection_path); + + if (!verify_request (self, context, arg_connection, arg_connection_path, &connection, &error)) { g_dbus_method_invocation_take_error (context, error); return; } + _LOGT ("request: save-secrets(\"%s\")", arg_connection_path); + NM_SECRET_AGENT_OLD_GET_CLASS (self)->save_secrets (self, connection, - connection_path, + arg_connection_path, save_secrets_cb, context); } @@ -477,64 +594,124 @@ delete_secrets_cb (NMSecretAgentOld *self, } static void -impl_secret_agent_old_delete_secrets (NMSecretAgentOld *self, - GDBusMethodInvocation *context, - GVariant *connection_dict, - const char *connection_path, - gpointer user_data) +impl_delete_secrets (NMSecretAgentOld *self, + GVariant *parameters, + GDBusMethodInvocation *context) { gs_unref_object NMConnection *connection = NULL; + gs_unref_variant GVariant *arg_connection = NULL; + const char *arg_connection_path; GError *error = NULL; - /* Make sure the request comes from NetworkManager and is valid */ - if (!verify_request (self, context, connection_dict, connection_path, &connection, &error)) { + g_variant_get (parameters, + "(@a{sa{sv}}&o)", + &arg_connection, + &arg_connection_path); + + if (!verify_request (self, context, arg_connection, arg_connection_path, &connection, &error)) { g_dbus_method_invocation_take_error (context, error); return; } + _LOGT ("request: delete-secrets(\"%s\")", arg_connection_path); + NM_SECRET_AGENT_OLD_GET_CLASS (self)->delete_secrets (self, connection, - connection_path, + arg_connection_path, delete_secrets_cb, context); } /*****************************************************************************/ -static gboolean -check_nm_running (NMSecretAgentOld *self, GError **error) +/** + * nm_secret_agent_old_enable: + * @self: the #NMSecretAgentOld instance + * @enable: whether to enable or disable the listener. + * + * This has the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER + * property. + * + * Unlike most other functions, you may already call this function before + * initialization completes. + * + * Since: 1.24 + */ +void +nm_secret_agent_old_enable (NMSecretAgentOld *self, + gboolean enable) +{ + NMSecretAgentOldPrivate *priv; + + g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); + + priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + enable = (!!enable); + + if (priv->auto_register != enable) { + priv->auto_register = enable; + priv->is_enabled = enable; + _notify (self, PROP_AUTO_REGISTER); + } + _register_state_change (self); +} + +static void +_secret_agent_old_destroy (NMSecretAgentOld *self) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - gs_free char *owner = NULL; - owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (priv->manager_proxy)); - if (owner) - return TRUE; + priv->is_destroyed = TRUE; - g_set_error (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_FAILED, - "NetworkManager is not running"); - return FALSE; + if (priv->exported_id != 0) { + g_dbus_connection_unregister_object (priv->dbus_connection, + nm_steal_int (&priv->exported_id)); + } + + _register_state_change (self); + + nm_assert (!priv->name_owner_changed_id); + nm_assert (!priv->name_owner_curr); + nm_assert (!priv->name_owner_next); + nm_assert (!priv->name_owner_cancellable); + nm_assert (!priv->registering_retry_source); + nm_assert (!priv->registering_cancellable); + nm_assert (!priv->init_data); + nm_assert (c_list_is_empty (&priv->gsi_lst_head)); + nm_assert (c_list_is_empty (&priv->pending_tasks_register_lst_head)); +} + +/** + * nm_secret_agent_old_destroy: + * @self: the #NMSecretAgentOld instance. + * + * Since 1.24, the instance will already register a D-Bus object on the + * D-Bus connection during initialization. That object will stay registered + * until @self gets unrefed (destroyed) or this function is called. This + * function performs the necessary cleanup to tear down the instance. Afterwards, + * the function can not longer be used. This is optional, but necessary to + * ensure unregistering the D-Bus object at a define point, when other users + * might still have a reference on @self. + * + * You may call this function any time and repeatedly. However, after destroying + * the instance, it is a bug to still use the instance for other purposes. The + * instance becomes defunct and cannot re-register. + * + * Since: 1.24 + */ +void +nm_secret_agent_old_destroy (NMSecretAgentOld *self) +{ + g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); + + _LOGT ("destroying"); + + _secret_agent_old_destroy (self); } /*****************************************************************************/ -static gboolean -_register_should_retry (NMSecretAgentOldPrivate *priv, - guint *out_timeout_msec) -{ - guint timeout_msec; - - if (priv->registering_try_count++ == 0) - timeout_msec = 0; - else if (nm_utils_get_monotonic_timestamp_msec () < priv->registering_timeout_msec) - timeout_msec = 1ULL * (1ULL << NM_MIN (7, priv->registering_try_count)); - else - return FALSE; - - *out_timeout_msec = timeout_msec; - return TRUE; -} - /** * nm_secret_agent_old_register: * @self: a #NMSecretAgentOld @@ -545,10 +722,17 @@ _register_should_retry (NMSecretAgentOldPrivate *priv, * indicating to NetworkManager that the agent is able to provide and save * secrets for connections on behalf of its user. * - * It is a programmer error to attempt to register an agent that is already - * registered, or in the process of registering. - * * Returns: %TRUE if registration was successful, %FALSE on error. + * + * Since 1.24, this can no longer fail unless the @cancellable gets + * cancelled. Contrary to nm_secret_agent_old_register_async(), this also + * does not wait for the registration to succeed. You cannot synchronously + * (without iterating the caller's GMainContext) wait for registration. + * + * Since 1.24, registration is idempotent. It has the same effect as setting + * %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %TRUE or nm_secret_agent_old_enable(). + * + * Deprecated: 1.24: use nm_secret_agent_old_enable() or nm_secret_agent_old_register_async(). **/ gboolean nm_secret_agent_old_register (NMSecretAgentOld *self, @@ -556,208 +740,61 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, GError **error) { NMSecretAgentOldPrivate *priv; - NMSecretAgentOldClass *class; g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); + g_return_val_if_fail (!error || !*error, FALSE); priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_val_if_fail (priv->registered == FALSE, FALSE); - g_return_val_if_fail (priv->registering_timeout_msec == 0, FALSE); - g_return_val_if_fail (priv->dbus_connection != NULL, FALSE); - g_return_val_if_fail (priv->manager_proxy != NULL, FALSE); + g_return_val_if_fail (priv->is_initialized && !priv->is_destroyed, FALSE); - /* Also make sure the subclass can actually respond to secrets requests */ - class = NM_SECRET_AGENT_OLD_GET_CLASS (self); - g_return_val_if_fail (class->get_secrets != NULL, FALSE); - g_return_val_if_fail (class->save_secrets != NULL, FALSE); - g_return_val_if_fail (class->delete_secrets != NULL, FALSE); + priv->is_enabled = TRUE; + _register_state_change (self); - if (!check_nm_running (self, error)) + if (g_cancellable_set_error_if_cancelled (cancellable, error)) return FALSE; - priv->suppress_auto = FALSE; - - /* Export our secret agent interface before registering with the manager */ - if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent), - priv->dbus_connection, - NM_DBUS_PATH_SECRET_AGENT, - error)) - return FALSE; - - priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_msec () + REGISTER_RETRY_TIMEOUT_MSEC; - priv->registering_try_count = 0; - - while (TRUE) { - gs_free_error GError *local = NULL; - - nmdbus_agent_manager_call_register_with_capabilities_sync (priv->manager_proxy, - priv->identifier, - priv->capabilities, - cancellable, - &local); - if (nm_dbus_error_is (local, NM_DBUS_ERROR_NAME_UNKNOWN_METHOD)) { - guint timeout_msec; - - if (_register_should_retry (priv, &timeout_msec)) { - if (timeout_msec > 0) - g_usleep (timeout_msec * 1000LU); - continue; - } - } - - priv->registering_timeout_msec = 0; - - if (local) { - g_dbus_error_strip_remote_error (local); - g_propagate_error (error, g_steal_pointer (&local)); - _internal_unregister (self); - return FALSE; - } - - priv->registered = TRUE; - _notify (self, PROP_REGISTERED); - return TRUE; - } + /* This is a synchronous function, meaning: we are not allowed to iterate + * the caller's GMainContext. This is a catch 22, because we don't want + * to perform synchronous calls that bypasses the ordering of our otherwise + * asynchronous mode of operation. Hence, we always signal success. + * That's why this function is deprecated. + * + * So despite claiming success, we might still be in the process of registering + * or NetworkManager might not be available. + * + * This is a change in behavior with respect to libnm before 1.24. + */ + return TRUE; } -/*****************************************************************************/ - -typedef struct { - GCancellable *cancellable; - GSource *timeout_source; - gulong cancellable_signal_id; -} RegisterData; - static void -_register_data_free (RegisterData *register_data) +_register_cancelled_cb (GObject *object, gpointer user_data) { - nm_clear_g_cancellable_disconnect (register_data->cancellable, ®ister_data->cancellable_signal_id); - nm_clear_g_source_inst (®ister_data->timeout_source); - g_clear_object (®ister_data->cancellable); - nm_g_slice_free (register_data); -} - -static gboolean -_register_retry_cb (gpointer user_data) -{ - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); + GTask *task0 = user_data; + gs_unref_object GTask *task = NULL; + NMSecretAgentOld *self = g_task_get_source_object (task0); NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GCancellable *cancellable; + gulong *p_cancelled_id; + NMCListElem *elem; + gs_free_error GError *error = NULL; - _LOGT ("register: retry registration..."); + elem = nm_c_list_elem_find_first (&priv->pending_tasks_register_lst_head, x, x == task0); - g_task_set_task_data (task, NULL, NULL); + g_return_if_fail (elem); - cancellable = g_task_get_cancellable (task); + task = nm_c_list_elem_free_steal (elem); - nmdbus_agent_manager_call_register_with_capabilities (priv->manager_proxy, - priv->identifier, - priv->capabilities, - cancellable, - _register_call_cb, - g_steal_pointer (&task)); - return G_SOURCE_REMOVE; -} - -static void -_register_cancelled_cb (GCancellable *cancellable, - gpointer user_data) -{ - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); - RegisterData *register_data = g_task_get_task_data (task); - GError *error = NULL; - - nm_clear_g_signal_handler (register_data->cancellable, ®ister_data->cancellable_signal_id); - g_task_set_task_data (task, NULL, NULL); - - _LOGT ("register: registration cancelled. Stop waiting..."); + p_cancelled_id = g_task_get_task_data (task); + if (p_cancelled_id) { + g_signal_handler_disconnect (g_task_get_cancellable (task), *p_cancelled_id); + g_task_set_task_data (task, NULL, NULL); + } nm_utils_error_set_cancelled (&error, FALSE, NULL); g_task_return_error (task, error); } -static void -_register_call_cb (GObject *proxy, - GAsyncResult *result, - gpointer user_data) -{ - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - gs_free_error GError *error = NULL; - - nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error); - - if (nm_utils_error_is_cancelled (error, FALSE)) { - /* FIXME: we should unregister right away. For now, don't do that, likely the - * application is anyway about to exit. */ - } else if (nm_dbus_error_is (error, NM_DBUS_ERROR_NAME_UNKNOWN_METHOD)) { - gboolean already_cancelled = FALSE; - RegisterData *register_data; - guint timeout_msec; - - if (!_register_should_retry (priv, &timeout_msec)) - goto done; - - _LOGT ("register: registration failed with error \"%s\". Retry in %u msec...", error->message, timeout_msec); - nm_assert (G_IS_TASK (task)); - nm_assert (!g_task_get_task_data (task)); - - register_data = g_slice_new (RegisterData); - - *register_data = (RegisterData) { - .cancellable = nm_g_object_ref (g_task_get_cancellable (task)), - }; - - g_task_set_task_data (task, - register_data, - (GDestroyNotify) _register_data_free); - - if (register_data->cancellable) { - register_data->cancellable_signal_id = g_cancellable_connect (register_data->cancellable, - G_CALLBACK (_register_cancelled_cb), - task, - NULL); - if (register_data->cancellable_signal_id == 0) - already_cancelled = TRUE; - } - - if (!already_cancelled) { - register_data->timeout_source = nm_g_source_attach (nm_g_timeout_source_new (timeout_msec, - g_task_get_priority (task), - _register_retry_cb, - task, - NULL), - g_task_get_context (task)); - } - - /* The reference of the task is owned by the _register_cancelled_cb and _register_retry_cb actions. - * Whichever completes first, will consume it. */ - g_steal_pointer (&task); - return; - } - -done: - priv->registering_timeout_msec = 0; - - if (error) { - _LOGT ("register: registration failed with error \"%s\"", error->message); - g_dbus_error_strip_remote_error (error); - _internal_unregister (self); - g_task_return_error (task, g_steal_pointer (&error)); - return; - } - - _LOGT ("register: registration succeeded"); - priv->registered = TRUE; - _notify (self, PROP_REGISTERED); - - g_task_return_boolean (task, TRUE); -} - /** * nm_secret_agent_old_register_async: * @self: a #NMSecretAgentOld @@ -769,8 +806,16 @@ done: * manager, indicating to NetworkManager that the agent is able to provide and * save secrets for connections on behalf of its user. * - * It is a programmer error to attempt to register an agent that is already - * registered, or in the process of registering. + * Since 1.24, registration cannot fail and is idempotent. It has + * the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %TRUE + * or nm_secret_agent_old_enable(). + * + * Since 1.24, the asynchronous result indicates whether the instance is successfully + * registered. In any case, this call enables the agent and it will automatically + * try to register and handle secret requests. A failure of this function only indicates + * that currently the instance might not be ready (but since it will automatically + * try to recover, it might be ready in a moment afterwards). Use this function if + * you want to check and ensure that the agent is registered. **/ void nm_secret_agent_old_register_async (NMSecretAgentOld *self, @@ -779,54 +824,39 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, gpointer user_data) { NMSecretAgentOldPrivate *priv; - NMSecretAgentOldClass *class; - gs_unref_object GTask *task = NULL; - gs_free_error GError *error = NULL; g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); + g_return_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable)); priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_if_fail (priv->registered == FALSE); - g_return_if_fail (priv->registering_timeout_msec == 0); - g_return_if_fail (priv->dbus_connection != NULL); - g_return_if_fail (priv->manager_proxy != NULL); + g_return_if_fail (priv->is_initialized && !priv->is_destroyed); - /* Also make sure the subclass can actually respond to secrets requests */ - class = NM_SECRET_AGENT_OLD_GET_CLASS (self); - g_return_if_fail (class->get_secrets != NULL); - g_return_if_fail (class->save_secrets != NULL); - g_return_if_fail (class->delete_secrets != NULL); + if (callback) { + GTask *task; - task = nm_g_task_new (self, cancellable, nm_secret_agent_old_register_async, callback, user_data); + task = nm_g_task_new (self, cancellable, nm_secret_agent_old_register_async, callback, user_data); - if (!check_nm_running (self, &error)) { - _LOGT ("register: failed because NetworkManager is not running"); - g_task_return_error (task, g_steal_pointer (&error)); - return; + c_list_link_tail (&priv->pending_tasks_register_lst_head, + &nm_c_list_elem_new_stale (task)->lst); + + if (cancellable) { + gulong cancelled_id; + + cancelled_id = g_cancellable_connect (cancellable, + G_CALLBACK (_register_cancelled_cb), + task, + NULL); + if (cancelled_id != 0) { + g_task_set_task_data (task, + g_memdup (&cancelled_id, sizeof (cancelled_id)), + g_free); + } + } } - /* Export our secret agent interface before registering with the manager */ - if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent), - priv->dbus_connection, - NM_DBUS_PATH_SECRET_AGENT, - &error)) { - _LOGT ("register: failed to export D-Bus service: %s", error->message); - g_task_return_error (task, g_steal_pointer (&error)); - return; - } - - priv->suppress_auto = FALSE; - priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_msec () + REGISTER_RETRY_TIMEOUT_MSEC; - priv->registering_try_count = 0; - - _LOGT ("register: starting asynchronous registration..."); - nmdbus_agent_manager_call_register_with_capabilities (priv->manager_proxy, - priv->identifier, - priv->capabilities, - cancellable, - _register_call_cb, - g_steal_pointer (&task)); + priv->is_enabled = TRUE; + _register_state_change (self); } /** @@ -838,6 +868,10 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, * Gets the result of a call to nm_secret_agent_old_register_async(). * * Returns: %TRUE if registration was successful, %FALSE on error. + * + * Since 1.24, registration cannot fail and is idempotent. It has + * the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %TRUE + * or nm_secret_agent_old_enable(). **/ gboolean nm_secret_agent_old_register_finish (NMSecretAgentOld *self, @@ -861,6 +895,12 @@ nm_secret_agent_old_register_finish (NMSecretAgentOld *self, * store secrets on behalf of this user. * * Returns: %TRUE if unregistration was successful, %FALSE on error + * + * Since 1.24, registration cannot fail and is idempotent. It has + * the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %FALSE + * or nm_secret_agent_old_enable(). + * + * Deprecated: 1.24: use nm_secret_agent_old_enable() **/ gboolean nm_secret_agent_old_unregister (NMSecretAgentOld *self, @@ -868,43 +908,19 @@ nm_secret_agent_old_unregister (NMSecretAgentOld *self, GError **error) { NMSecretAgentOldPrivate *priv; - gboolean success; g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); + g_return_val_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable), FALSE); + g_return_val_if_fail (!error || !*error, FALSE); priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_val_if_fail (priv->dbus_connection != NULL, FALSE); - g_return_val_if_fail (priv->manager_proxy != NULL, FALSE); + g_return_val_if_fail (priv->is_initialized && !priv->is_destroyed, FALSE); - priv->suppress_auto = TRUE; + priv->is_enabled = FALSE; + _register_state_change (self); - success = nmdbus_agent_manager_call_unregister_sync (priv->manager_proxy, cancellable, error); - if (error && *error) - g_dbus_error_strip_remote_error (*error); - _internal_unregister (self); - - return success; -} - -static void -unregister_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) -{ - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); - gs_free_error GError *error = NULL; - - _internal_unregister (self); - - if (!nmdbus_agent_manager_call_unregister_finish (NMDBUS_AGENT_MANAGER (proxy), - result, - &error)) { - g_dbus_error_strip_remote_error (error); - g_task_return_error (task, g_steal_pointer (&error)); - return; - } - - g_task_return_boolean (task, TRUE); + return !g_cancellable_set_error_if_cancelled (cancellable, error); } /** @@ -917,6 +933,12 @@ unregister_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) * Asynchronously unregisters the #NMSecretAgentOld with the NetworkManager secret * manager, indicating to NetworkManager that the agent will no longer provide * or store secrets on behalf of this user. + * + * Since 1.24, registration cannot fail and is idempotent. It has + * the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %FALSE + * or nm_secret_agent_old_enable(). + * + * Deprecated: 1.24: use nm_secret_agent_old_enable() **/ void nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, @@ -925,29 +947,23 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, gpointer user_data) { NMSecretAgentOldPrivate *priv; - gs_unref_object GTask *task = NULL; - gs_free_error GError *error = NULL; g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); + g_return_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable)); priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - g_return_if_fail (priv->dbus_connection != NULL); - g_return_if_fail (priv->manager_proxy != NULL); + g_return_if_fail (priv->is_initialized && !priv->is_destroyed); - task = nm_g_task_new (self, cancellable, nm_secret_agent_old_unregister_async, callback, user_data); + if (callback) { + gs_unref_object GTask *task = NULL; - if (!check_nm_running (self, &error)) { - g_task_return_error (task, g_steal_pointer (&error)); - return; + task = nm_g_task_new (self, cancellable, nm_secret_agent_old_unregister_async, callback, user_data); + g_task_return_boolean (task, TRUE); } - priv->suppress_auto = TRUE; - - nmdbus_agent_manager_call_unregister (priv->manager_proxy, - cancellable, - unregister_cb, - g_steal_pointer (&task)); + priv->is_enabled = FALSE; + _register_state_change (self); } /** @@ -959,6 +975,12 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, * Gets the result of a call to nm_secret_agent_old_unregister_async(). * * Returns: %TRUE if unregistration was successful, %FALSE on error. + * + * Since 1.24, registration cannot fail and is idempotent. It has + * the same effect as setting %NM_SECRET_AGENT_OLD_AUTO_REGISTER to %FALSE + * or nm_secret_agent_old_enable(). + * + * Deprecated: 1.24: use nm_secret_agent_old_enable() **/ gboolean nm_secret_agent_old_unregister_finish (NMSecretAgentOld *self, @@ -971,20 +993,6 @@ nm_secret_agent_old_unregister_finish (NMSecretAgentOld *self, return g_task_propagate_boolean (G_TASK (result), error); } -/** - * nm_secret_agent_old_get_registered: - * @self: a #NMSecretAgentOld - * - * Returns: a %TRUE if the agent is registered, %FALSE if it is not. - **/ -gboolean -nm_secret_agent_old_get_registered (NMSecretAgentOld *self) -{ - g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); - - return NM_SECRET_AGENT_OLD_GET_PRIVATE (self)->registered; -} - /*****************************************************************************/ /** @@ -1114,118 +1122,675 @@ validate_identifier (const char *identifier) /*****************************************************************************/ -static void -init_common (NMSecretAgentOld *self) +static gboolean +_register_retry_cb (gpointer user_data) { + NMSecretAgentOld *self = user_data; NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + nm_auto_pop_gmaincontext GMainContext *dbus_context = NULL; - priv->session_bus = _nm_dbus_bus_type () == G_BUS_TYPE_SESSION; + dbus_context = nm_g_main_context_push_thread_default_if_necessary (priv->dbus_context); - g_signal_connect (priv->manager_proxy, "notify::g-name-owner", - G_CALLBACK (name_owner_changed), self); + nm_clear_g_source_inst (&priv->registering_retry_source); + _register_dbus_call (self); + return G_SOURCE_CONTINUE; } static void -init_async_registered (GObject *object, GAsyncResult *result, gpointer user_data) +_register_call_cb (GObject *source, + GAsyncResult *result, + gpointer user_data) { - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); - GError *error = NULL; + NMSecretAgentOld *self = user_data; + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + gs_unref_variant GVariant *ret = NULL; + gs_free_error GError *error = NULL; - nm_secret_agent_old_register_finish (self, result, &error); + ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error); - if (error) - g_task_return_error (task, error); + if (nm_utils_error_is_cancelled (error, FALSE)) + return; + + nm_assert (!priv->registering_retry_source); + nm_assert (!priv->is_registered); + nm_assert (priv->registering_cancellable); + + if ( nm_dbus_error_is (error, NM_DBUS_ERROR_NAME_UNKNOWN_METHOD) + && nm_utils_get_monotonic_timestamp_msec () < priv->registering_timeout_msec) { + guint timeout_msec; + + timeout_msec = (2u << NM_MIN (6u, ++priv->registering_try_count)); + + _LOGT ("register: registration failed with error \"%s\". Retry in %u msec...", error->message, timeout_msec); + + priv->registering_retry_source = nm_g_source_attach (nm_g_timeout_source_new (timeout_msec, + G_PRIORITY_DEFAULT, + _register_retry_cb, + self, + NULL), + priv->dbus_context); + return; + } + + g_clear_object (&priv->registering_cancellable); + + if (error) { + /* registration apparently failed. However we still keep priv->registered_against_server TRUE, because + * + * - eventually we want to still make an Unregister() call. Even if it probably has no effect, + * better be sure. + * + * - we actually accept secret request (from the right name owner). We register so that + * NetworkManager knows that we are here. We don't require the registration to succeed + * for our purpose. If NetworkManager makes requests for us, despite the registration + * failing, that is fine. */ + _LOGT ("register: registration failed with error \"%s\"", error->message); + goto out; + } + + _LOGT ("register: registration succeeded"); + priv->is_registered = TRUE; + _notify (self, PROP_REGISTERED); + +out: + _register_state_complete (self); +} + +static void +_register_dbus_call (NMSecretAgentOld *self) +{ + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + _dbus_connection_call (self, + nm_ref_string_get_str (priv->name_owner_curr), + NM_DBUS_PATH_AGENT_MANAGER, + NM_DBUS_INTERFACE_AGENT_MANAGER, + "RegisterWithCapabilities", + g_variant_new ("(su)", + priv->identifier, + (guint32) priv->capabilities), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + _CALL_REGISTER_TIMEOUT_MSEC, + priv->registering_cancellable, + _register_call_cb, + self); +} + +static void +_get_connection_unix_user_cb (GObject *source, + GAsyncResult *result, + gpointer user_data) +{ + NMSecretAgentOld *self; + NMSecretAgentOldPrivate *priv; + gs_unref_variant GVariant *ret = NULL; + gs_free_error GError *error = NULL; + guint32 sender_uid = 0; + + ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error); + if (nm_utils_error_is_cancelled (error, FALSE)) + return; + + self = user_data; + priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + nm_assert (priv->registering_cancellable); + nm_assert (!priv->registered_against_server); + + if (ret) + g_variant_get (ret, "(u)", &sender_uid); + + if ( ret + && sender_uid == 0) + _LOGT ("register: peer %s is owned by root. Validated to accept requests.", priv->name_owner_curr->str); + else if ( ret + && priv->session_bus) { + _LOGT ("register: peer %s is owned by user %d for session bus. Validated to accept requests.", priv->name_owner_curr->str, sender_uid); + } else { + /* the peer is not validated. We don't actually register. */ + if (ret) + _LOGT ("register: peer %s is owned by user %u. Not validated as NetworkManager service.", priv->name_owner_curr->str, sender_uid); + else + _LOGT ("register: failed to get user id for peer %s: %s. Not validated as NetworkManager service.", priv->name_owner_curr->str, error->message); + + /* we actually don't do anything and keep the agent unregistered. + * + * We keep priv->registering_cancellable set to not retry this again, until we loose the + * name owner. But the state of the agent is lingering and won't accept any requests. */ + return; + } + + priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_msec () + REGISTER_RETRY_TIMEOUT_MSEC; + priv->registering_try_count = 0; + priv->registered_against_server = TRUE; + _register_dbus_call (self); +} + +/*****************************************************************************/ + +static void +_name_owner_changed (NMSecretAgentOld *self, + const char *name_owner, + gboolean is_event) +{ + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + if (is_event) { + if (priv->name_owner_cancellable) { + /* we are still fetching the name-owner. Ignore this event. */ + return; + } + } else + g_clear_object (&priv->name_owner_cancellable); + + nm_ref_string_unref (priv->name_owner_next); + priv->name_owner_next = nm_ref_string_new (nm_str_not_empty (name_owner)); + + _LOGT ("name-owner changed: %s%s%s -> %s%s%s", + NM_PRINT_FMT_QUOTED (priv->name_owner_curr, "\"", priv->name_owner_curr->str, "\"", "(null)"), + NM_PRINT_FMT_QUOTED (priv->name_owner_next, "\"", priv->name_owner_next->str, "\"", "(null)")); + + _register_state_change (self); +} + +static void +_name_owner_changed_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, + gpointer user_data) +{ + NMSecretAgentOld *self = user_data; + const char *new_owner; + + if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) + return; + + g_variant_get (parameters, + "(&s&s&s)", + NULL, + NULL, + &new_owner); + + _name_owner_changed (self, new_owner, TRUE); +} + +static void +_name_owner_get_cb (const char *name_owner, + GError *error, + gpointer user_data) +{ + if ( name_owner + || !nm_utils_error_is_cancelled (error, FALSE)) + _name_owner_changed (user_data, name_owner, FALSE); +} + +/*****************************************************************************/ + +static void +_method_call (GDBusConnection *connection, + const char *sender, + const char *object_path, + const char *interface_name, + const char *method_name, + GVariant *parameters, + GDBusMethodInvocation *context, + gpointer user_data) +{ + NMSecretAgentOld *self = user_data; + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + nm_assert (nm_streq0 (object_path, NM_DBUS_PATH_SECRET_AGENT)); + nm_assert (nm_streq0 (interface_name, NM_DBUS_INTERFACE_SECRET_AGENT)); + nm_assert (sender); + nm_assert (nm_streq0 (sender, g_dbus_method_invocation_get_sender (context))); + + if ( !priv->name_owner_curr + || !priv->registered_against_server) { + /* priv->registered_against_server means that we started to register, but not necessarily + * that the registration fully succeeded. However, we already authenticated the request + * and so we accept it, even if the registration is not yet complete. */ + g_dbus_method_invocation_return_error_literal (context, + NM_SECRET_AGENT_ERROR, + NM_SECRET_AGENT_ERROR_PERMISSION_DENIED, + "Request by non authenticated peer rejected"); + return; + } + + if (nm_streq (method_name, "GetSecrets")) + impl_get_secrets (self, parameters, context); + else if (nm_streq (method_name, "CancelGetSecrets")) + impl_cancel_get_secrets (self, parameters, context); + else if (nm_streq (method_name, "SaveSecrets")) + impl_save_secrets (self, parameters, context); + else if (nm_streq (method_name, "DeleteSecrets")) + impl_delete_secrets (self, parameters, context); else - g_task_return_boolean (task, TRUE); + nm_assert_not_reached (); +} + +/*****************************************************************************/ + +static void +_register_state_complete (NMSecretAgentOld *self) +{ + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + NMCListElem *elem; + gboolean any_tasks_to_complete = FALSE; + + if (!c_list_is_empty (&priv->pending_tasks_register_lst_head)) { + /* add a dummy sentinel. We want to complete all the task we started + * so far, but as we invoke user callbacks, the user might register + * new tasks. Those we don't complete in this run. */ + g_object_ref (self); + any_tasks_to_complete = TRUE; + c_list_link_tail (&priv->pending_tasks_register_lst_head, + &nm_c_list_elem_new_stale (&any_tasks_to_complete)->lst); + } + + _init_complete (self, NULL); + + if (any_tasks_to_complete) { + while ((elem = c_list_first_entry (&priv->pending_tasks_register_lst_head, NMCListElem, lst))) { + gpointer data = nm_c_list_elem_free_steal (elem); + gs_unref_object GTask *task = NULL; + + if (data == &any_tasks_to_complete) { + any_tasks_to_complete = FALSE; + break; + } + + task = data; + + if (!priv->is_registered) { + g_task_return_error (task, + g_error_new_literal (NM_SECRET_AGENT_ERROR, + NM_SECRET_AGENT_ERROR_FAILED, + _("registration failed"))); + continue; + } + g_task_return_boolean (task, TRUE); + } + nm_assert (!any_tasks_to_complete); + g_object_unref (self); + } } static void -init_async_got_proxy (GObject *object, GAsyncResult *result, gpointer user_data) +_register_state_change_do (NMSecretAgentOld *self) { - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GError *error = NULL; - priv->manager_proxy = nmdbus_agent_manager_proxy_new_finish (result, &error); - if (!priv->manager_proxy) { - g_task_return_error (task, error); + if (priv->is_destroyed) + priv->is_enabled = FALSE; + + if ( !priv->is_enabled + || priv->registration_force_unregister + || priv->name_owner_curr != priv->name_owner_next) { + GetSecretsInfo *info; + + while ((info = c_list_first_entry (&priv->gsi_lst_head, GetSecretsInfo, gsi_lst))) { + _cancel_get_secret_request (self, info, "The secret agent is going away"); + _register_state_change (self); + return; + } + + priv->registration_force_unregister = FALSE; + + nm_clear_g_cancellable (&priv->registering_cancellable); + nm_clear_g_source_inst (&priv->registering_retry_source); + + if (priv->registered_against_server) { + priv->registered_against_server = FALSE; + if (priv->name_owner_curr) { + _LOGT ("register: unregister from %s", priv->name_owner_curr->str); + _dbus_connection_call (self, + priv->name_owner_curr->str, + NM_DBUS_PATH_AGENT_MANAGER, + NM_DBUS_INTERFACE_AGENT_MANAGER, + "Unregister", + g_variant_new ("()"), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + _CALL_REGISTER_TIMEOUT_MSEC, + NULL, + NULL, + NULL); + } + } + + if (!priv->is_enabled) { + nm_clear_g_cancellable (&priv->name_owner_cancellable); + nm_clear_g_dbus_connection_signal (priv->dbus_connection, + &priv->name_owner_changed_id); + nm_clear_pointer (&priv->name_owner_curr, nm_ref_string_unref); + nm_clear_pointer (&priv->name_owner_next, nm_ref_string_unref); + } + + if (priv->is_registered) { + priv->is_registered = FALSE; + if (!priv->is_destroyed) { + _LOGT ("register: now unregistered"); + _notify (self, PROP_REGISTERED); + _register_state_change (self); + return; + } + } + + if (!priv->is_enabled) { + _register_state_complete (self); + return; + } + + if (priv->name_owner_curr != priv->name_owner_next) { + nm_ref_string_unref (priv->name_owner_curr); + priv->name_owner_curr = nm_ref_string_ref (priv->name_owner_next); + } + } + + if (priv->name_owner_changed_id == 0) { + nm_assert (!priv->name_owner_cancellable); + nm_assert (!priv->name_owner_curr); + nm_assert (!priv->name_owner_next); + priv->name_owner_cancellable = g_cancellable_new (); + priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, + NM_DBUS_SERVICE, + _name_owner_changed_cb, + self, + NULL); + nm_dbus_connection_call_get_name_owner (priv->dbus_connection, + NM_DBUS_SERVICE, + -1, + priv->name_owner_cancellable, + _name_owner_get_cb, + self); return; } - init_common (self); - - if (!priv->auto_register) { - g_task_return_boolean (task, TRUE); + if (priv->name_owner_cancellable) { + /* we still wait for the name owner. Nothing to do for now. */ return; } - nm_secret_agent_old_register_async (self, - g_task_get_cancellable (task), - init_async_registered, - task); - g_steal_pointer (&task); + if (!priv->name_owner_curr) { + /* we don't have a name owner. We are done and wait. */ + _register_state_complete (self); + return; + } + + if (priv->registering_cancellable) { + /* we are already registering... wait longer. */ + return; + } + + nm_assert (!priv->registering_retry_source); + + if (!priv->is_registered) { + /* start registering... */ + priv->registering_cancellable = g_cancellable_new (); + _dbus_connection_call (self, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetConnectionUnixUser", + g_variant_new ("(s)", priv->name_owner_curr->str), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + _CALL_REGISTER_TIMEOUT_MSEC, + priv->registering_cancellable, + _get_connection_unix_user_cb, + self); + return; + } + + /* we are fully registered and done. */ + _register_state_complete (self); } static void -init_async_start (NMSecretAgentOld *self, GTask *task_take) +_register_state_change (NMSecretAgentOld *self) +{ + _nm_unused gs_unref_object NMSecretAgentOld *self_keep_alive = g_object_ref (self); + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + nm_auto_pop_gmaincontext GMainContext *dbus_context = NULL; + + if (priv->register_state_change_reenter == 0) { + /* We are not yet initialized. Do nothing. */ + return; + } + + if (priv->register_state_change_reenter != 1) { + /* Recursive calls are prevented. Do nothing for now, but repeat + * the state change afterwards. */ + priv->register_state_change_reenter = 3; + return; + } + + dbus_context = nm_g_main_context_push_thread_default_if_necessary (priv->dbus_context); + +again: + priv->register_state_change_reenter = 2; + + _register_state_change_do (self); + + if (priv->register_state_change_reenter != 2) + goto again; + + priv->register_state_change_reenter = 1; +} + +/*****************************************************************************/ + +static void +_init_complete (NMSecretAgentOld *self, + GError *error_take) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + gs_free_error GError *error = g_steal_pointer (&error_take); + GError *error_cancelled = NULL; - nmdbus_agent_manager_proxy_new (priv->dbus_connection, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES - | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, - NM_DBUS_SERVICE, - NM_DBUS_PATH_AGENT_MANAGER, - g_task_get_cancellable (task_take), - init_async_got_proxy, - task_take); - g_steal_pointer (&task_take); + if (!priv->init_data) + return; + + if (g_cancellable_set_error_if_cancelled (priv->init_data->cancellable, &error_cancelled)) { + g_clear_error (&error); + g_propagate_error (&error, error_cancelled); + } + + priv->is_initialized = (!error); + + _LOGT ("%s init complete with %s%s%s", + priv->init_data->is_sync ? "sync" : "async", + NM_PRINT_FMT_QUOTED (error_take, "error: ", error_take->message, "", "success")); + + nml_init_data_return (g_steal_pointer (&priv->init_data), + g_steal_pointer (&error)); } static void -init_async_got_bus (GObject *initable, GAsyncResult *result, gpointer user_data) +_init_register_object (NMSecretAgentOld *self) { - gs_unref_object GTask *task = user_data; - NMSecretAgentOld *self = g_task_get_source_object (task); NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GError *error = NULL; + gs_free_error GError *error = NULL; + GDBusInterfaceVTable interface_vtable = { + .method_call = _method_call, + }; + + if (g_cancellable_set_error_if_cancelled (priv->init_data->cancellable, &error)) { + _init_complete (self, g_steal_pointer (&error)); + return; + } + + priv->exported_id = g_dbus_connection_register_object (priv->dbus_connection, + NM_DBUS_PATH_SECRET_AGENT, + (GDBusInterfaceInfo*) &interface_info, + &interface_vtable, + self, + NULL, + &error); + if (priv->exported_id == 0) { + _init_complete (self, g_steal_pointer (&error)); + return; + } + + priv->register_state_change_reenter = 1; + + _register_state_change (self); +} + +static void +_init_got_bus (GObject *initable, GAsyncResult *result, gpointer user_data) +{ + NMSecretAgentOld *self = user_data; + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + gs_free_error GError *error = NULL; priv->dbus_connection = g_bus_get_finish (result, &error); if (!priv->dbus_connection) { - g_task_return_error (task, error); + _init_complete (self, g_steal_pointer (&error)); return; } - init_async_start (self, g_steal_pointer (&task)); + _LOGT ("init: got GDBusConnection"); _notify (self, PROP_DBUS_CONNECTION); + + _init_register_object (self); } static void -init_async (GAsyncInitable *initable, int io_priority, - GCancellable *cancellable, GAsyncReadyCallback callback, - gpointer user_data) +_init_start (NMSecretAgentOld *self) { - NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (initable); NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GTask *task; - - _LOGT ("init-async starting..."); - - task = g_task_new (self, cancellable, callback, user_data); - g_task_set_priority (task, io_priority); if (!priv->dbus_connection) { - g_bus_get (_nm_dbus_bus_type (), - cancellable, - init_async_got_bus, - task); + GBusType bus_type; + + bus_type = _nm_dbus_bus_type (); + + priv->session_bus = (bus_type == G_BUS_TYPE_SESSION); + + g_bus_get (bus_type, + priv->init_data->cancellable, + _init_got_bus, + self); return; } - init_async_start (self, task); + _init_register_object (self); +} + +static void +init_async (GAsyncInitable *initable, + int io_priority, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + NMSecretAgentOld *self; + NMSecretAgentOldClass *klass; + NMSecretAgentOldPrivate *priv; + nm_auto_pop_gmaincontext GMainContext *dbus_context = NULL; + gs_unref_object GTask *task = NULL; + + g_return_if_fail (NM_IS_SECRET_AGENT_OLD (initable)); + + self = NM_SECRET_AGENT_OLD (initable); + priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + g_return_if_fail (!priv->dbus_context); + g_return_if_fail (!priv->is_destroyed); + + klass = NM_SECRET_AGENT_OLD_GET_CLASS (self); + g_return_if_fail (klass->get_secrets); + g_return_if_fail (klass->cancel_get_secrets); + g_return_if_fail (klass->save_secrets); + g_return_if_fail (klass->delete_secrets); + + _LOGT ("init-async starting..."); + + priv->dbus_context = g_main_context_ref (priv->main_context); + + dbus_context = nm_g_main_context_push_thread_default_if_necessary (priv->dbus_context); + + task = nm_g_task_new (self, cancellable, init_async, callback, user_data); + g_task_set_priority (task, io_priority); + + priv->init_data = nml_init_data_new_async (cancellable, g_steal_pointer (&task)); + + _init_start (self); +} + +static gboolean +init_finish (GAsyncInitable *initable, GAsyncResult *result, GError **error) +{ + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (initable), FALSE); + g_return_val_if_fail (nm_g_task_is_valid (result, initable, init_async), FALSE); + + return g_task_propagate_boolean (G_TASK (result), error); +} + +/*****************************************************************************/ + +static gboolean +init_sync (GInitable *initable, + GCancellable *cancellable, + GError **error) +{ + gs_unref_object NMSecretAgentOld *self = NULL; + NMSecretAgentOldPrivate *priv; + NMSecretAgentOldClass *klass; + GMainLoop *main_loop; + GError *local_error = NULL; + + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (initable), FALSE); + + self = g_object_ref (NM_SECRET_AGENT_OLD (initable)); + priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + g_return_val_if_fail (!priv->dbus_context, FALSE); + g_return_val_if_fail (!priv->is_destroyed, FALSE); + + klass = NM_SECRET_AGENT_OLD_GET_CLASS (self); + g_return_val_if_fail (klass->get_secrets, FALSE); + g_return_val_if_fail (klass->cancel_get_secrets, FALSE); + g_return_val_if_fail (klass->save_secrets, FALSE); + g_return_val_if_fail (klass->delete_secrets, FALSE); + + _LOGT ("init-sync"); + + /* See NMClient's sync-init method for explanation about why we create + * an internal GMainContext priv->dbus_context. */ + + priv->dbus_context = g_main_context_new (); + + g_main_context_push_thread_default (priv->dbus_context); + + main_loop = g_main_loop_new (priv->dbus_context, FALSE); + + priv->init_data = nml_init_data_new_sync (cancellable, main_loop, &local_error); + + _init_start (self); + + g_main_loop_run (main_loop); + + g_main_loop_unref (main_loop); + + g_main_context_pop_thread_default (priv->dbus_context); + + nm_context_busy_watcher_integrate_source (priv->main_context, + priv->dbus_context, + priv->context_busy_watcher); + + if (local_error) { + g_propagate_error (error, local_error); + return FALSE; + } + + return TRUE; } /*****************************************************************************/ @@ -1249,7 +1814,7 @@ get_property (GObject *object, g_value_set_boolean (value, priv->auto_register); break; case PROP_REGISTERED: - g_value_set_boolean (value, priv->registered); + g_value_set_boolean (value, priv->is_registered); break; case PROP_CAPABILITIES: g_value_set_flags (value, priv->capabilities); @@ -1266,8 +1831,9 @@ set_property (GObject *object, const GValue *value, GParamSpec *pspec) { - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (object); - const char *identifier; + NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (object); + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + guint u; switch (prop_id) { case PROP_DBUS_CONNECTION: @@ -1275,18 +1841,24 @@ set_property (GObject *object, priv->dbus_connection = g_value_dup_object (value); break; case PROP_IDENTIFIER: - identifier = g_value_get_string (value); - - g_return_if_fail (validate_identifier (identifier)); - - g_free (priv->identifier); - priv->identifier = g_strdup (identifier); + /* construct-only */ + priv->identifier = g_value_dup_string (value); + g_return_if_fail (validate_identifier (priv->identifier)); break; case PROP_AUTO_REGISTER: + /* construct */ priv->auto_register = g_value_get_boolean (value); + priv->is_enabled = priv->auto_register; + _register_state_change (self); break; case PROP_CAPABILITIES: - priv->capabilities = g_value_get_flags (value); + /* construct */ + u = g_value_get_flags (value); + if (u != priv->capabilities) { + priv->capabilities = u; + priv->registration_force_unregister = TRUE; + _register_state_change (self); + } break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); @@ -1304,83 +1876,46 @@ nm_secret_agent_old_init (NMSecretAgentOld *self) _LOGT ("create new instance"); c_list_init (&priv->gsi_lst_head); + c_list_init (&priv->pending_tasks_register_lst_head); priv->main_context = g_main_context_ref_thread_default (); - - priv->dbus_secret_agent = nmdbus_secret_agent_skeleton_new (); - _nm_dbus_bind_properties (self, priv->dbus_secret_agent); - _nm_dbus_bind_methods (self, priv->dbus_secret_agent, - "GetSecrets", impl_secret_agent_old_get_secrets, - "CancelGetSecrets", impl_secret_agent_old_cancel_get_secrets, - "DeleteSecrets", impl_secret_agent_old_delete_secrets, - "SaveSecrets", impl_secret_agent_old_save_secrets, - NULL); -} - -static gboolean -init_sync (GInitable *initable, GCancellable *cancellable, GError **error) -{ - NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (initable); - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - - _LOGT ("init-sync"); - - if (!priv->dbus_connection) { - priv->dbus_connection = g_bus_get_sync (_nm_dbus_bus_type (), cancellable, error); - if (!priv->dbus_connection) - return FALSE; - _notify (self, PROP_DBUS_CONNECTION); - } - - priv->manager_proxy = nmdbus_agent_manager_proxy_new_sync (priv->dbus_connection, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES - | G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, - NM_DBUS_SERVICE, - NM_DBUS_PATH_AGENT_MANAGER, - cancellable, - error); - if (!priv->manager_proxy) - return FALSE; - - init_common (self); - - if (priv->auto_register) - return nm_secret_agent_old_register (self, cancellable, error); - else - return TRUE; + priv->context_busy_watcher = g_object_new (G_TYPE_OBJECT, NULL); } static void dispose (GObject *object) { NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (object); - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - GetSecretsInfo *info; _LOGT ("disposing"); - if (priv->registered) { - priv->registered = FALSE; - nm_secret_agent_old_unregister_async (self, NULL, NULL, NULL); - } - - nm_clear_g_free (&priv->identifier); - - while ((info = c_list_first_entry (&priv->gsi_lst_head, GetSecretsInfo, gsi_lst))) - get_secrets_info_free (info); - - if (priv->dbus_secret_agent) { - g_signal_handlers_disconnect_matched (priv->dbus_secret_agent, G_SIGNAL_MATCH_DATA, - 0, 0, NULL, NULL, self); - g_clear_object (&priv->dbus_secret_agent); - } - - g_clear_object (&priv->manager_proxy); + _secret_agent_old_destroy (self); G_OBJECT_CLASS (nm_secret_agent_old_parent_class)->dispose (object); +} + +static void +finalize (GObject *object) +{ + NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (object); + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + + _LOGT ("finalizing"); + + if (priv->dbus_context) { + nml_cleanup_context_busy_watcher_on_idle (g_steal_pointer (&priv->context_busy_watcher), + priv->dbus_context); + } g_clear_object (&priv->dbus_connection); + nm_clear_pointer (&priv->dbus_context, g_main_context_unref); nm_clear_pointer (&priv->main_context, g_main_context_unref); + + g_clear_object (&priv->context_busy_watcher); + + g_free (priv->identifier); + + G_OBJECT_CLASS (nm_secret_agent_old_parent_class)->finalize (object); } static void @@ -1390,9 +1925,10 @@ nm_secret_agent_old_class_init (NMSecretAgentOldClass *class) g_type_class_add_private (class, sizeof (NMSecretAgentOldPrivate)); - object_class->dispose = dispose; object_class->get_property = get_property; object_class->set_property = set_property; + object_class->dispose = dispose; + object_class->finalize = finalize; /** * NMSecretAgentOld:dbus-connection: @@ -1437,17 +1973,17 @@ nm_secret_agent_old_class_init (NMSecretAgentOldClass *class) * * In particular, if this property is %TRUE at construct time, then the * agent will register itself with NetworkManager during - * construction/initialization, and initialization will fail with an error - * if the agent is unable to register itself. + * construction/initialization and initialization will only complete + * after registration is completed (either successfully or unsucessfully). + * Since 1.24, a failure to register will no longer cause initialization + * of #NMSecretAgentOld to fail. * * If the property is %FALSE, the agent will not automatically register with - * NetworkManager, and nm_secret_agent_old_register() or + * NetworkManager, and nm_secret_agent_old_enable() or * nm_secret_agent_old_register_async() must be called to register it. * - * Calling nm_secret_agent_old_unregister() will suppress auto-registration - * until nm_secret_agent_old_register() is called, which re-enables - * auto-registration. This ensures that the agent remains un-registered when - * you expect it to be unregistered. + * Calling nm_secret_agent_old_enable() has the same effect as setting this + * property. **/ obj_properties[PROP_AUTO_REGISTER] = g_param_spec_boolean (NM_SECRET_AGENT_OLD_AUTO_REGISTER, "", "", @@ -1471,6 +2007,9 @@ nm_secret_agent_old_class_init (NMSecretAgentOldClass *class) * NMSecretAgentOld:capabilities: * * A bitfield of %NMSecretAgentCapabilities. + * + * Changing this property is possible at any time. In case the secret + * agent is currently registered, this will cause a re-registration. **/ obj_properties[PROP_CAPABILITIES] = g_param_spec_flags (NM_SECRET_AGENT_OLD_CAPABILITIES, "", "", @@ -1492,6 +2031,6 @@ nm_secret_agent_old_initable_iface_init (GInitableIface *iface) static void nm_secret_agent_old_async_initable_iface_init (GAsyncInitableIface *iface) { - iface->init_async = init_async; - /* Use default implementation for init_finish */ + iface->init_async = init_async; + iface->init_finish = init_finish; } diff --git a/libnm/nm-secret-agent-old.h b/libnm/nm-secret-agent-old.h index 2f71fc1987..07deee2ce4 100644 --- a/libnm/nm-secret-agent-old.h +++ b/libnm/nm-secret-agent-old.h @@ -127,9 +127,12 @@ typedef struct { /* Called when the subclass should retrieve and return secrets. Subclass * must copy or reference any arguments it may require after returning from * this method, as the arguments will freed (except for 'self', 'callback', - * and 'user_data' of course). If the request is canceled, the callback - * should still be called, but with the - * NM_SECRET_AGENT_OLD_ERROR_AGENT_CANCELED error. + * and 'user_data' of course). + * + * Before version 1.24, if the request is canceled, the callback + * should still be called, but with the NM_SECRET_AGENT_ERROR_AGENT_CANCELED + * error. Since 1.24, invoking the callback has no effect during cancellation + * and may be omitted. */ void (*get_secrets) (NMSecretAgentOld *self, NMConnection *connection, @@ -141,10 +144,12 @@ typedef struct { gpointer user_data); /* Called when the subclass should cancel an outstanding request to - * get secrets for a given connection. Canceling the request MUST - * call the callback that was passed along with the initial get_secrets - * call, sending the NM_SECRET_AGENT_OLD_ERROR/ - * NM_SECRET_AGENT_OLD_ERROR_AGENT_CANCELED error to that callback. + * get secrets for a given connection. + * + * Before version 1.24, canceling the request MUST call the callback that was + * passed along with the initial get_secrets call, sending the NM_SECRET_AGENT_ERROR/ + * NM_SECRET_AGENT_ERROR_AGENT_CANCELED error to that callback. Since 1.24, + * the get_secrets callback will be ignored during cancellation and may be omitted. */ void (*cancel_get_secrets) (NMSecretAgentOld *self, const char *connection_path, @@ -186,9 +191,20 @@ GDBusConnection *nm_secret_agent_old_get_dbus_connection (NMSecretAgentOld *self NM_AVAILABLE_IN_1_24 GMainContext *nm_secret_agent_old_get_main_context (NMSecretAgentOld *self); -gboolean nm_secret_agent_old_register (NMSecretAgentOld *self, - GCancellable *cancellable, - GError **error); +NM_AVAILABLE_IN_1_24 +GObject *nm_secret_agent_old_get_context_busy_watcher (NMSecretAgentOld *self); + +NM_AVAILABLE_IN_1_24 +const char *nm_secret_agent_old_get_dbus_name_owner (NMSecretAgentOld *self); + +gboolean nm_secret_agent_old_get_registered (NMSecretAgentOld *self); + +/*****************************************************************************/ + +NM_AVAILABLE_IN_1_24 +void nm_secret_agent_old_enable (NMSecretAgentOld *self, + gboolean enable); + void nm_secret_agent_old_register_async (NMSecretAgentOld *self, GCancellable *cancellable, GAsyncReadyCallback callback, @@ -197,18 +213,33 @@ gboolean nm_secret_agent_old_register_finish (NMSecretAgentOld *self, GAsyncResult *result, GError **error); +NM_AVAILABLE_IN_1_24 +void nm_secret_agent_old_destroy (NMSecretAgentOld *self); + +/*****************************************************************************/ + +NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable) +gboolean nm_secret_agent_old_register (NMSecretAgentOld *self, + GCancellable *cancellable, + GError **error); + +NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable) gboolean nm_secret_agent_old_unregister (NMSecretAgentOld *self, GCancellable *cancellable, GError **error); + +NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable) void nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); + +NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable) gboolean nm_secret_agent_old_unregister_finish (NMSecretAgentOld *self, GAsyncResult *result, GError **error); -gboolean nm_secret_agent_old_get_registered (NMSecretAgentOld *self); +/*****************************************************************************/ void nm_secret_agent_old_get_secrets (NMSecretAgentOld *self, NMConnection *connection, diff --git a/libnm/tests/test-secret-agent.c b/libnm/tests/test-secret-agent.c index 26df6f3c6e..9bceeb588d 100644 --- a/libnm/tests/test-secret-agent.c +++ b/libnm/tests/test-secret-agent.c @@ -122,12 +122,12 @@ test_secret_agent_init (TestSecretAgent *agent) } static NMSecretAgentOld * -test_secret_agent_new (void) +test_secret_agent_new (gboolean auto_register) { return nmtstc_context_object_new (test_secret_agent_get_type (), TRUE, NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", - NM_SECRET_AGENT_OLD_AUTO_REGISTER, FALSE, + NM_SECRET_AGENT_OLD_AUTO_REGISTER, auto_register, NULL); } @@ -211,7 +211,7 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data) { static int static_counter = 0; const int counter = static_counter++; - const char *agent_notes = test_data; + const char *create_agent = test_data; NMConnection *connection; NMSettingConnection *s_con; NMSettingWireless *s_wireless; @@ -273,18 +273,25 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data) g_main_loop_run (sadata->loop); g_assert (sadata->connection); - if (agent_notes) { - sadata->agent = test_secret_agent_new (); + if (nm_streq (create_agent, "1")) { + gboolean auto_register = nmtst_get_rand_bool (); - if (!strcmp (agent_notes, "sync")) { + sadata->agent = test_secret_agent_new (auto_register); + + if (auto_register) { + g_assert (nm_secret_agent_old_get_registered (sadata->agent)); nm_secret_agent_old_register (sadata->agent, NULL, &error); g_assert_no_error (error); - g_assert (nm_secret_agent_old_get_registered (sadata->agent)); } else { - nm_secret_agent_old_register_async (sadata->agent, NULL, - register_cb, sadata); + g_assert (!nm_secret_agent_old_get_registered (sadata->agent)); + nm_secret_agent_old_register_async (sadata->agent, + NULL, + register_cb, + sadata); g_main_loop_run (sadata->loop); } + + g_assert (nm_secret_agent_old_get_registered (sadata->agent)); } } @@ -306,6 +313,9 @@ test_cleanup (TestSecretAgentData *sadata, gconstpointer test_data) nm_client_get_context_busy_watcher (sadata->client)); if (sadata->agent) { + nmtst_context_busy_watcher_add (&watcher_data, + nm_secret_agent_old_get_context_busy_watcher (sadata->agent)); + if (nm_secret_agent_old_get_registered (sadata->agent)) { nm_secret_agent_old_unregister (sadata->agent, NULL, &error); g_assert_no_error (error); @@ -542,17 +552,18 @@ test_secret_agent_good (TestSecretAgentData *sadata, gconstpointer test_data) g_assert_cmpint (sadata->secrets_requested, ==, 1); } +/*****************************************************************************/ + static void async_init_cb (GObject *object, GAsyncResult *result, gpointer user_data) { GMainLoop *loop = user_data; - GError *error = NULL; - GObject *agent; + gs_free_error GError *error = NULL; + gs_unref_object GObject *agent = NULL; agent = g_async_initable_new_finish (G_ASYNC_INITABLE (object), result, &error); - g_assert_error (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_FAILED); - g_assert (agent == NULL); - g_clear_error (&error); + nmtst_assert_success (NM_IS_SECRET_AGENT_OLD (agent), error); + g_assert (!nm_secret_agent_old_get_registered (NM_SECRET_AGENT_OLD (agent))); g_main_loop_quit (loop); } @@ -560,89 +571,151 @@ async_init_cb (GObject *object, GAsyncResult *result, gpointer user_data) static void test_secret_agent_nm_not_running (void) { - NMSecretAgentOld *agent; - GMainLoop *loop; + gs_unref_object NMSecretAgentOld *agent = NULL; + nm_auto_unref_gmainloop GMainLoop *loop = NULL; GError *error = NULL; - agent = g_initable_new (test_secret_agent_get_type (), NULL, &error, + agent = g_initable_new (test_secret_agent_get_type (), + NULL, + &error, NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", NULL); - g_assert_error (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_FAILED); - g_assert (agent == NULL); - g_clear_error (&error); + nmtst_assert_success (NM_IS_SECRET_AGENT_OLD (agent), error); + g_assert (!nm_secret_agent_old_get_registered (agent)); + g_clear_object (&agent); loop = g_main_loop_new (NULL, FALSE); g_async_initable_new_async (test_secret_agent_get_type (), G_PRIORITY_DEFAULT, - NULL, async_init_cb, loop, + NULL, + async_init_cb, + loop, NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent", NULL); g_main_loop_run (loop); - g_main_loop_unref (loop); } +/*****************************************************************************/ + +typedef struct { + int step; + int invoke_count; +} AutoRegisterData; + static void registered_changed (GObject *object, GParamSpec *pspec, gpointer user_data) { - GMainLoop *loop = user_data; + NMSecretAgentOld *agent = NM_SECRET_AGENT_OLD (object); + AutoRegisterData *data = user_data; - g_main_loop_quit (loop); + g_assert (data); + g_assert (NM_IS_SECRET_AGENT_OLD (agent)); + + data->invoke_count++; + g_assert_cmpint (data->invoke_count, ==, data->step); + + switch (data->step) { + case 1: + case 3: + g_assert (nm_secret_agent_old_get_registered (agent)); + break; + case 2: + case 4: + g_assert (!nm_secret_agent_old_get_registered (agent)); + break; + default: + g_assert_not_reached (); + } } static void test_secret_agent_auto_register (void) { NMTstcServiceInfo *sinfo; - NMSecretAgentOld *agent; - GMainLoop *loop; + gs_unref_object NMSecretAgentOld *agent = NULL; GError *error = NULL; + AutoRegisterData auto_register_data = { + .step = 0, + .invoke_count = 0, + }; + gulong signal_id; + NMTstContextBusyWatcherData watcher_data = { }; sinfo = nmtstc_service_init (); if (!nmtstc_service_available (sinfo)) return; - loop = g_main_loop_new (NULL, FALSE); - - agent = test_secret_agent_new (); - g_object_set (agent, - NM_SECRET_AGENT_OLD_AUTO_REGISTER, TRUE, - NULL); - g_signal_connect (agent, "notify::" NM_SECRET_AGENT_OLD_REGISTERED, - G_CALLBACK (registered_changed), loop); - + agent = test_secret_agent_new (FALSE); g_assert (!nm_secret_agent_old_get_registered (agent)); + + signal_id = g_signal_connect (agent, "notify::" NM_SECRET_AGENT_OLD_REGISTERED, + G_CALLBACK (registered_changed), &auto_register_data); + + if (nmtst_get_rand_bool ()) { + g_object_set (agent, + NM_SECRET_AGENT_OLD_AUTO_REGISTER, TRUE, + NULL); + } else + nm_secret_agent_old_enable (agent, TRUE); + g_assert (!nm_secret_agent_old_get_registered (agent)); + nm_secret_agent_old_register (agent, NULL, &error); g_assert_no_error (error); - g_assert (nm_secret_agent_old_get_registered (agent)); - - /* The GLib ObjectManager doesn't like when we drop the service - * in between it sees the service disappear and the call to - * GetManagedObjects. Give it a chance to do its business. - * Arguably a bug. */ - g_main_context_iteration (NULL, FALSE); - - /* Shut down test service */ - nmtstc_service_cleanup (sinfo); - g_main_loop_run (loop); g_assert (!nm_secret_agent_old_get_registered (agent)); - /* Restart test service */ + auto_register_data.step = 1; + nmtst_main_context_iterate_until_assert (NULL, + 1000, + nm_secret_agent_old_get_registered (agent)); + + auto_register_data.step = 2; + nm_secret_agent_old_enable (agent, FALSE); + g_assert (!nm_secret_agent_old_get_registered (agent)); + + nmtst_main_context_iterate_until (NULL, + nmtst_get_rand_uint32 () % 200, + FALSE); + + g_assert (!nm_secret_agent_old_get_registered (agent)); + + nmtstc_service_cleanup (sinfo); + + g_assert (!nm_secret_agent_old_get_registered (agent)); + + nm_secret_agent_old_enable (agent, TRUE); + + g_assert (!nm_secret_agent_old_get_registered (agent)); + + nmtst_main_context_iterate_until (NULL, + nmtst_get_rand_uint32 () % 200, + FALSE); + + g_assert (!nm_secret_agent_old_get_registered (agent)); + sinfo = nmtstc_service_init (); g_assert (nmtstc_service_available (sinfo)); - g_main_loop_run (loop); - g_assert (nm_secret_agent_old_get_registered (agent)); + auto_register_data.step = 3; + nmtst_main_context_iterate_until_assert (NULL, + 1000, + nm_secret_agent_old_get_registered (agent)); - /* Let ObjectManager initialize (see above). */ - g_main_context_iteration (NULL, FALSE); - - /* Shut down test service again */ nmtstc_service_cleanup (sinfo); - g_main_loop_run (loop); - g_assert (!nm_secret_agent_old_get_registered (agent)); - g_object_unref (agent); - g_main_loop_unref (loop); + auto_register_data.step = 4; + nmtst_main_context_iterate_until_assert (NULL, + 1000, + !nm_secret_agent_old_get_registered (agent)); + + nm_clear_g_signal_handler (agent, &signal_id); + + nmtst_context_busy_watcher_add (&watcher_data, nm_secret_agent_old_get_context_busy_watcher (agent)); + + g_clear_object (&agent); + + nmtst_context_busy_watcher_wait (&watcher_data); + + nmtst_main_context_assert_no_dispatch (NULL, nmtst_get_rand_uint32 () % 500); } /*****************************************************************************/ @@ -656,10 +729,10 @@ main (int argc, char **argv) nmtst_init (&argc, &argv, TRUE); - g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, NULL, test_setup, test_secret_agent_none, test_cleanup); - g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "sync", test_setup, test_secret_agent_no_secrets, test_cleanup); - g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "async", test_setup, test_secret_agent_cancel, test_cleanup); - g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "async", test_setup, test_secret_agent_good, test_cleanup); + g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, "0", test_setup, test_secret_agent_none, test_cleanup); + g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "1", test_setup, test_secret_agent_no_secrets, test_cleanup); + g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "1", test_setup, test_secret_agent_cancel, test_cleanup); + g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "1", test_setup, test_secret_agent_good, test_cleanup); g_test_add_func ("/libnm/secret-agent/nm-not-running", test_secret_agent_nm_not_running); g_test_add_func ("/libnm/secret-agent/auto-register", test_secret_agent_auto_register); diff --git a/po/POTFILES.in b/po/POTFILES.in index 9379fae271..852901d8ab 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -134,6 +134,7 @@ libnm/nm-device-wpan.c libnm/nm-device.c libnm/nm-object.c libnm/nm-remote-connection.c +libnm/nm-secret-agent-old.c libnm/nm-vpn-plugin-old.c libnm/nm-vpn-service-plugin.c data/org.freedesktop.NetworkManager.policy.in.in