secret-agent: rework secret-agent to better handle service shutdown

The secret-agent D-Bus API knows 4 methods: GetSecrets, SaveSecrets,
DeleteSecrets and CancelGetSecrets. When we cancel a GetSecrets
request, we must issue another CancelGetSecrets to tell the agent
that the request was aborted. This is also true during shutdown.
Well, technically, during shutdown we anyway drop off the bus and
it woudn't matter. In practice, I think we should get this right and
always cancel properly.

To better handle shutdown change the following:

- each request now takes a reference on NMSecretAgent. That means,
  as long as there are pending requests, the instance stays alive.
  The way to get this right during shutdown, is that NMSecretAgent
  registers itself via nm_shutdown_wait_obj_register() and
  NetworkManager is supposed to keep running as long as requests
  are keeping the instance alive.

- now, the 3 regular methods are cancellable (which means: we are
  no longer interested in the result). CancelGetSecrets is not
  cancellable, but it has a short timeout NM_SHUTDOWN_TIMEOUT_MS
  to handle this. We anyway don't really care about the result,
  aside logging and to be sure that the request fully completed.

- this means, a request (NMSecretAgentCallId) can now immediately
  be cancelled and destroyed, both when the request returns and
  when the caller cancels it. The exception is GetSecrets which
  keeps the request alive while waiting for CancelGetSecrets. But
  this is easily handled by unlinking the call-id and pass it on
  to the CancelGetSecrets callback.
  Previously, the NMSecretAgentCallId was only destroyed when
  the D-Bus call returns, even if it was cancelled earlier. That's
  unnecessary complicated.

- previously, D-Bus requests SaveSecrets and DeleteSecrets were not cancellable.
  That is a problem. We need to be able to cancel them in order to shutdown in
  time.

- use GDBusConnection instead of GDBusProxy. As most of the time, GDBusProxy
  provides features we don't use.

- again, don't log direct pointer values, but obfuscate the indentifiers.

3 files changed, 351 insertions, 313 deletions

diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c
index 2f9827d54a..571ab721b6 100644
--- a/src/settings/nm-agent-manager.c
+++ b/src/settings/nm-agent-manager.c
@@ -93,13 +93,13 @@ NM_DEFINE_SINGLETON_GETTER (NMAgentManager, nm_agent_manager_get, NM_TYPE_AGENT_
             if (!(self)) \
                 g_snprintf (__prefix1, sizeof (__prefix1), "%s%s", ""_NMLOG_PREFIX_NAME"", "[]"); \
             else if ((self) != singleton_instance) \
-                g_snprintf (__prefix1, sizeof (__prefix1), "%s[%p]", ""_NMLOG_PREFIX_NAME"", (self)); \
+                g_snprintf (__prefix1, sizeof (__prefix1), "%s["NM_HASH_OBFUSCATE_PTR_FMT"]", ""_NMLOG_PREFIX_NAME"", NM_HASH_OBFUSCATE_PTR (self)); \
             else \
                 g_strlcpy (__prefix1, _NMLOG_PREFIX_NAME, sizeof (__prefix1)); \
             if (__agent) { \
                 g_snprintf (__prefix2, sizeof (__prefix2), \
-                            ": req[%p, %s]", \
-                            __agent, \
+                            ": agent["NM_HASH_OBFUSCATE_PTR_FMT",%s]", \
+                            NM_HASH_OBFUSCATE_PTR (__agent), \
                             nm_secret_agent_get_description (__agent)); \
             } else \
                 __prefix2[0] = '\0'; \
@@ -109,9 +109,9 @@ NM_DEFINE_SINGLETON_GETTER (NMAgentManager, nm_agent_manager_get, NM_TYPE_AGENT_
         } \
     } G_STMT_END
 
-#define LOG_REQ_FMT          "[%p/%s%s%s%s%s%s]"
+#define LOG_REQ_FMT          "["NM_HASH_OBFUSCATE_PTR_FMT"/%s%s%s%s%s%s]"
 #define LOG_REQ_ARG(req) \
