fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2025-12-24 21:50:17 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

secret-agent: rework handling of asynchronous request and cancelling

Browse source 
Refactor the handling of the asynchronous requests so that now
NMSecretAgent has the following properties:

- The callback will *always* be invoked exactly once (sans crashes).
  Even if you cancel the call or if you dispose NMSecretAgent with
  pending calls. That allows the caller to rely on being called back
  and possibly cleanup the user-data.

- Callbacks are always invoked asynchronously with respect to their
  start-call.

- You can cancel all 3 types of operations, not only the 'GetSecrets'
  call. Note that this will still not cancel the calls 'DeleteSecrets'
  and 'SaveSecrets' on a D-Bus level.
  When cancelling, the callback will be invoked synchronously with
  respect to the cancel call, with an GError indicating the cancellation
  (G_IO_ERROR_CANCELLED).

- During dispose, the callback is also invoked synchronously, with
  some other error reason.

This also fixes a crash where handling of the asynchronous data was
messed up and the priv->requests hash would end up to containing dangling
pointers.

https://bugzilla.redhat.com/show_bug.cgi?id=1253407
This commit is contained in:
Thomas Haller 2015-08-14 10:36:50 +02:00
parent 9cace5b411
commit 92dda6472c
2 changed files with 176 additions and 65 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

70
src/settings/nm-agent-manager.c
View file

