From 452f14216a57671cad6e2b0cc7077865e7ffa4db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 16:56:58 +0100 Subject: [PATCH 1/8] shared: add nm_dbus_error_is() helper (cherry picked from commit ce36494c0a483b51275b9818d58e1ad150e4efbc) --- shared/nm-glib-aux/nm-dbus-aux.c | 23 +++++++++++++++++++++++ shared/nm-glib-aux/nm-dbus-aux.h | 13 +++++++++++++ 2 files changed, 36 insertions(+) diff --git a/shared/nm-glib-aux/nm-dbus-aux.c b/shared/nm-glib-aux/nm-dbus-aux.c index f55958b489..75b282bc14 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.c +++ b/shared/nm-glib-aux/nm-dbus-aux.c @@ -341,3 +341,26 @@ nm_dbus_connection_call_finish_variant_strip_dbus_error_cb (GObject *source, { _call_finish_cb (source, result, user_data, FALSE, TRUE); } + +/*****************************************************************************/ + +gboolean +_nm_dbus_error_is (GError *error, ...) +{ + gs_free char *dbus_error = NULL; + const char *name; + va_list ap; + + dbus_error = g_dbus_error_get_remote_error (error); + if (!dbus_error) + return FALSE; + + va_start (ap, error); + while ((name = va_arg (ap, const char *))) { + if (nm_streq (dbus_error, name)) + return TRUE; + } + va_end (ap); + + return FALSE; +} diff --git a/shared/nm-glib-aux/nm-dbus-aux.h b/shared/nm-glib-aux/nm-dbus-aux.h index bf7731d22f..840e23c275 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.h +++ b/shared/nm-glib-aux/nm-dbus-aux.h @@ -192,4 +192,17 @@ void nm_dbus_connection_call_finish_variant_strip_dbus_error_cb (GObject *source /*****************************************************************************/ +gboolean _nm_dbus_error_is (GError *error, ...) G_GNUC_NULL_TERMINATED; + +#define nm_dbus_error_is(error, ...) \ + ({ \ + GError *const _error = (error); \ + \ + _error && _nm_dbus_error_is (_error, __VA_ARGS__, NULL); \ + }) + +#define NM_DBUS_ERROR_NAME_UNKNOWN_METHOD "org.freedesktop.DBus.Error.UnknownMethod" + +/*****************************************************************************/ + #endif /* __NM_DBUS_AUX_H__ */ From e20c8d8ad5d9803a63fbc76db836b0f785401e6a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 14:02:48 +0100 Subject: [PATCH 2/8] shared: implement _LOGx() macros using log levels that are themself defines "nm-glib-aux/nm-logging-fwd.h" provides macros like _LOGD() to be reused by various parts which implement logging (by defining _NMLOG() accordingly). libnm also has logging, however it uses different logging levels aside LOGD_DEBUG. Instead, implement _LOGD() using a define _LOGL_DEBUG, so that libnm can redefine thos _LOGL_DEBUG defines and use the _LOGD() macro. (cherry picked from commit 1b00fd2fd202e228200a98c76ac455a0ddb87c81) --- shared/nm-glib-aux/nm-logging-fwd.h | 132 +++++++++++++++------------- 1 file changed, 69 insertions(+), 63 deletions(-) diff --git a/shared/nm-glib-aux/nm-logging-fwd.h b/shared/nm-glib-aux/nm-logging-fwd.h index a5783a6ff1..4766178835 100644 --- a/shared/nm-glib-aux/nm-logging-fwd.h +++ b/shared/nm-glib-aux/nm-logging-fwd.h @@ -137,39 +137,45 @@ extern void _nm_utils_monotonic_timestamp_initialized (const struct timespec *tp /*****************************************************************************/ +#define _LOGL_TRACE LOGL_TRACE +#define _LOGL_DEBUG LOGL_DEBUG +#define _LOGL_INFO LOGL_INFO +#define _LOGL_WARN LOGL_WARN +#define _LOGL_ERR LOGL_ERR + /* This is the default definition of _NMLOG_ENABLED(). Special implementations * might want to undef this and redefine it. */ #define _NMLOG_ENABLED(level) ( nm_logging_enabled ((level), (_NMLOG_DOMAIN)) ) -#define _LOGT(...) _NMLOG (LOGL_TRACE, __VA_ARGS__) -#define _LOGD(...) _NMLOG (LOGL_DEBUG, __VA_ARGS__) -#define _LOGI(...) _NMLOG (LOGL_INFO , __VA_ARGS__) -#define _LOGW(...) _NMLOG (LOGL_WARN , __VA_ARGS__) -#define _LOGE(...) _NMLOG (LOGL_ERR , __VA_ARGS__) +#define _LOGT(...) _NMLOG (_LOGL_TRACE, __VA_ARGS__) +#define _LOGD(...) _NMLOG (_LOGL_DEBUG, __VA_ARGS__) +#define _LOGI(...) _NMLOG (_LOGL_INFO , __VA_ARGS__) +#define _LOGW(...) _NMLOG (_LOGL_WARN , __VA_ARGS__) +#define _LOGE(...) _NMLOG (_LOGL_ERR , __VA_ARGS__) -#define _LOGT_ENABLED(...) _NMLOG_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOGD_ENABLED(...) _NMLOG_ENABLED (LOGL_DEBUG, ##__VA_ARGS__) -#define _LOGI_ENABLED(...) _NMLOG_ENABLED (LOGL_INFO , ##__VA_ARGS__) -#define _LOGW_ENABLED(...) _NMLOG_ENABLED (LOGL_WARN , ##__VA_ARGS__) -#define _LOGE_ENABLED(...) _NMLOG_ENABLED (LOGL_ERR , ##__VA_ARGS__) +#define _LOGT_ENABLED(...) _NMLOG_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOGD_ENABLED(...) _NMLOG_ENABLED (_LOGL_DEBUG, ##__VA_ARGS__) +#define _LOGI_ENABLED(...) _NMLOG_ENABLED (_LOGL_INFO , ##__VA_ARGS__) +#define _LOGW_ENABLED(...) _NMLOG_ENABLED (_LOGL_WARN , ##__VA_ARGS__) +#define _LOGE_ENABLED(...) _NMLOG_ENABLED (_LOGL_ERR , ##__VA_ARGS__) -#define _LOGT_err(errsv, ...) _NMLOG_err (errsv, LOGL_TRACE, __VA_ARGS__) -#define _LOGD_err(errsv, ...) _NMLOG_err (errsv, LOGL_DEBUG, __VA_ARGS__) -#define _LOGI_err(errsv, ...) _NMLOG_err (errsv, LOGL_INFO , __VA_ARGS__) -#define _LOGW_err(errsv, ...) _NMLOG_err (errsv, LOGL_WARN , __VA_ARGS__) -#define _LOGE_err(errsv, ...) _NMLOG_err (errsv, LOGL_ERR , __VA_ARGS__) +#define _LOGT_err(errsv, ...) _NMLOG_err (errsv, _LOGL_TRACE, __VA_ARGS__) +#define _LOGD_err(errsv, ...) _NMLOG_err (errsv, _LOGL_DEBUG, __VA_ARGS__) +#define _LOGI_err(errsv, ...) _NMLOG_err (errsv, _LOGL_INFO , __VA_ARGS__) +#define _LOGW_err(errsv, ...) _NMLOG_err (errsv, _LOGL_WARN , __VA_ARGS__) +#define _LOGE_err(errsv, ...) _NMLOG_err (errsv, _LOGL_ERR , __VA_ARGS__) /* _LOGT() and _LOGt() both log with level TRACE, but the latter is disabled by default, * unless building with --with-more-logging. */ #if NM_MORE_LOGGING -#define _LOGt_ENABLED(...) _NMLOG_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOGt(...) _NMLOG (LOGL_TRACE, __VA_ARGS__) -#define _LOGt_err(errsv, ...) _NMLOG_err (errsv, LOGL_TRACE, __VA_ARGS__) +#define _LOGt_ENABLED(...) _NMLOG_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOGt(...) _NMLOG (_LOGL_TRACE, __VA_ARGS__) +#define _LOGt_err(errsv, ...) _NMLOG_err (errsv, _LOGL_TRACE, __VA_ARGS__) #else /* still call the logging macros to get compile time checks, but they will be optimized out. */ -#define _LOGt_ENABLED(...) ( FALSE && (_NMLOG_ENABLED (LOGL_TRACE, ##__VA_ARGS__)) ) -#define _LOGt(...) G_STMT_START { if (FALSE) { _NMLOG (LOGL_TRACE, __VA_ARGS__); } } G_STMT_END -#define _LOGt_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG_err (errsv, LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOGt_ENABLED(...) ( FALSE && (_NMLOG_ENABLED (_LOGL_TRACE, ##__VA_ARGS__)) ) +#define _LOGt(...) G_STMT_START { if (FALSE) { _NMLOG (_LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOGt_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG_err (errsv, _LOGL_TRACE, __VA_ARGS__); } } G_STMT_END #endif /*****************************************************************************/ @@ -183,64 +189,64 @@ extern void _nm_utils_monotonic_timestamp_initialized (const struct timespec *tp #define _NMLOG2_ENABLED(level) ( nm_logging_enabled ((level), (_NMLOG2_DOMAIN)) ) -#define _LOG2T(...) _NMLOG2 (LOGL_TRACE, __VA_ARGS__) -#define _LOG2D(...) _NMLOG2 (LOGL_DEBUG, __VA_ARGS__) -#define _LOG2I(...) _NMLOG2 (LOGL_INFO , __VA_ARGS__) -#define _LOG2W(...) _NMLOG2 (LOGL_WARN , __VA_ARGS__) -#define _LOG2E(...) _NMLOG2 (LOGL_ERR , __VA_ARGS__) +#define _LOG2T(...) _NMLOG2 (_LOGL_TRACE, __VA_ARGS__) +#define _LOG2D(...) _NMLOG2 (_LOGL_DEBUG, __VA_ARGS__) +#define _LOG2I(...) _NMLOG2 (_LOGL_INFO , __VA_ARGS__) +#define _LOG2W(...) _NMLOG2 (_LOGL_WARN , __VA_ARGS__) +#define _LOG2E(...) _NMLOG2 (_LOGL_ERR , __VA_ARGS__) -#define _LOG2T_ENABLED(...) _NMLOG2_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOG2D_ENABLED(...) _NMLOG2_ENABLED (LOGL_DEBUG, ##__VA_ARGS__) -#define _LOG2I_ENABLED(...) _NMLOG2_ENABLED (LOGL_INFO , ##__VA_ARGS__) -#define _LOG2W_ENABLED(...) _NMLOG2_ENABLED (LOGL_WARN , ##__VA_ARGS__) -#define _LOG2E_ENABLED(...) _NMLOG2_ENABLED (LOGL_ERR , ##__VA_ARGS__) +#define _LOG2T_ENABLED(...) _NMLOG2_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOG2D_ENABLED(...) _NMLOG2_ENABLED (_LOGL_DEBUG, ##__VA_ARGS__) +#define _LOG2I_ENABLED(...) _NMLOG2_ENABLED (_LOGL_INFO , ##__VA_ARGS__) +#define _LOG2W_ENABLED(...) _NMLOG2_ENABLED (_LOGL_WARN , ##__VA_ARGS__) +#define _LOG2E_ENABLED(...) _NMLOG2_ENABLED (_LOGL_ERR , ##__VA_ARGS__) -#define _LOG2T_err(errsv, ...) _NMLOG2_err (errsv, LOGL_TRACE, __VA_ARGS__) -#define _LOG2D_err(errsv, ...) _NMLOG2_err (errsv, LOGL_DEBUG, __VA_ARGS__) -#define _LOG2I_err(errsv, ...) _NMLOG2_err (errsv, LOGL_INFO , __VA_ARGS__) -#define _LOG2W_err(errsv, ...) _NMLOG2_err (errsv, LOGL_WARN , __VA_ARGS__) -#define _LOG2E_err(errsv, ...) _NMLOG2_err (errsv, LOGL_ERR , __VA_ARGS__) +#define _LOG2T_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_TRACE, __VA_ARGS__) +#define _LOG2D_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_DEBUG, __VA_ARGS__) +#define _LOG2I_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_INFO , __VA_ARGS__) +#define _LOG2W_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_WARN , __VA_ARGS__) +#define _LOG2E_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_ERR , __VA_ARGS__) #if NM_MORE_LOGGING -#define _LOG2t_ENABLED(...) _NMLOG2_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOG2t(...) _NMLOG2 (LOGL_TRACE, __VA_ARGS__) -#define _LOG2t_err(errsv, ...) _NMLOG2_err (errsv, LOGL_TRACE, __VA_ARGS__) +#define _LOG2t_ENABLED(...) _NMLOG2_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOG2t(...) _NMLOG2 (_LOGL_TRACE, __VA_ARGS__) +#define _LOG2t_err(errsv, ...) _NMLOG2_err (errsv, _LOGL_TRACE, __VA_ARGS__) #else /* still call the logging macros to get compile time checks, but they will be optimized out. */ -#define _LOG2t_ENABLED(...) ( FALSE && (_NMLOG2_ENABLED (LOGL_TRACE, ##__VA_ARGS__)) ) -#define _LOG2t(...) G_STMT_START { if (FALSE) { _NMLOG2 (LOGL_TRACE, __VA_ARGS__); } } G_STMT_END -#define _LOG2t_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG2_err (errsv, LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOG2t_ENABLED(...) ( FALSE && (_NMLOG2_ENABLED (_LOGL_TRACE, ##__VA_ARGS__)) ) +#define _LOG2t(...) G_STMT_START { if (FALSE) { _NMLOG2 (_LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOG2t_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG2_err (errsv, _LOGL_TRACE, __VA_ARGS__); } } G_STMT_END #endif #define _NMLOG3_ENABLED(level) ( nm_logging_enabled ((level), (_NMLOG3_DOMAIN)) ) -#define _LOG3T(...) _NMLOG3 (LOGL_TRACE, __VA_ARGS__) -#define _LOG3D(...) _NMLOG3 (LOGL_DEBUG, __VA_ARGS__) -#define _LOG3I(...) _NMLOG3 (LOGL_INFO , __VA_ARGS__) -#define _LOG3W(...) _NMLOG3 (LOGL_WARN , __VA_ARGS__) -#define _LOG3E(...) _NMLOG3 (LOGL_ERR , __VA_ARGS__) +#define _LOG3T(...) _NMLOG3 (_LOGL_TRACE, __VA_ARGS__) +#define _LOG3D(...) _NMLOG3 (_LOGL_DEBUG, __VA_ARGS__) +#define _LOG3I(...) _NMLOG3 (_LOGL_INFO , __VA_ARGS__) +#define _LOG3W(...) _NMLOG3 (_LOGL_WARN , __VA_ARGS__) +#define _LOG3E(...) _NMLOG3 (_LOGL_ERR , __VA_ARGS__) -#define _LOG3T_ENABLED(...) _NMLOG3_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOG3D_ENABLED(...) _NMLOG3_ENABLED (LOGL_DEBUG, ##__VA_ARGS__) -#define _LOG3I_ENABLED(...) _NMLOG3_ENABLED (LOGL_INFO , ##__VA_ARGS__) -#define _LOG3W_ENABLED(...) _NMLOG3_ENABLED (LOGL_WARN , ##__VA_ARGS__) -#define _LOG3E_ENABLED(...) _NMLOG3_ENABLED (LOGL_ERR , ##__VA_ARGS__) +#define _LOG3T_ENABLED(...) _NMLOG3_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOG3D_ENABLED(...) _NMLOG3_ENABLED (_LOGL_DEBUG, ##__VA_ARGS__) +#define _LOG3I_ENABLED(...) _NMLOG3_ENABLED (_LOGL_INFO , ##__VA_ARGS__) +#define _LOG3W_ENABLED(...) _NMLOG3_ENABLED (_LOGL_WARN , ##__VA_ARGS__) +#define _LOG3E_ENABLED(...) _NMLOG3_ENABLED (_LOGL_ERR , ##__VA_ARGS__) -#define _LOG3T_err(errsv, ...) _NMLOG3_err (errsv, LOGL_TRACE, __VA_ARGS__) -#define _LOG3D_err(errsv, ...) _NMLOG3_err (errsv, LOGL_DEBUG, __VA_ARGS__) -#define _LOG3I_err(errsv, ...) _NMLOG3_err (errsv, LOGL_INFO , __VA_ARGS__) -#define _LOG3W_err(errsv, ...) _NMLOG3_err (errsv, LOGL_WARN , __VA_ARGS__) -#define _LOG3E_err(errsv, ...) _NMLOG3_err (errsv, LOGL_ERR , __VA_ARGS__) +#define _LOG3T_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_TRACE, __VA_ARGS__) +#define _LOG3D_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_DEBUG, __VA_ARGS__) +#define _LOG3I_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_INFO , __VA_ARGS__) +#define _LOG3W_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_WARN , __VA_ARGS__) +#define _LOG3E_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_ERR , __VA_ARGS__) #if NM_MORE_LOGGING -#define _LOG3t_ENABLED(...) _NMLOG3_ENABLED (LOGL_TRACE, ##__VA_ARGS__) -#define _LOG3t(...) _NMLOG3 (LOGL_TRACE, __VA_ARGS__) -#define _LOG3t_err(errsv, ...) _NMLOG3_err (errsv, LOGL_TRACE, __VA_ARGS__) +#define _LOG3t_ENABLED(...) _NMLOG3_ENABLED (_LOGL_TRACE, ##__VA_ARGS__) +#define _LOG3t(...) _NMLOG3 (_LOGL_TRACE, __VA_ARGS__) +#define _LOG3t_err(errsv, ...) _NMLOG3_err (errsv, _LOGL_TRACE, __VA_ARGS__) #else /* still call the logging macros to get compile time checks, but they will be optimized out. */ -#define _LOG3t_ENABLED(...) ( FALSE && (_NMLOG3_ENABLED (LOGL_TRACE, ##__VA_ARGS__)) ) -#define _LOG3t(...) G_STMT_START { if (FALSE) { _NMLOG3 (LOGL_TRACE, __VA_ARGS__); } } G_STMT_END -#define _LOG3t_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG3_err (errsv, LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOG3t_ENABLED(...) ( FALSE && (_NMLOG3_ENABLED (_LOGL_TRACE, ##__VA_ARGS__)) ) +#define _LOG3t(...) G_STMT_START { if (FALSE) { _NMLOG3 (_LOGL_TRACE, __VA_ARGS__); } } G_STMT_END +#define _LOG3t_err(errsv, ...) G_STMT_START { if (FALSE) { _NMLOG3_err (errsv, _LOGL_TRACE, __VA_ARGS__); } } G_STMT_END #endif /*****************************************************************************/ From 2b0533012ada3ee02f1a68498b3e9aca8c8d7755 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 14:05:47 +0100 Subject: [PATCH 3/8] libnm: allow using _LOGx() macros in libnm (cherry picked from commit 1b0f0f8c47e58f5c14778ba77f0ca43026c73d11) --- libnm/nm-libnm-utils.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libnm/nm-libnm-utils.h b/libnm/nm-libnm-utils.h index 98739db47c..8ac05d7076 100644 --- a/libnm/nm-libnm-utils.h +++ b/libnm/nm-libnm-utils.h @@ -8,6 +8,7 @@ #include "c-list/src/c-list.h" #include "nm-glib-aux/nm-ref-string.h" +#include "nm-glib-aux/nm-logging-fwd.h" #include "nm-types.h" #include "nm-object.h" #include "nm-client.h" @@ -66,6 +67,18 @@ typedef enum { | NML_DBUS_LOG_LEVEL_WARN, } NMLDBusLogLevel; +#undef _LOGL_TRACE +#undef _LOGL_DEBUG +#undef _LOGL_INFO +#undef _LOGL_WARN +#undef _LOGL_ERR + +#define _LOGL_TRACE NML_DBUS_LOG_LEVEL_TRACE +#define _LOGL_DEBUG NML_DBUS_LOG_LEVEL_DEBUG +#define _LOGL_INFO NML_DBUS_LOG_LEVEL_INFO +#define _LOGL_WARN NML_DBUS_LOG_LEVEL_WARN +#define _LOGL_ERR NML_DBUS_LOG_LEVEL_ERR + extern volatile int _nml_dbus_log_level; int _nml_dbus_log_level_init (void); From c414173fa95c5be55b63b544caa49f59ceb43387 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 12:48:17 +0100 Subject: [PATCH 4/8] libnm/secret-agent: drop fallback to Register() method for secret-agent RegisterWithCapabilities() is supported since NetworkManager 0.9.9.1. Of course, we don't support such old server anymore (also, because we require the standard D-Bus interfaces like ObjectManager). (cherry picked from commit 263aa63caa7dd238026a83db4dc6b7e95693046e) --- libnm/nm-secret-agent-old.c | 97 +++++++++++-------------------------- 1 file changed, 29 insertions(+), 68 deletions(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index de9df7dc68..67623af39c 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -477,6 +477,7 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, { NMSecretAgentOldPrivate *priv; NMSecretAgentOldClass *class; + gboolean success; g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); @@ -506,97 +507,57 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, return FALSE; priv->registering = TRUE; - if (nmdbus_agent_manager_call_register_with_capabilities_sync (priv->manager_proxy, - priv->identifier, - priv->capabilities, - cancellable, NULL)) - goto success; - - /* Might be an old NetworkManager that doesn't support capabilities; - * fall back to old Register() method instead. - */ - if (nmdbus_agent_manager_call_register_sync (priv->manager_proxy, - priv->identifier, - cancellable, error)) - goto success; - - /* Failure */ + success = nmdbus_agent_manager_call_register_with_capabilities_sync (priv->manager_proxy, + priv->identifier, + priv->capabilities, + cancellable, + error); priv->registering = FALSE; - _internal_unregister (self); - return FALSE; -success: - priv->registering = FALSE; + if (!success) { + _internal_unregister (self); + return FALSE; + } + priv->registered = TRUE; _notify (self, PROP_REGISTERED); return TRUE; } -static void -reg_result (NMSecretAgentOld *self, GSimpleAsyncResult *simple, GError *error) -{ - NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - _nm_unused gs_unref_object GSimpleAsyncResult *simple_free = simple; - - priv->registering = FALSE; - - if (error) { - g_simple_async_result_take_error (simple, error); - g_simple_async_result_complete (simple); - - /* If registration failed we shouldn't expose ourselves on the bus */ - _internal_unregister (self); - } else { - priv->registered = TRUE; - _notify (self, PROP_REGISTERED); - - g_simple_async_result_set_op_res_gboolean (simple, TRUE); - g_simple_async_result_complete (simple); - } -} - -static void -reg_request_cb (GObject *proxy, - GAsyncResult *result, - gpointer user_data) -{ - GSimpleAsyncResult *simple = user_data; - NMSecretAgentOld *self; - GError *error = NULL; - - self = NM_SECRET_AGENT_OLD (g_async_result_get_source_object (G_ASYNC_RESULT (simple))); - g_object_unref (self); /* drop extra ref added by get_source_object() */ - - if (!nmdbus_agent_manager_call_register_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error)) - g_dbus_error_strip_remote_error (error); - reg_result (self, simple, error); -} - static void reg_with_caps_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) { - GSimpleAsyncResult *simple = user_data; + _nm_unused gs_unref_object GSimpleAsyncResult *simple = user_data; NMSecretAgentOld *self; NMSecretAgentOldPrivate *priv; + gs_free_error GError *error = NULL; self = NM_SECRET_AGENT_OLD (g_async_result_get_source_object (G_ASYNC_RESULT (simple))); g_object_unref (self); /* drop extra ref added by get_source_object() */ priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - if (nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, NULL)) { - reg_result (self, simple, NULL); + if (!nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error)) + g_dbus_error_strip_remote_error (error); + + priv->registering = FALSE; + + if (error) { + g_simple_async_result_take_error (simple, g_steal_pointer (&error)); + g_simple_async_result_complete (simple); + + /* If registration failed we shouldn't expose ourselves on the bus */ + _internal_unregister (self); return; } - /* Might be an old NetworkManager that doesn't support capabilities; - * fall back to old Register() method instead. - */ - nmdbus_agent_manager_call_register (priv->manager_proxy, - priv->identifier, - NULL, reg_request_cb, simple); + priv->registered = TRUE; + _notify (self, PROP_REGISTERED); + + g_simple_async_result_set_op_res_gboolean (simple, TRUE); + g_simple_async_result_complete (simple); } /** From eeb3a25ca22b55a411ad589c0eb2f1dcc85dd144 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 14:16:56 +0100 Subject: [PATCH 5/8] libnm/secret-agent: support debug logging from secret-agent (cherry picked from commit 392befb5fdc2bc3830258d0f1bf817ac1b874af9) --- libnm/nm-secret-agent-old.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 67623af39c..73e78febdb 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -64,6 +64,14 @@ G_DEFINE_ABSTRACT_TYPE_WITH_CODE (NMSecretAgentOld, nm_secret_agent_old, G_TYPE_ /*****************************************************************************/ +#define _NMLOG(level, ...) \ + NML_DBUS_LOG((level), \ + "secret-agent["NM_HASH_OBFUSCATE_PTR_FMT"]: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ + NM_HASH_OBFUSCATE_PTR (self) \ + _NM_UTILS_MACRO_REST (__VA_ARGS__)) + +/*****************************************************************************/ + static void _internal_unregister (NMSecretAgentOld *self) { @@ -111,6 +119,9 @@ name_owner_changed (GObject *proxy, 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); @@ -1073,6 +1084,8 @@ nm_secret_agent_old_init (NMSecretAgentOld *self) { NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + _LOGT ("create new instance"); + c_list_init (&priv->gsi_lst_head); priv->dbus_secret_agent = nmdbus_secret_agent_skeleton_new (); _nm_dbus_bind_properties (self, priv->dbus_secret_agent); @@ -1090,6 +1103,8 @@ 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"); + priv->bus = g_bus_get_sync (_nm_dbus_bus_type (), cancellable, error); if (!priv->bus) return FALSE; @@ -1117,9 +1132,12 @@ init_async (GAsyncInitable *initable, int io_priority, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data) { + NMSecretAgentOld *self = NM_SECRET_AGENT_OLD (initable); GTask *task; - task = g_task_new (initable, cancellable, callback, user_data); + _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 (), @@ -1135,6 +1153,8 @@ dispose (GObject *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); From 37f1c52fa94f43c8e17b2ada19fa364e600602a7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 16:06:18 +0100 Subject: [PATCH 6/8] libnm/secret-agent: use GTask for nm_secret_agent_old_register*() This change is of course right and read nicer. Also, the GTask captures the current g_main_context_get_thread_default(). We will need that next. (cherry picked from commit cff4e937ac48282c7d05a713132aa4e349f402b9) --- libnm/nm-secret-agent-old.c | 86 +++++++++++++------------------------ 1 file changed, 31 insertions(+), 55 deletions(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 73e78febdb..91de7716bf 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -540,35 +540,27 @@ reg_with_caps_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) { - _nm_unused gs_unref_object GSimpleAsyncResult *simple = user_data; - NMSecretAgentOld *self; - NMSecretAgentOldPrivate *priv; + 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; - self = NM_SECRET_AGENT_OLD (g_async_result_get_source_object (G_ASYNC_RESULT (simple))); - g_object_unref (self); /* drop extra ref added by get_source_object() */ - - priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); - if (!nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error)) g_dbus_error_strip_remote_error (error); priv->registering = FALSE; if (error) { - g_simple_async_result_take_error (simple, g_steal_pointer (&error)); - g_simple_async_result_complete (simple); - /* If registration failed we shouldn't expose ourselves on the bus */ _internal_unregister (self); + g_task_return_error (task, g_steal_pointer (&error)); return; } priv->registered = TRUE; _notify (self, PROP_REGISTERED); - g_simple_async_result_set_op_res_gboolean (simple, TRUE); - g_simple_async_result_complete (simple); + g_task_return_boolean (task, TRUE); } /** @@ -593,8 +585,8 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, { NMSecretAgentOldPrivate *priv; NMSecretAgentOldClass *class; - gs_unref_object GSimpleAsyncResult *simple = NULL; - GError *error = NULL; + gs_unref_object GTask *task = NULL; + gs_free_error GError *error = NULL; g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); @@ -611,14 +603,10 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, g_return_if_fail (class->save_secrets != NULL); g_return_if_fail (class->delete_secrets != NULL); - simple = g_simple_async_result_new (G_OBJECT (self), callback, user_data, - nm_secret_agent_old_register_async); - if (cancellable) - g_simple_async_result_set_check_cancellable (simple, cancellable); + task = nm_g_task_new (self, cancellable, nm_secret_agent_old_register_async, callback, user_data); if (!check_nm_running (self, &error)) { - g_simple_async_result_take_error (simple, error); - g_simple_async_result_complete_in_idle (simple); + g_task_return_error (task, g_steal_pointer (&error)); return; } @@ -627,8 +615,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, priv->bus, NM_DBUS_PATH_SECRET_AGENT, &error)) { - g_simple_async_result_take_error (simple, error); - g_simple_async_result_complete_in_idle (simple); + g_task_return_error (task, g_steal_pointer (&error)); return; } @@ -640,7 +627,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, priv->capabilities, NULL, reg_with_caps_cb, - g_steal_pointer (&simple)); + g_steal_pointer (&task)); } /** @@ -658,12 +645,10 @@ nm_secret_agent_old_register_finish (NMSecretAgentOld *self, GAsyncResult *result, GError **error) { - g_return_val_if_fail (g_simple_async_result_is_valid (result, G_OBJECT (self), nm_secret_agent_old_register_async), FALSE); + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); + g_return_val_if_fail (nm_g_task_is_valid (result, self, nm_secret_agent_old_register_async), FALSE); - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (result), error)) - return FALSE; - else - return TRUE; + return g_task_propagate_boolean (G_TASK (result), error); } /** @@ -706,24 +691,21 @@ nm_secret_agent_old_unregister (NMSecretAgentOld *self, static void unregister_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) { - gs_unref_object GSimpleAsyncResult *simple = user_data; - NMSecretAgentOld *self; - GError *error = NULL; - - self = NM_SECRET_AGENT_OLD (g_async_result_get_source_object (G_ASYNC_RESULT (simple))); - g_object_unref (self); /* drop extra ref added by get_source_object() */ + 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_simple_async_result_set_op_res_gboolean (simple, TRUE); - else { + if (!nmdbus_agent_manager_call_unregister_finish (NMDBUS_AGENT_MANAGER (proxy), + result, + &error)) { g_dbus_error_strip_remote_error (error); - g_simple_async_result_take_error (simple, error); + g_task_return_error (task, g_steal_pointer (&error)); + return; } - g_simple_async_result_complete (simple); + g_task_return_boolean (task, TRUE); } /** @@ -744,8 +726,8 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, gpointer user_data) { NMSecretAgentOldPrivate *priv; - gs_unref_object GSimpleAsyncResult *simple = NULL; - GError *error = NULL; + gs_unref_object GTask *task = NULL; + gs_free_error GError *error = NULL; g_return_if_fail (NM_IS_SECRET_AGENT_OLD (self)); @@ -754,14 +736,10 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, g_return_if_fail (priv->bus != NULL); g_return_if_fail (priv->manager_proxy != NULL); - simple = g_simple_async_result_new (G_OBJECT (self), callback, user_data, - nm_secret_agent_old_unregister_async); - if (cancellable) - g_simple_async_result_set_check_cancellable (simple, cancellable); + task = nm_g_task_new (self, cancellable, nm_secret_agent_old_unregister_async, callback, user_data); if (!check_nm_running (self, &error)) { - g_simple_async_result_take_error (simple, error); - g_simple_async_result_complete_in_idle (simple); + g_task_return_error (task, g_steal_pointer (&error)); return; } @@ -770,7 +748,7 @@ nm_secret_agent_old_unregister_async (NMSecretAgentOld *self, nmdbus_agent_manager_call_unregister (priv->manager_proxy, cancellable, unregister_cb, - g_steal_pointer (&simple)); + g_steal_pointer (&task)); } /** @@ -788,12 +766,10 @@ nm_secret_agent_old_unregister_finish (NMSecretAgentOld *self, GAsyncResult *result, GError **error) { - g_return_val_if_fail (g_simple_async_result_is_valid (result, G_OBJECT (self), nm_secret_agent_old_unregister_async), FALSE); + g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), FALSE); + g_return_val_if_fail (nm_g_task_is_valid (result, self, nm_secret_agent_old_unregister_async), FALSE); - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (result), error)) - return FALSE; - else - return TRUE; + return g_task_propagate_boolean (G_TASK (result), error); } /** From 494cbe144df69e6a30e99f81437057fb1e3411b0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 16:57:01 +0100 Subject: [PATCH 7/8] libnm/secret-agent: fix race registering secret agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When NetworkManager starts, NMSecretAgentOld gets a name-owner changed signal and registers right away. Especially since commit ce0e898fb476 ('libnm: refactor caching of D-Bus objects in NMClient') this hits a race where NetworkManager does not yet export the org.freedesktop.NetworkManager.AgentManager interface and the registration fails: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager Previously, when NMClient recevied a name-owner changed, that would block the main loop long enough to avoid the race. Note that NMClient has nothing to do with NMSecretAgentOld, however in practice all applications that use NMSecretAgentOld also use NMClient. While we should fix the race server-side, we also need to work around it in the client. Retry. Also, make the async request actually cancellable and actually honor the passed GCancellable. Check output: $ LIBNM_CLIENT_DEBUG=trace ./clients/cli/nmcli agent secret |& grep secret-agent libnm-dbus: [21399.04862] secret-agent[2f2af4ee102d7570]: create new instance libnm-dbus: [21399.04863] secret-agent[2f2af4ee102d7570]: init-sync libnm-dbus: [21404.08147] secret-agent[2f2af4ee102d7570]: name owner changed: (null) libnm-dbus: [21404.09085] secret-agent[2f2af4ee102d7570]: name owner changed: ":1.2504" libnm-dbus: [21404.09085] secret-agent[2f2af4ee102d7570]: register: starting asynchronous registration... libnm-dbus: [21404.09178] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 0 msec... libnm-dbus: [21404.09178] secret-agent[2f2af4ee102d7570]: register: retry registration... libnm-dbus: [21404.09195] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 4 msec... libnm-dbus: [21404.09236] secret-agent[2f2af4ee102d7570]: register: retry registration... [...] libnm-dbus: [21405.01782] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec... libnm-dbus: [21405.03063] secret-agent[2f2af4ee102d7570]: register: retry registration... libnm-dbus: [21405.03068] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec... libnm-dbus: [21405.04354] secret-agent[2f2af4ee102d7570]: register: retry registration... libnm-dbus: [21406.01097] secret-agent[2f2af4ee102d7570]: register: registration succeeded (cherry picked from commit f0d3243f2ba973bc83687c28d8e7fab4b0104650) --- libnm/nm-secret-agent-old.c | 225 +++++++++++++++++++++++++++++++----- 1 file changed, 193 insertions(+), 32 deletions(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 91de7716bf..565fdfbdc2 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -7,16 +7,20 @@ #include "nm-secret-agent-old.h" +#include "c-list/src/c-list.h" +#include "nm-core-internal.h" +#include "nm-dbus-helpers.h" #include "nm-dbus-interface.h" #include "nm-enum-types.h" -#include "nm-dbus-helpers.h" +#include "nm-glib-aux/nm-dbus-aux.h" +#include "nm-glib-aux/nm-time-utils.h" #include "nm-simple-connection.h" -#include "nm-core-internal.h" -#include "c-list/src/c-list.h" #include "introspection/org.freedesktop.NetworkManager.SecretAgent.h" #include "introspection/org.freedesktop.NetworkManager.AgentManager.h" +#define REGISTER_RETRY_TIMEOUT_MSEC 2000 + /*****************************************************************************/ typedef struct { @@ -45,8 +49,10 @@ typedef struct { NMSecretAgentCapabilities capabilities; + gint64 registering_timeout_msec; + guint registering_try_count; + bool registered:1; - bool registering:1; bool session_bus:1; bool auto_register:1; bool suppress_auto:1; @@ -72,6 +78,12 @@ 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 void _internal_unregister (NMSecretAgentOld *self) { @@ -80,7 +92,7 @@ _internal_unregister (NMSecretAgentOld *self) if (priv->registered) { g_dbus_interface_skeleton_unexport (G_DBUS_INTERFACE_SKELETON (priv->dbus_secret_agent)); priv->registered = FALSE; - priv->registering = FALSE; + priv->registering_timeout_msec = 0; _notify (self, PROP_REGISTERED); } } @@ -105,7 +117,7 @@ should_auto_register (NMSecretAgentOld *self) return ( priv->auto_register && !priv->suppress_auto && !priv->registered - && !priv->registering); + && priv->registering_timeout_msec == 0); } static void @@ -466,6 +478,23 @@ check_nm_running (NMSecretAgentOld *self, GError **error) /*****************************************************************************/ +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_ms () < 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 @@ -488,14 +517,13 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, { NMSecretAgentOldPrivate *priv; NMSecretAgentOldClass *class; - gboolean success; g_return_val_if_fail (NM_IS_SECRET_AGENT_OLD (self), 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 == 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->manager_proxy != NULL, FALSE); @@ -517,46 +545,175 @@ nm_secret_agent_old_register (NMSecretAgentOld *self, error)) return FALSE; - priv->registering = TRUE; - success = nmdbus_agent_manager_call_register_with_capabilities_sync (priv->manager_proxy, - priv->identifier, - priv->capabilities, - cancellable, - error); - priv->registering = FALSE; + priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_ms () + REGISTER_RETRY_TIMEOUT_MSEC; + priv->registering_try_count = 0; - if (!success) { - _internal_unregister (self); - return FALSE; + while (TRUE) { + gs_free_error GError *local = NULL; + gs_free char *dbus_error = 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; } +} - priv->registered = TRUE; - _notify (self, PROP_REGISTERED); - return TRUE; +/*****************************************************************************/ + +typedef struct { + GCancellable *cancellable; + GSource *timeout_source; + gulong cancellable_signal_id; +} RegisterData; + +static void +_register_data_free (RegisterData *register_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); + NMSecretAgentOldPrivate *priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); + GCancellable *cancellable; + + _LOGT ("register: retry registration..."); + + g_task_set_task_data (task, NULL, NULL); + + cancellable = g_task_get_cancellable (task); + + 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 -reg_with_caps_cb (GObject *proxy, - GAsyncResult *result, - gpointer user_data) +_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..."); + + 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; - if (!nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error)) - g_dbus_error_strip_remote_error (error); + nmdbus_agent_manager_call_register_with_capabilities_finish (NMDBUS_AGENT_MANAGER (proxy), result, &error); - priv->registering = FALSE; + 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 = nm_utils_get_monotonic_timestamp_ms () + REGISTER_RETRY_TIMEOUT_MSEC; + priv->registering_try_count = 0; if (error) { - /* If registration failed we shouldn't expose ourselves on the bus */ + _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); @@ -593,7 +750,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, priv = NM_SECRET_AGENT_OLD_GET_PRIVATE (self); g_return_if_fail (priv->registered == FALSE); - g_return_if_fail (priv->registering == FALSE); + g_return_if_fail (priv->registering_timeout_msec == 0); g_return_if_fail (priv->bus != NULL); g_return_if_fail (priv->manager_proxy != NULL); @@ -606,6 +763,7 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, 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; } @@ -615,18 +773,21 @@ nm_secret_agent_old_register_async (NMSecretAgentOld *self, priv->bus, 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 = TRUE; + priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_ms () + 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, - NULL, - reg_with_caps_cb, + cancellable, + _register_call_cb, g_steal_pointer (&task)); } From 793e390c225017102333b188a69b4d60ed7ef884 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 16 Dec 2019 18:53:04 +0100 Subject: [PATCH 8/8] libnm/secret-agent: fix reseting timeout when registration completes Fixes: f0d3243f2ba9 ('libnm/secret-agent: fix race registering secret agent') (cherry picked from commit 1f9dabcb735b3b9f0511cf72d1ed23e540ca49ee) --- libnm/nm-secret-agent-old.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libnm/nm-secret-agent-old.c b/libnm/nm-secret-agent-old.c index 565fdfbdc2..c862106480 100644 --- a/libnm/nm-secret-agent-old.c +++ b/libnm/nm-secret-agent-old.c @@ -702,8 +702,7 @@ _register_call_cb (GObject *proxy, } done: - priv->registering_timeout_msec = nm_utils_get_monotonic_timestamp_ms () + REGISTER_RETRY_TIMEOUT_MSEC; - priv->registering_try_count = 0; + priv->registering_timeout_msec = 0; if (error) { _LOGT ("register: registration failed with error \"%s\"", error->message);