-	(req), \
+	NM_HASH_OBFUSCATE_PTR (req), \
 	NM_PRINT_FMT_QUOTE_STRING ((req)->detail), \
 	NM_PRINT_FMT_QUOTED (((req)->request_type == REQUEST_TYPE_CON_GET) && (req)->con.get.setting_name, \
 	                     "/\"", (req)->con.get.setting_name, "\"", \
@@ -539,12 +539,10 @@ request_free (Request *req)
 	if (req->idle_id)
 		g_source_remove (req->idle_id);
 
-	if (req->current && req->current_call_id) {
-		/* cancel-secrets invokes the done-callback synchronously -- in which case
-		 * the handler just return.
-		 * Hence, we can proceed to free @req... */
-		nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
-	}
+	/* cancel-secrets invokes the done-callback synchronously -- in which case
+	 * the handler just return.
+	 * Hence, we can proceed to free @req... */
+	nm_secret_agent_cancel_call (req->current, req->current_call_id);
 
 	g_object_unref (req->subject);
 
@@ -742,12 +740,9 @@ request_next_agent (Request *req)
 
 	self = req->self;
 
-	if (req->current) {
-		if (req->current_call_id)
-			nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
-		g_clear_object (&req->current);
-	}
+	nm_secret_agent_cancel_call (req->current, req->current_call_id);
 	nm_assert (!req->current_call_id);
+	g_clear_object (&req->current);
 
 	if (req->pending) {
 		/* Send the request to the next agent */
@@ -882,10 +877,8 @@ _con_get_request_done (NMSecretAgent *agent,
 			req_complete_error (req, error);
 			g_error_free (error);
 		} else {
-			if (req->current_call_id) {
-				/* Tell the failed agent we're no longer interested. */
-				nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
-			}
+			/* Tell the failed agent we're no longer interested. */
+			nm_secret_agent_cancel_call (req->current, req->current_call_id);
 
 			/* Try the next agent */
 			request_next_agent (req);
diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c
index 141e385b1f..e6d4630a2c 100644
--- a/src/settings/nm-secret-agent.c
+++ b/src/settings/nm-secret-agent.c
@@ -35,11 +35,17 @@
 
 /*****************************************************************************/
 
+#define METHOD_GET_SECRETS        "GetSecrets"
+#define METHOD_CANCEL_GET_SECRETS "CancelGetSecrets"
+#define METHOD_SAVE_SECRETS       "SaveSecrets"
+#define METHOD_DELETE_SECRETS     "DeleteSecrets"
+
 enum {
 	DISCONNECTED,
 
 	LAST_SIGNAL
 };
+
 static guint signals[LAST_SIGNAL] = { 0 };
 
 typedef struct {
@@ -49,11 +55,12 @@ typedef struct {
 	char *identifier;
 	char *owner_username;
 	char *dbus_owner;
-	NMSecretAgentCapabilities capabilities;
-	GDBusProxy *proxy;
 	GDBusConnection *dbus_connection;
+	GCancellable *name_owner_cancellable;
 	CList requests;
-	guint on_disconnected_id;
+	NMSecretAgentCapabilities capabilities;
+	guint name_owner_changed_id;
+	bool shutdown_wait_obj_registered:1;
 } NMSecretAgentPrivate;
 
 struct _NMSecretAgent {
@@ -81,8 +88,7 @@ G_DEFINE_TYPE (NMSecretAgent, nm_secret_agent, G_TYPE_OBJECT)
             if ((self)) { \
                 g_snprintf (_prefix, \
                             sizeof (_prefix), \
-                            "%s["NM_HASH_OBFUSCATE_PTR_FMT"]", \
-                            ""_NMLOG_PREFIX_NAME"", \
+                            _NMLOG_PREFIX_NAME"["NM_HASH_OBFUSCATE_PTR_FMT"]", \
                             NM_HASH_OBFUSCATE_PTR (self)); \
             } else \
                 g_strlcpy (_prefix, _NMLOG_PREFIX_NAME, sizeof (_prefix)); \
@@ -98,17 +104,30 @@ G_DEFINE_TYPE (NMSecretAgent, nm_secret_agent, G_TYPE_OBJECT)
         } \
     } G_STMT_END
 
-#define LOG_REQ_FMT          "req["NM_HASH_OBFUSCATE_PTR_FMT",%s,%s%s%s%s]"
-#define LOG_REQ_ARG(req) \
-    NM_HASH_OBFUSCATE_PTR (req), \
-    (req)->dbus_command, \
-    NM_PRINT_FMT_QUOTE_STRING ((req)->path), \
-    ((req)->cancellable ? "" : " (cancelled)")
+#define _NMLOG2(level, call_id, ...) \
+    G_STMT_START { \
+        NMSecretAgentCallId *const _call_id = (call_id); \
+        \
+        nm_assert (_call_id); \
+        \
+        nm_log ((level), \
+                (_NMLOG_DOMAIN), \
+                NULL, \
+                NULL, \
+                "%s["NM_HASH_OBFUSCATE_PTR_FMT"] request ["NM_HASH_OBFUSCATE_PTR_FMT",%s,%s%s%s%s]: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \
+                _NMLOG_PREFIX_NAME, \
+                NM_HASH_OBFUSCATE_PTR (_call_id->self), \
+                NM_HASH_OBFUSCATE_PTR (_call_id), \
+                _call_id->method_name, \
+                NM_PRINT_FMT_QUOTE_STRING (_call_id->path), \
+                (_call_id->cancellable ? "" : " (cancelled)") \
+                _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
+    } G_STMT_END
 
 /*****************************************************************************/
 
 NM_UTILS_FLAGS2STR_DEFINE_STATIC (_capabilities_to_string, NMSecretAgentCapabilities,
-	NM_UTILS_FLAGS2STR (NM_SECRET_AGENT_CAPABILITY_NONE, "none"),
+	NM_UTILS_FLAGS2STR (NM_SECRET_AGENT_CAPABILITY_NONE,      "none"),
 	NM_UTILS_FLAGS2STR (NM_SECRET_AGENT_CAPABILITY_VPN_HINTS, "vpn-hints"),
 );
 
@@ -116,69 +135,100 @@ NM_UTILS_FLAGS2STR_DEFINE_STATIC (_capabilities_to_string, NMSecretAgentCapabili
 
 struct _NMSecretAgentCallId {
 	CList lst;
-	NMSecretAgent *agent;
+	NMSecretAgent *self;
 	GCancellable *cancellable;
 	char *path;
-	const char *dbus_command;
+	const char *method_name;
 	char *setting_name;
-	gboolean is_get_secrets;
 	NMSecretAgentCallback callback;
 	gpointer callback_data;
 };
 
 static NMSecretAgentCallId *
-request_new (NMSecretAgent *self,
-             const char *dbus_command, /* this must be a static string. */
-             const char *path,
-             const char *setting_name,
-             NMSecretAgentCallback callback,
-             gpointer callback_data)
+_call_id_new (NMSecretAgent *self,
+              const char *method_name, /* this must be a static string. */
+              const char *path,
+              const char *setting_name,
+              NMSecretAgentCallback callback,
+              gpointer callback_data)
 {
-	NMSecretAgentCallId *r;
-
-	r = g_slice_new0 (NMSecretAgentCallId);
-	r->agent = self;
-	r->path = g_strdup (path);
-	r->setting_name = g_strdup (setting_name);
-	r->dbus_command = dbus_command,
-	r->callback = callback;
-	r->callback_data = callback_data;
-	r->cancellable = g_cancellable_new ();
-	c_list_link_tail (&NM_SECRET_AGENT_GET_PRIVATE (self)->requests,
-	                  &r->lst);
-	_LOGT ("request "LOG_REQ_FMT": created", LOG_REQ_ARG (r));
-	return r;
+	NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
+	NMSecretAgentCallId *call_id;
+
+	call_id = g_slice_new (NMSecretAgentCallId);
+	*call_id = (NMSecretAgentCallId) {
+		.self              = g_object_ref (self),
+		.path              = g_strdup (path),
+		.setting_name      = g_strdup (setting_name),
+		.method_name       = method_name,
+		.callback          = callback,
+		.callback_data     = callback_data,
+		.cancellable       = g_cancellable_new (),
+	};
+	c_list_link_tail (&priv->requests, &call_id->lst);
+
+	_LOG2T (call_id, "new request...");
+
+	if (!priv->shutdown_wait_obj_registered) {
+		/* self has async requests (that keep self alive). As long as
+		 * we have pending requests, shutdown is blocked. */
+		priv->shutdown_wait_obj_registered = TRUE;
+		nm_shutdown_wait_obj_register (G_OBJECT (self), "secret-agent");
+	}
+
+	return call_id;
 }
-#define request_new(self,dbus_command,path,setting_name,callback,callback_data) request_new(self,""dbus_command"",path,setting_name,callback,callback_data)
+
+#define _call_id_new(self, method_name, path, setting_name, callback, callback_data) _call_id_new(self, ""method_name"", path, setting_name, callback, callback_data)
 
 static void
-request_free (NMSecretAgentCallId *r)
+_call_id_free (NMSecretAgentCallId *call_id)
 {
-	NMSecretAgent *self = r->agent;
-
-	_LOGT ("request "LOG_REQ_FMT": destroyed", LOG_REQ_ARG (r));
-	c_list_unlink_stale (&r->lst);
-	g_free (r->path);
-	g_free (r->setting_name);
-	if (r->cancellable)
-		g_object_unref (r->cancellable);
-	g_slice_free (NMSecretAgentCallId, r);
+	c_list_unlink_stale (&call_id->lst);
+	g_free (call_id->path);
+	g_free (call_id->setting_name);
+	nm_g_object_unref (call_id->cancellable);
+	g_object_unref (call_id->self);
+	nm_g_slice_free (call_id);
 }
 
-static gboolean
-request_check_return (NMSecretAgentCallId *r)
+static void
+_call_id_invoke_callback (NMSecretAgentCallId *call_id,
+                          GVariant *secrets,
+                          GError *error,
+                          gboolean cancelled,
+                          gboolean free_call_id)
 {
-	if (!r->cancellable)
-		return FALSE;
+	gs_free_error GError *error_cancelled = NULL;
+
+	nm_assert (call_id);
+	nm_assert (!c_list_is_empty (&call_id->lst));
 
-	g_return_val_if_fail (NM_IS_SECRET_AGENT (r->agent), FALSE);
+	c_list_unlink (&call_id->lst);
 
-	nm_assert (c_list_contains (&NM_SECRET_AGENT_GET_PRIVATE (r->agent)->requests,
-	                            &r->lst));
+	if (cancelled) {
+		nm_assert (!secrets);
+		nm_assert (!error);
+		if (call_id->callback) {
+			nm_utils_error_set_cancelled (&error_cancelled, FALSE, "NMSecretAgent");
+			error = error_cancelled;
+		}
+		_LOG2T (call_id, "cancelled");
+	} else if (error) {
+		nm_assert (!secrets);
+		_LOG2T (call_id, "completed with failure: %s", error->message);
+	} else {
+		nm_assert (   !secrets
+		           || g_variant_is_of_type (secrets, G_VARIANT_TYPE ("a{sa{sv}}")));
+		nm_assert ((!!secrets) == nm_streq0 (call_id->method_name, METHOD_GET_SECRETS));
+		_LOG2T (call_id, "completed successfully");
+	}
 
-	c_list_unlink (&r->lst);
+	if (call_id->callback)
+		call_id->callback (call_id->self, call_id, secrets, error, call_id->callback_data);
 
-	return TRUE;
+	if (free_call_id)
+		_call_id_free (call_id);
 }
 
 /*****************************************************************************/
@@ -209,6 +259,8 @@ nm_secret_agent_get_description (NMSecretAgent *agent)
 	return priv->description;
 }
 
+/*****************************************************************************/
+
 const char *
 nm_secret_agent_get_dbus_owner (NMSecretAgent *agent)
 {
@@ -265,6 +317,8 @@ nm_secret_agent_get_subject (NMSecretAgent *agent)
 	return NM_SECRET_AGENT_GET_PRIVATE (agent)->subject;
 }
 
+/*****************************************************************************/
+
 /**
  * nm_secret_agent_add_permission:
  * @agent: A #NMSecretAgent.
@@ -323,32 +377,38 @@ nm_secret_agent_has_permission (NMSecretAgent *agent, const char *permission)
 /*****************************************************************************/
 
 static void
-get_callback (GObject *proxy,
-              GAsyncResult *result,
-              gpointer user_data)
+_dbus_call_cb (GObject *source,
+               GAsyncResult *result,
+               gpointer user_data)
 {
-	NMSecretAgentCallId *r = user_data;
-
-	if (request_check_return (r)) {
-		NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
-		gs_free_error GError *error = NULL;
-		gs_unref_variant GVariant *ret = NULL;
-		gs_unref_variant GVariant *secrets = NULL;
-
-		ret = _nm_dbus_proxy_call_finish (priv->proxy, result, G_VARIANT_TYPE ("(a{sa{sv}})"), &error);
-		if (!ret)
-			g_dbus_error_strip_remote_error (error);
-		else {
+	NMSecretAgentCallId *call_id;
+	gs_unref_variant GVariant *ret = NULL;
+	gs_unref_variant GVariant *secrets = NULL;
+	gs_free_error GError *error = NULL;
+
+	ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error);
+
+	if (   !ret
+	    && nm_utils_error_is_cancelled (error, FALSE))
+		return;
+
+	call_id = user_data;
+
+	if (!ret)
+		g_dbus_error_strip_remote_error (error);
+	else {
+		if (nm_streq (call_id->method_name, METHOD_GET_SECRETS)) {
 			g_variant_get (ret,
 			               "(@a{sa{sv}})",
 			               &secrets);
 		}
-		r->callback (r->agent, r, secrets, error, r->callback_data);
 	}
 
-	request_free (r);
+	_call_id_invoke_callback (call_id, secrets, error, FALSE, TRUE);
 }
 
+/*****************************************************************************/
+
 NMSecretAgentCallId *
 nm_secret_agent_get_secrets (NMSecretAgent *self,
                              const char *path,
@@ -361,160 +421,139 @@ nm_secret_agent_get_secrets (NMSecretAgent *self,
 {
 	NMSecretAgentPrivate *priv;
 	GVariant *dict;
-	NMSecretAgentCallId *r;
+	NMSecretAgentCallId *call_id;
 
 	g_return_val_if_fail (NM_IS_SECRET_AGENT (self), NULL);
 	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
 	g_return_val_if_fail (path && *path, NULL);
-	g_return_val_if_fail (setting_name != NULL, NULL);
+	g_return_val_if_fail (setting_name, NULL);
+	g_return_val_if_fail (callback, NULL);
 
 	priv = NM_SECRET_AGENT_GET_PRIVATE (self);
-	g_return_val_if_fail (priv->proxy != NULL, NULL);
 
 	dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL);
 
 	/* Mask off the private flags if present */
-	flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM;
-	flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS;
-
-	r = request_new (self, "GetSecrets", path, setting_name, callback, callback_data);
-	r->is_get_secrets = TRUE;
-
-	g_dbus_proxy_call (priv->proxy,
-	                   "GetSecrets",
-	                   g_variant_new ("(@a{sa{sv}}os^asu)",
-	                                  dict,
-	                                  path,
-	                                  setting_name,
-	                                  hints ?: NM_PTRARRAY_EMPTY (const char *),
-	                                  (guint32) flags),
-	                   G_DBUS_CALL_FLAGS_NONE,
-	                   120000,
-	                   r->cancellable,
-	                   get_callback,
-	                   r);
-
-	g_dbus_proxy_set_default_timeout (G_DBUS_PROXY (priv->proxy), -1);
-
-	return r;
+	flags &= ~(  NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM
+	           | NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS);
+
+	call_id = _call_id_new (self, METHOD_GET_SECRETS, path, setting_name, callback, callback_data);
+
+	g_dbus_connection_call (priv->dbus_connection,
+	                        priv->dbus_owner,
+	                        NM_DBUS_PATH_SECRET_AGENT,
+	                        NM_DBUS_INTERFACE_SECRET_AGENT,
+	                        call_id->method_name,
+	                        g_variant_new ("(@a{sa{sv}}os^asu)",
+	                                       dict,
+	                                       path,
+	                                       setting_name,
+	                                       hints ?: NM_PTRARRAY_EMPTY (const char *),
+	                                       (guint32) flags),
+	                        G_VARIANT_TYPE ("(a{sa{sv}})"),
+	                        G_DBUS_CALL_FLAGS_NO_AUTO_START,
+	                        120000,
+	                        call_id->cancellable,
+	                        _dbus_call_cb,
+	                        call_id);
+
+	return call_id;
 }
 
 /*****************************************************************************/
 
 static void
-cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data)
+_call_cancel_cb (GObject *source,
+                 GAsyncResult *result,
+                 gpointer user_data)
 {
-	gs_free char *description = user_data;
+	NMSecretAgentCallId *call_id = user_data;
 	gs_free_error GError *error = NULL;
 	gs_unref_variant GVariant *ret = NULL;
 
-	ret = _nm_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), result, G_VARIANT_TYPE ("()"), &error);
-	if (!ret) {
-		nm_log_dbg (LOGD_AGENTS, "%s%s%s: agent failed to cancel secrets: %s",
-		            NM_PRINT_FMT_QUOTED (description, "(", description, ")", "???"),
-		            error->message);
-	}
-}
+	ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error);
 
-static void
-do_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId *r, gboolean disposing)
-{
-	NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
-	GCancellable *cancellable;
-	NMSecretAgentCallback callback;
-	gpointer callback_data;
-
-	g_return_if_fail (r->agent == self);
-	g_return_if_fail (r->cancellable);
-
-	if (   r->is_get_secrets
-	    && priv->proxy) {
-		/* for GetSecrets call, we must cancel the request. */
-		g_dbus_proxy_call (G_DBUS_PROXY (priv->proxy),
-		                   "CancelGetSecrets",
-		                   g_variant_new ("(os)",
-		                                  r->path,
-		                                  r->setting_name),
-		                   G_DBUS_CALL_FLAGS_NONE,
-		                   -1,
-		                   NULL,
-		                   cancel_done,
-		                   g_strdup (nm_secret_agent_get_description (self)));
+	if (ret)
+		_LOG2T (call_id, "success cancelling GetSecrets");
+	else if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
+		_LOG2T (call_id, "cancelling GetSecrets no longer works as service disconnected");
+	else {
+		_LOG2T (call_id, "failed to cancel GetSecrets: %s",
+		        error->message);
 	}
 
-	cancellable = r->cancellable;
-	callback = r->callback;
-	callback_data = r->callback_data;
-
-	/* During g_cancellable_cancel() the d-bus method might return synchronously.
-	 * Clear r->cancellable first, so that it doesn't actually do anything.
-	 * After that, @r might be already freed. */
-	r->cancellable = NULL;
-	g_cancellable_cancel (cancellable);
-	g_object_unref (cancellable);
-
-	/* Don't free the request @r. It will be freed when the d-bus call returns.
-	 * Only clear r->cancellable to indicate that the request was cancelled. */
-
-	if (callback) {
-		gs_free_error GError *error = NULL;
-
-		nm_utils_error_set_cancelled (&error, disposing, "NMSecretAgent");
-		/* @r might be a dangling pointer at this point. However, that is no problem
-		 * to pass it as (opaque) call_id. */
-		callback (self, r, NULL, error, callback_data);
-	}
+	_call_id_free (call_id);
 }
 
 /**
- * nm_secret_agent_cancel_secrets:
- * @self: #NMSecretAgent instance
- * @call_id: the call id to cancel
+ * nm_secret_agent_cancel_call:
+ * @self: the #NMSecretAgent instance for the @call_id.
+ *   Maybe be %NULL if @call_id is %NULL.
+ * @call_id: (allow-none): the call id to cancel. May be %NULL for convenience,
+ *   in which case it does nothing.
  *
  * It is an error to pass an invalid @call_id or a @call_id for an operation
- * that already completed. NMSecretAgent will always invoke the callback,
- * also for cancel() and dispose().
- * In case of nm_secret_agent_cancel_secrets() this will synchronously invoke the
- * callback before nm_secret_agent_cancel_secrets() returns.
+ * that already completed. It is also an error to cancel the call from inside
+ * the callback, at that point the call is already completed.
+ * In case of nm_secret_agent_cancel_call() this will synchronously invoke the
+ * callback before nm_secret_agent_cancel_call() returns.
  */
 void
-nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId *call_id)
+nm_secret_agent_cancel_call (NMSecretAgent *self,
+                             NMSecretAgentCallId *call_id)
 {
-	NMSecretAgentCallId *r = call_id;
-
-	g_return_if_fail (NM_IS_SECRET_AGENT (self));
-	g_return_if_fail (r);
-
-	nm_assert (c_list_contains (&NM_SECRET_AGENT_GET_PRIVATE (self)->requests,
-	                            &r->lst));
-
-	c_list_unlink (&r->lst);
+	NMSecretAgentPrivate *priv;
+	gboolean free_call_id = TRUE;
 
-	do_cancel_secrets (self, r, FALSE);
-}
+	if (!call_id) {
+		/* for convenience, %NULL is accepted fine. */
+		nm_assert (!self || NM_IS_SECRET_AGENT (self));
+		return;
+	}
 
-/*****************************************************************************/
+	g_return_if_fail (NM_IS_SECRET_AGENT (call_id->self));
+	g_return_if_fail (!c_list_is_empty (&call_id->lst));
 
-static void
-agent_save_cb (GObject *proxy,
-               GAsyncResult *result,
-               gpointer user_data)
-{
-	NMSecretAgentCallId *r = user_data;
+	/* Theoretically, call-id already has a self pointer. But nm_secret_agent_cancel_call() has only
+	 * one user: NMAgentManager. And that one has the self-pointer at hand, so the only purpose of
+	 * the @self argument is to assert that we are cancelling the expected call.
+	 *
+	 * We could drop the @self argument, but that just remove an additional assert-check from
+	 * our code, without making a simplification for the only caller of this function. */
+	g_return_if_fail (self == call_id->self);
 
-	if (request_check_return (r)) {
-		gs_free_error GError *error = NULL;
-		gs_unref_variant GVariant *ret = NULL;
+	priv = NM_SECRET_AGENT_GET_PRIVATE (self);
 
-		ret = _nm_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), result, G_VARIANT_TYPE ("()"), &error);
-		if (!ret)
-			g_dbus_error_strip_remote_error (error);
-		r->callback (r->agent, r, NULL, error, r->callback_data);
+	nm_assert (c_list_contains (&priv->requests,
+	                            &call_id->lst));
+
+	nm_clear_g_cancellable (&call_id->cancellable);
+
+	if (nm_streq (call_id->method_name, METHOD_GET_SECRETS)) {
+		g_dbus_connection_call (priv->dbus_connection,
+		                        priv->dbus_owner,
+		                        NM_DBUS_PATH_SECRET_AGENT,
+		                        NM_DBUS_INTERFACE_SECRET_AGENT,
+		                        METHOD_CANCEL_GET_SECRETS,
+		                        g_variant_new ("(os)",
+		                                       call_id->path,
+		                                       call_id->setting_name),
+		                        G_VARIANT_TYPE ("()"),
+		                        G_DBUS_CALL_FLAGS_NO_AUTO_START,
+		                        NM_SHUTDOWN_TIMEOUT_MS,
+		                        NULL, /* this operation is not cancellable. We rely on the timeout. */
+		                        _call_cancel_cb,
+		                        call_id);
+		/* we keep call-id alive, but it will be unlinked from priv->requests.
+		 * _call_cancel_cb() will finally free it later. */
+		free_call_id = FALSE;
 	}
 
-	request_free (r);
+	_call_id_invoke_callback (call_id, NULL, NULL, TRUE, free_call_id);
 }
 