@ -585,9 +585,12 @@ request_next_agent (Request *req)
{
GError *error = NULL;
req->current_call_id = NULL;
if (req->current)
g_object_unref (req->current);
if (req->current) {
if (req->current_call_id)
nm_secret_agent_cancel_secrets (req->current, req->current_call_id);
g_clear_object (&req->current);
}
g_warn_if_fail (!req->current_call_id);
if (req->pending) {
/* Send the request to the next agent */
@ -600,8 +603,6 @@ request_next_agent (Request *req)
req->next_callback (req);
} else {
req->current = NULL;
/* No more secret agents are available to fulfill this secrets request */
error = g_error_new_literal (NM_AGENT_MANAGER_ERROR,
NM_AGENT_MANAGER_ERROR_NO_SECRETS,
@ -783,13 +784,22 @@ get_done_cb (NMSecretAgent *agent,
char *agent_uname = NULL;
g_return_if_fail (call_id == parent->current_call_id);
g_return_if_fail (agent == parent->current);
parent->current_call_id = NULL;
if (error) {
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed secrets request %p/%s/%s: (%d) %s",
if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
nm_log_dbg (LOGD_AGENTS, "(%s) get secrets request cancelled: %p/%s/%s",
nm_secret_agent_get_description (agent),
req, parent->detail, req->setting_name);
return;
}
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed secrets request %p/%s/%s: %s",
nm_secret_agent_get_description (agent),
req, parent->detail, req->setting_name,
error ? error->code : -1,
(error && error->message) ? error->message : "(unknown)");
error->message);
if (g_error_matches (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_USER_CANCELED)) {
error = g_error_new_literal (NM_AGENT_MANAGER_ERROR,
@ -900,9 +910,8 @@ get_agent_request_secrets (ConnectionRequest *req, gboolean include_system_secre
req->flags,
get_done_cb,
req);
if (parent->current_call_id == NULL) {
/* Shouldn't hit this, but handle it anyway */
g_warn_if_fail (parent->current_call_id != NULL);
if (!parent->current_call_id) {
g_warn_if_reached ();
request_next_agent (parent);
}
@ -1230,13 +1239,22 @@ save_done_cb (NMSecretAgent *agent,
const char *agent_dbus_owner;
g_return_if_fail (call_id == parent->current_call_id);
g_return_if_fail (agent == parent->current);
parent->current_call_id = NULL;
if (error) {
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed save secrets request %p/%s: (%d) %s",
if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
nm_log_dbg (LOGD_AGENTS, "(%s) save secrets request cancelled: %p/%s",
nm_secret_agent_get_description (agent),
req, parent->detail);
return;
}
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed save secrets request %p/%s: %s",
nm_secret_agent_get_description (agent),
req, parent->detail,
error ? error->code : -1,
(error && error->message) ? error->message : "(unknown)");
error->message);
/* Try the next agent */
request_next_agent (parent);
maybe_remove_agent_on_error (agent, error);
@ -1260,9 +1278,8 @@ save_next_cb (Request *parent)
req->connection,
save_done_cb,
req);
if (parent->current_call_id == NULL) {
/* Shouldn't hit this, but handle it anyway */
g_warn_if_fail (parent->current_call_id != NULL);
if (!parent->current_call_id) {
g_warn_if_reached ();
request_next_agent (parent);
}
}
@ -1323,12 +1340,20 @@ delete_done_cb (NMSecretAgent *agent,
Request *req = user_data;
g_return_if_fail (call_id == req->current_call_id);
g_return_if_fail (agent == req->current);
req->current_call_id = NULL;
if (error) {
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed delete secrets request %p/%s: (%d) %s",
if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
nm_log_dbg (LOGD_AGENTS, "(%s) delete secrets request cancelled: %p/%s",
nm_secret_agent_get_description (agent), req, req->detail);
return;
}
nm_log_dbg (LOGD_AGENTS, "(%s) agent failed delete secrets request %p/%s: %s",
nm_secret_agent_get_description (agent), req, req->detail,
error ? error->code : -1,
(error && error->message) ? error->message : "(unknown)");
error->message);
} else {
nm_log_dbg (LOGD_AGENTS, "(%s) agent deleted secrets for request %p/%s",
nm_secret_agent_get_description (agent), req, req->detail);
@ -1349,9 +1374,8 @@ delete_next_cb (Request *parent)
req->connection,
delete_done_cb,
req);
if (parent->current_call_id == NULL) {
/* Shouldn't hit this, but handle it anyway */
g_warn_if_fail (parent->current_call_id != NULL);
if (!parent->current_call_id) {
g_warn_if_reached ();
request_next_agent (parent);
}
}

171
src/settings/nm-secret-agent.c
View file

@ -68,6 +68,7 @@ struct _NMSecretAgentCallId {
GCancellable *cancellable;
char *path;
char *setting_name;
gboolean is_get_secrets;
NMSecretAgentCallback callback;
gpointer callback_data;
};
@ -98,10 +99,29 @@ request_free (Request *r)
{
g_free (r->path);
g_free (r->setting_name);
g_object_unref (r->cancellable);
if (r->cancellable)
g_object_unref (r->cancellable);
g_slice_free (Request, r);
}
static gboolean
request_check_return (Request *r)
{
NMSecretAgentPrivate *priv;
if (!r->cancellable)
return FALSE;
g_return_val_if_fail (NM_IS_SECRET_AGENT (r->agent), FALSE);
priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
if (!g_hash_table_remove (priv->requests, r))
g_return_val_if_reached (FALSE);
return TRUE;
}
/*************************************************************/
const char *
@ -264,21 +284,19 @@ get_callback (GObject *proxy,
gpointer user_data)
{
Request *r = user_data;
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
GVariant *secrets = NULL;
GError *error = NULL;
if (!g_cancellable_is_cancelled (r->cancellable)) {
if (request_check_return (r)) {
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
gs_free_error GError *error = NULL;
gs_unref_variant GVariant *secrets = NULL;
nmdbus_secret_agent_call_get_secrets_finish (priv->proxy, &secrets, result, &error);
if (error)
g_dbus_error_strip_remote_error (error);
r->callback (r->agent, r, secrets, error, r->callback_data);
g_clear_pointer (&secrets, g_variant_unref);
g_clear_error (&error);
}
g_hash_table_remove (priv->requests, r);
request_free (r);
}
NMSecretAgentCallId
@ -309,6 +327,8 @@ nm_secret_agent_get_secrets (NMSecretAgent *self,
flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS;
r = request_new (self, nm_connection_get_path (connection), setting_name, callback, callback_data);
r->is_get_secrets = TRUE;
g_hash_table_add (priv->requests, r);
nmdbus_secret_agent_call_get_secrets (priv->proxy,
dict,
nm_connection_get_path (connection),
@ -317,11 +337,12 @@ nm_secret_agent_get_secrets (NMSecretAgent *self,
flags,
r->cancellable,
get_callback, r);
g_hash_table_insert (priv->requests, r, r);
return r;
}
/*************************************************************/
static void
cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data)
{
@ -329,33 +350,85 @@ cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data)
GError *error = NULL;
if (!nmdbus_secret_agent_call_cancel_get_secrets_finish (NMDBUS_SECRET_AGENT (proxy), result, &error)) {
g_dbus_error_strip_remote_error (error);
nm_log_dbg (LOGD_AGENTS, "(%s): agent failed to cancel secrets: %s",
description, error->message);
nm_log_dbg (LOGD_AGENTS, "%s%s%s: agent failed to cancel secrets: %s",
NM_PRINT_FMT_QUOTED (description, "(", description, ")", "???"),
error->message);
g_clear_error (&error);
}
g_free (description);
}
static void
do_cancel_secrets (NMSecretAgent *self, Request *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. */
nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy,
r->path, r->setting_name,
NULL,
cancel_done,
g_strdup (nm_secret_agent_get_description (self)));
}
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) {
GError *error = NULL;
if (disposing) {
/* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously
* when the user calls nm_act_request_cancel_secrets().
* When the user disposes the instance, we also invoke the callback synchronously,
* but with a different error-reason. */
g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED,
"Disposing NMSecretAgent instance");
} else {
g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
"Request cancelled");
}
/* @r might be a dangling pointer at this point. However, that is no problem
* to pass it as (opaque) call_id. */
callback (self, r, NULL, error, callback_data);
g_error_free (error);
}
}
void
nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call)
nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call_id)
{
NMSecretAgentPrivate *priv;
Request *r = (gpointer) call;
Request *r = call_id;
g_return_if_fail (NM_IS_SECRET_AGENT (self));
g_return_if_fail (r);
g_return_if_fail (self != NULL);
priv = NM_SECRET_AGENT_GET_PRIVATE (self);
g_return_if_fail (priv->proxy != NULL);
g_return_if_fail (r != NULL);
if (!g_hash_table_remove (priv->requests, r))
g_return_if_reached ();
g_cancellable_cancel (r->cancellable);
nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy,
r->path, r->setting_name,
NULL,
cancel_done,
g_strdup (nm_secret_agent_get_description (self)));
do_cancel_secrets (self, r, FALSE);
}
/*************************************************************/
@ -366,18 +439,18 @@ agent_save_cb (GObject *proxy,
gpointer user_data)
{
Request *r = user_data;
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
GError *error = NULL;
if (!g_cancellable_is_cancelled (r->cancellable)) {
if (request_check_return (r)) {
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
gs_free_error GError *error = NULL;
nmdbus_secret_agent_call_save_secrets_finish (priv->proxy, result, &error);
if (error)
g_dbus_error_strip_remote_error (error);
r->callback (r->agent, r, NULL, error, r->callback_data);
g_clear_error (&error);
}
g_hash_table_remove (priv->requests, r);
request_free (r);
}
NMSecretAgentCallId
@ -401,31 +474,35 @@ nm_secret_agent_save_secrets (NMSecretAgent *self,
dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL);
r = request_new (self, cpath, NULL, callback, callback_data);
g_hash_table_add (priv->requests, r);
nmdbus_secret_agent_call_save_secrets (priv->proxy,
dict, cpath,
NULL,
NULL, /* cancelling the request does *not* cancel the D-Bus call. */
agent_save_cb, r);
g_hash_table_insert (priv->requests, r, r);
return r;
}
/*************************************************************/
static void
agent_delete_cb (GObject *proxy,
GAsyncResult *result,
gpointer user_data)
{
Request *r = user_data;
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
GError *error = NULL;
if (!g_cancellable_is_cancelled (r->cancellable)) {
if (request_check_return (r)) {
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent);
gs_free_error GError *error = NULL;
nmdbus_secret_agent_call_delete_secrets_finish (priv->proxy, result, &error);
if (error)
g_dbus_error_strip_remote_error (error);
r->callback (r->agent, r, NULL, error, r->callback_data);
g_clear_error (&error);
}
g_hash_table_remove (priv->requests, r);
request_free (r);
}
NMSecretAgentCallId
@ -449,15 +526,17 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self,
dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_NO_SECRETS);
r = request_new (self, cpath, NULL, callback, callback_data);
g_hash_table_add (priv->requests, r);
nmdbus_secret_agent_call_delete_secrets (priv->proxy,
dict, cpath,
NULL,
NULL, /* cancelling the request does *not* cancel the D-Bus call. */
agent_delete_cb, r);
g_hash_table_insert (priv->requests, r, r);
return r;
}
/*************************************************************/
static void
name_owner_changed_cb (GObject *proxy,
GParamSpec *pspec,
@ -532,14 +611,22 @@ nm_secret_agent_init (NMSecretAgent *self)
{
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
priv->requests = g_hash_table_new_full (g_direct_hash, g_direct_equal,
NULL, (GDestroyNotify) request_free);
priv->requests = g_hash_table_new (g_direct_hash, g_direct_equal);
}
static void
dispose (GObject *object)
{
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (object);
NMSecretAgent *self = NM_SECRET_AGENT (object);
NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self);
GHashTableIter iter;
Request *r;
g_hash_table_iter_init (&iter, priv->requests);
while (g_hash_table_iter_next (&iter, (gpointer *) &r, NULL)) {
g_hash_table_iter_remove (&iter);
do_cancel_secrets (self, r, TRUE);
}
g_clear_object (&priv->proxy);
g_clear_object (&priv->subject);