+/*****************************************************************************/
+
 NMSecretAgentCallId *
 nm_secret_agent_save_secrets (NMSecretAgent *self,
                               const char *path,
@@ -524,7 +563,7 @@ nm_secret_agent_save_secrets (NMSecretAgent *self,
 {
 	NMSecretAgentPrivate *priv;
 	GVariant *dict;
-	NMSecretAgentCallId *r;
+	NMSecretAgentCallId *call_id;
 
 	g_return_val_if_fail (NM_IS_SECRET_AGENT (self), NULL);
 	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
@@ -535,43 +574,28 @@ nm_secret_agent_save_secrets (NMSecretAgent *self,
 	/* Caller should have ensured that only agent-owned secrets exist in 'connection' */
 	dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL);
 
-	r = request_new (self, "SaveSecrets", path, NULL, callback, callback_data);
-	g_dbus_proxy_call (priv->proxy,
-	                   "SaveSecrets",
-	                   g_variant_new ("(@a{sa{sv}}o)",
-	                                  dict,
-	                                  path),
-	                   G_DBUS_CALL_FLAGS_NONE,
-	                   -1,
-	                   NULL, /* cancelling the request does *not* cancel the D-Bus call. */
-	                   agent_save_cb,
-	                   r);
-
-	return r;
+	call_id = _call_id_new (self, METHOD_SAVE_SECRETS, path, NULL, callback, callback_data);
+
+	g_dbus_connection_call (priv->dbus_connection,
+	                        priv->dbus_owner,
+	                        NM_DBUS_PATH_SECRET_AGENT,
+	                        NM_DBUS_INTERFACE_SECRET_AGENT,
+	                        call_id->method_name,
+	                        g_variant_new ("(@a{sa{sv}}o)",
+	                                       dict,
+	                                       path),
+	                        G_VARIANT_TYPE ("()"),
+	                        G_DBUS_CALL_FLAGS_NO_AUTO_START,
+	                        60000,
+	                        call_id->cancellable,
+	                        _dbus_call_cb,
+	                        call_id);
+
+	return call_id;
 }
 
 /*****************************************************************************/
 
-static void
-agent_delete_cb (GObject *proxy,
-                 GAsyncResult *result,
-                 gpointer user_data)
-{
-	NMSecretAgentCallId *r = user_data;
-
-	if (request_check_return (r)) {
-		gs_free_error GError *error = NULL;
-		gs_unref_variant GVariant *ret = NULL;
-
-		ret = _nm_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), result, G_VARIANT_TYPE ("()"), &error);
-		if (!ret)
-			g_dbus_error_strip_remote_error (error);
-		r->callback (r->agent, r, NULL, error, r->callback_data);
-	}
-
-	request_free (r);
-}
-
 NMSecretAgentCallId *
 nm_secret_agent_delete_secrets (NMSecretAgent *self,
                                 const char *path,
@@ -581,7 +605,7 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self,
 {
 	NMSecretAgentPrivate *priv;
 	GVariant *dict;
-	NMSecretAgentCallId *r;
+	NMSecretAgentCallId *call_id;
 
 	g_return_val_if_fail (NM_IS_SECRET_AGENT (self), NULL);
 	g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL);
@@ -592,58 +616,90 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self,
 	/* No secrets sent; agents must be smart enough to track secrets using the UUID or something */
 	dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_NO_SECRETS);
 
-	r = request_new (self, "DeleteSecrets", path, NULL, callback, callback_data);
-	g_dbus_proxy_call (priv->proxy,
-	                   "DeleteSecrets",
-	                   g_variant_new ("(@a{sa{sv}}o)",
-	                                  dict,
-	                                  path),
-	                   G_DBUS_CALL_FLAGS_NONE,
-	                   -1,
-	                   NULL, /* cancelling the request does *not* cancel the D-Bus call. */
-	                   agent_delete_cb,
-	                   r);
-	return r;
+	call_id = _call_id_new (self, METHOD_DELETE_SECRETS, path, NULL, callback, callback_data);
+
+	g_dbus_connection_call (priv->dbus_connection,
+	                        priv->dbus_owner,
+	                        NM_DBUS_PATH_SECRET_AGENT,
+	                        NM_DBUS_INTERFACE_SECRET_AGENT,
+	                        call_id->method_name,
+	                        g_variant_new ("(@a{sa{sv}}o)",
+	                                       dict,
+	                                       path),
+	                        G_VARIANT_TYPE ("()"),
+	                        G_DBUS_CALL_FLAGS_NO_AUTO_START,
+	                        60000,
+	                        call_id->cancellable,
+	                        _dbus_call_cb,
+	                        call_id);
+	return call_id;
 }
 
 /*****************************************************************************/
 
 static void
-_on_disconnected_cleanup (NMSecretAgentPrivate *priv)
+name_owner_changed (NMSecretAgent *self,
+                    const char *owner)
 {
+	NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
+
+	nm_assert (!priv->name_owner_cancellable);
+
+	owner = nm_str_not_empty (owner);
+
+	_LOGT ("name-owner-changed: %s%s%s",
+	       NM_PRINT_FMT_QUOTED (owner, "has ", owner, "", "disconnected"));
+
+	if (owner)
+		return;
+
 	nm_clear_g_dbus_connection_signal (priv->dbus_connection,
-	                                   &priv->on_disconnected_id);
-	g_clear_object (&priv->dbus_connection);
-	g_clear_object (&priv->proxy);
+	                                   &priv->name_owner_changed_id);
+
+	g_signal_emit (self, signals[DISCONNECTED], 0);
 }
 
 static void
-_on_disconnected_name_owner_changed (GDBusConnection *dbus_connection,
-                                     const char       *sender_name,
-                                     const char       *object_path,
-                                     const char       *interface_name,
-                                     const char       *signal_name,
-                                     GVariant         *parameters,
-                                     gpointer          user_data)
+name_owner_changed_cb (GDBusConnection *dbus_connection,
+                       const char       *sender_name,
+                       const char       *object_path,
+                       const char       *interface_name,
+                       const char       *signal_name,
+                       GVariant         *parameters,
+                       gpointer          user_data)
 {
 	NMSecretAgent *self = NM_SECRET_AGENT (user_data);
-	NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
-	const char *old_owner = NULL, *new_owner = NULL;
+	const char *new_owner = NULL;
+
+	if (g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) {
+		g_variant_get (parameters,
+		               "(&s&s&s)",
+		               NULL,
+		               NULL,
+		               &new_owner);
+	}
 
-	g_variant_get (parameters,
-	               "(&s&s&s)",
-	               NULL,
-	               &old_owner,
-	               &new_owner);
+	nm_clear_g_cancellable (&NM_SECRET_AGENT_GET_PRIVATE (self)->name_owner_cancellable);
 
-	_LOGT ("name-owner-changed: %s%s%s => %s%s%s",
-	       NM_PRINT_FMT_QUOTE_STRING (old_owner),
-	       NM_PRINT_FMT_QUOTE_STRING (new_owner));
+	name_owner_changed (self, new_owner);
+}
 
-	if (!*new_owner) {
-		_on_disconnected_cleanup (priv);
-		g_signal_emit (self, signals[DISCONNECTED], 0);
-	}
+static void
+get_name_owner_cb (const char *name_owner,
+                   GError *error,
+                   gpointer user_data)
+{
+	NMSecretAgent *self;
+
+	if (   !name_owner
+	    && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		return;
+
+	self = user_data;
+
+	g_clear_object (&NM_SECRET_AGENT_GET_PRIVATE (self)->name_owner_cancellable);
+
+	name_owner_changed (self, name_owner);
 }
 
 /*****************************************************************************/
@@ -664,7 +720,6 @@ nm_secret_agent_new (GDBusMethodInvocation *context,
 	char buf_caps[150];
 	gulong uid;
 	GDBusConnection *dbus_connection;
-	gs_free_error GError *error = NULL;
 
 	g_return_val_if_fail (context != NULL, NULL);
 	g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL);
@@ -703,28 +758,19 @@ nm_secret_agent_new (GDBusMethodInvocation *context,
 	priv->capabilities = capabilities;
 	priv->subject = g_object_ref (subject);
 
-	priv->proxy = g_dbus_proxy_new_sync (priv->dbus_connection,
-	                                       G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES
-	                                     | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS,
-	                                     NULL,
-	                                     priv->dbus_owner,
-	                                     NM_DBUS_PATH_SECRET_AGENT,
-	                                     NM_DBUS_INTERFACE_SECRET_AGENT,
-	                                     NULL,
-	                                     &error);
-	if (!priv->proxy) {
-		_LOGW ("could not create proxy for %s on connection %s: %s",
-		       NM_DBUS_INTERFACE_SECRET_AGENT,
-		       priv->dbus_owner,
-		       error->message);
-		g_clear_error (&error);
-	}
+	priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection,
+	                                                                                      priv->dbus_owner,
+	                                                                                      name_owner_changed_cb,
+	                                                                                      self,
+	                                                                                      NULL);
 
-	priv->on_disconnected_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection,
-	                                                                                   priv->dbus_owner,
-	                                                                                   _on_disconnected_name_owner_changed,
-	                                                                                   self,
-	                                                                                   NULL);
+	priv->name_owner_cancellable = g_cancellable_new ();
+	nm_dbus_connection_call_get_name_owner (priv->dbus_connection,
+	                                        priv->dbus_owner,
+	                                        -1,
+	                                        priv->name_owner_cancellable,
+	                                        get_name_owner_cb,
+	                                        self);
 
 	return self;
 }
@@ -743,18 +789,13 @@ dispose (GObject *object)
 {
 	NMSecretAgent *self = NM_SECRET_AGENT (object);
 	NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
-	CList *iter;
 
-again:
-	c_list_for_each (iter, &priv->requests) {
-		c_list_unlink (iter);
-		do_cancel_secrets (self, c_list_entry (iter, NMSecretAgentCallId, lst), TRUE);
-		goto again;
-	}
+	nm_assert (c_list_is_empty (&priv->requests));
 
-	_on_disconnected_cleanup (priv);
+	nm_clear_g_dbus_connection_signal (priv->dbus_connection,
+	                                   &priv->name_owner_changed_id);
 
-	g_clear_object (&priv->subject);
+	nm_clear_g_cancellable (&priv->name_owner_cancellable);
 
 	G_OBJECT_CLASS (nm_secret_agent_parent_class)->dispose (object);
 }
@@ -772,6 +813,10 @@ finalize (GObject *object)
 
 	nm_c_list_elem_free_all (&priv->permissions, g_free);
 
+	g_clear_object (&priv->subject);
+
+	g_clear_object (&priv->dbus_connection);
+
 	G_OBJECT_CLASS (nm_secret_agent_parent_class)->finalize (object);
 
 	_LOGT ("finalized");
diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h
index 209a10094b..b85a8b8748 100644
--- a/src/settings/nm-secret-agent.h
+++ b/src/settings/nm-secret-agent.h
@@ -79,9 +79,6 @@ NMSecretAgentCallId *nm_secret_agent_get_secrets (NMSecretAgent *agent,
                                                   NMSecretAgentCallback callback,
                                                   gpointer callback_data);
 
-void        nm_secret_agent_cancel_secrets (NMSecretAgent *agent,
-                                            NMSecretAgentCallId *call_id);
-
 NMSecretAgentCallId *nm_secret_agent_save_secrets (NMSecretAgent *agent,
                                                    const char *path,
                                                    NMConnection *connection,
@@ -94,4 +91,7 @@ NMSecretAgentCallId *nm_secret_agent_delete_secrets (NMSecretAgent *agent,
                                                      NMSecretAgentCallback callback,
                                                      gpointer callback_data);
 
+void nm_secret_agent_cancel_call (NMSecretAgent *self,
+                                  NMSecretAgentCallId *call_id);
+
 #endif /* __NETWORKMANAGER_SECRET_AGENT_H__ */