Attachment 89223 Details for Bug 27727 – 16/26] Make account deletion async (sort of), and imply a selective commit

[patch] 16/26] Make account deletion async (sort of), and imply a selective commit

0016-Make-account-deletion-async-sort-of-and-imply-a-sele.patch (text/plain), 27.33 KB, created by Simon McVittie on 2013-11-14 18:16:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-11-14 18:16:16 UTC

Size: 27.33 KB

patch

obsolete

>From e46909f3e2adadcbf8bb12a9c8f920e9c5a8105e Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Wed, 13 Nov 2013 19:44:46 +0000
>Subject: [PATCH 16/26] Make account deletion async (sort of), and imply a
> selective commit
>
>This means we don't need to commit separately after each deletion,
>and means account plugins don't have to have the concept of flagging
>an account for "delete this later" - much rejoicing.
>
>It also has the incidental benefit that we no longer use the C++
>reserved word 'delete' in a header file.
>---
> mission-control-plugins/account-storage.c |  97 ++++++++++--------
> mission-control-plugins/account-storage.h |  19 +++-
> src/mcd-account-manager-default.c         | 159 ++++++++++++++----------------
> src/mcd-account.c                         |   2 -
> src/mcd-storage.c                         |  28 +++++-
> tests/twisted/dbus-account-plugin.c       | 145 ++++++++++++++-------------
> tests/twisted/mcp-account-diversion.c     |  41 +++++---
> 7 files changed, 275 insertions(+), 216 deletions(-)
>
>diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c
>index 53b00be..b87909c 100644
>--- a/mission-control-plugins/account-storage.c
>+++ b/mission-control-plugins/account-storage.c
>@@ -119,13 +119,24 @@ default_get (const McpAccountStorage *storage,
>   return FALSE;
> }
> 
>-static gboolean
>-default_delete (const McpAccountStorage *storage,
>-    const McpAccountManager *am,
>+static void
>+default_delete_async (McpAccountStorage *storage,
>+    McpAccountManager *am,
>     const gchar *account,
>-    const gchar *key)
>+    GAsyncReadyCallback callback,
>+    gpointer user_data)
> {
>-  return FALSE;
>+  g_task_report_new_error (storage, callback, user_data,
>+      default_delete_async, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
>+      "This storage plugin cannot delete accounts");
>+}
>+
>+static gboolean
>+default_delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *res,
>+    GError **error)
>+{
>+  return g_task_propagate_boolean (G_TASK (res), error);
> }
> 
> static gboolean
>@@ -230,7 +241,8 @@ class_init (gpointer klass,
> 
>   iface->get = default_get;
>   iface->create = default_create;
>-  iface->delete = default_delete;
>+  iface->delete_async = default_delete_async;
>+  iface->delete_finish = default_delete_finish;
>   iface->commit = default_commit;
>   iface->list = default_list;
>   iface->ready = default_ready;
>@@ -646,61 +658,62 @@ mcp_account_storage_create (const McpAccountStorage *storage,
> }
> 
> /**
>- * McpAccountStorageDeleteFunc:
>+ * mcp_account_storage_delete_async:
>  * @storage: an #McpAccountStorage instance
>  * @am: an #McpAccountManager instance
>  * @account: the unique name of the account
>- * @key: (allow-none): the setting whose value we wish to store - either an
>- *  attribute like "DisplayName", or "param-" plus a parameter like
>- *  "account" - or %NULL to delete the entire account
>+ * @callback: called on success or failure
>+ * @user_data: data for @callback
>  *
>- * An implementation of mcp_account_storage_delete().
>+ * Delete the account @account, and commit the change,
>+ * emitting #McpAccountStorage::deleted afterwards.
>  *
>- * Returns: %TRUE if the setting or settings are not
>- * the plugin's cache after this operation, %FALSE otherwise.
>+ * Unlike the 'delete' virtual method in earlier MC versions, this
>+ * function is expected to commit the change to long-term storage,
>+ * is expected to emit #McpAccountStorage::deleted, and is
>+ * not called for the deletion of individual attributes or parameters.
>+ *
>+ * The default implementation just returns failure (asynchronously),
>+ * and is appropriate for read-only storage.
>  */
>+void
>+mcp_account_storage_delete_async (McpAccountStorage *storage,
>+    McpAccountManager *am,
>+    const gchar *account,
>+    GAsyncReadyCallback callback,
>+    gpointer user_data)
>+{
>+  McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>+
>+  SDEBUG (storage, "");
>+  g_return_if_fail (iface != NULL);
>+  g_return_if_fail (iface->delete_async != NULL);
>+
>+  iface->delete_async (storage, am, account, callback, user_data);
>+}
> 
> /**
>- * mcp_account_storage_delete:
>+ * mcp_account_storage_delete_finish:
>  * @storage: an #McpAccountStorage instance
>- * @am: an #McpAccountManager instance
>- * @account: the unique name of the account
>- * @key: (allow-none): the setting whose value we wish to store - either an
>- *  attribute like "DisplayName", or "param-" plus a parameter like
>- *  "account" - or %NULL to delete the entire account
>- *
>- * The plugin is expected to remove the setting for @key from its
>- * internal cache and to remember that its state has changed, so
>- * that it can delete said setting from its long term storage if
>- * its long term storage method makes this necessary.
>+ * @res: the result of mcp_account_storage_delete_async()
>+ * @error: used to raise an error if %FALSE is returned
>  *
>- * If @key is %NULL, the plugin should forget all its settings for
>- * @account,and remember to delete the entire account from its storage later.
>+ * Process the result of mcp_account_storage_delete_async().
>  *
>- * The plugin is not expected to update its long term storage at
>- * this point.
>- *
>- * The default implementation just returns %FALSE, and is appropriate for
>- * read-only storage.
>- *
>- * Returns: %TRUE if the setting or settings are not
>- * the plugin's cache after this operation, %FALSE otherwise.
>- * This is very unlikely to ever be %FALSE, as a plugin is always
>- * expected to be able to manipulate its own cache.
>+ * Returns: %TRUE on success, %FALSE if the account could not be deleted
>  */
> gboolean
>-mcp_account_storage_delete (const McpAccountStorage *storage,
>-    const McpAccountManager *am,
>-    const gchar *account,
>-    const gchar *key)
>+mcp_account_storage_delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *result,
>+    GError **error)
> {
>   McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> 
>   SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, FALSE);
>-  g_return_val_if_fail (iface->delete != NULL, FALSE);
>+  g_return_val_if_fail (iface->delete_finish != NULL, FALSE);
> 
>-  return iface->delete (storage, am, account, key);
>+  return iface->delete_finish (storage, result, error);
> }
> 
> /**
>diff --git a/mission-control-plugins/account-storage.h b/mission-control-plugins/account-storage.h
>index 198e835..ff3633e 100644
>--- a/mission-control-plugins/account-storage.h
>+++ b/mission-control-plugins/account-storage.h
>@@ -107,7 +107,14 @@ struct _McpAccountStorageIface
>   const gchar *provider;
> 
>   McpAccountStorageGetFunc get;
>-  McpAccountStorageDeleteFunc delete;
>+  void (*delete_async) (McpAccountStorage *storage,
>+      McpAccountManager *am,
>+      const gchar *account,
>+      GAsyncReadyCallback callback,
>+      gpointer user_data);
>+  gboolean (*delete_finish) (McpAccountStorage *storage,
>+      GAsyncResult *res,
>+      GError **error);
>   McpAccountStorageCommitFunc commit;
>   McpAccountStorageListFunc list;
>   McpAccountStorageReadyFunc ready;
>@@ -149,10 +156,14 @@ gchar * mcp_account_storage_create (const McpAccountStorage *storage,
>     const gchar *identification,
>     GError **error);
> 
>-gboolean mcp_account_storage_delete (const McpAccountStorage *storage,
>-    const McpAccountManager *am,
>+void mcp_account_storage_delete_async (McpAccountStorage *storage,
>+    McpAccountManager *am,
>     const gchar *account,
>-    const gchar *key);
>+    GAsyncReadyCallback callback,
>+    gpointer user_data);
>+gboolean mcp_account_storage_delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *result,
>+    GError **error);
> 
> void mcp_account_storage_ready (const McpAccountStorage *storage,
>     const McpAccountManager *am);
>diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
>index e277fe5..fef98dc 100644
>--- a/src/mcd-account-manager-default.c
>+++ b/src/mcd-account-manager-default.c
>@@ -47,8 +47,6 @@ typedef struct {
>     /* owned string, parameter (without "param-") => owned string, value
>      * parameters of unknwn type to be stored in the variant-file */
>     GHashTable *untyped_parameters;
>-    /* TRUE if the entire account is pending deletion */
>-    gboolean pending_deletion;
>     /* TRUE if the account doesn't really exist, but is here to stop us
>      * loading it from a lower-priority file */
>     gboolean absent;
>@@ -79,7 +77,6 @@ ensure_stored_account (McdAccountManagerDefault *self,
>       g_hash_table_insert (self->accounts, g_strdup (account), sa);
>     }
> 
>-  sa->pending_deletion = FALSE;
>   sa->absent = FALSE;
>   return sa;
> }
>@@ -346,38 +343,96 @@ _create (const McpAccountStorage *self,
>   return unique_name;
> }
> 
>-static gboolean
>-_delete (const McpAccountStorage *self,
>-      const McpAccountManager *am,
>-      const gchar *account,
>-      const gchar *key)
>+static void
>+delete_async (McpAccountStorage *self,
>+    McpAccountManager *am,
>+    const gchar *account,
>+    GAsyncReadyCallback callback,
>+    gpointer user_data)
> {
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>   McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
>+  GTask *task;
>+  gchar *filename = NULL;
>+  const gchar * const *iter;
>+
>+  task = g_task_new (amd, NULL, callback, user_data);
> 
>   if (sa == NULL || sa->absent)
>     {
>       /* Apparently we never had this account anyway. The plugin API
>        * considers this to be "success". */
>-      return TRUE;
>+      g_task_return_boolean (task, TRUE);
>+      goto finally;
>     }
> 
>-  if (key == NULL)
>+  filename = account_file_in (g_get_user_data_dir (), account);
>+
>+  DEBUG ("Deleting account %s from %s", account, filename);
>+
>+  if (g_unlink (filename) != 0)
>     {
>-      amd->save = TRUE;
>+      int e = errno;
> 
>-      /* flag the whole account as purged */
>-      sa->pending_deletion = TRUE;
>-      g_hash_table_remove_all (sa->attributes);
>-      g_hash_table_remove_all (sa->parameters);
>-      g_hash_table_remove_all (sa->untyped_parameters);
>+      /* ENOENT is OK, anything else is more upsetting */
>+      if (e != ENOENT)
>+        {
>+          WARNING ("Unable to delete %s: %s", filename,
>+              g_strerror (e));
>+          g_task_return_new_error (task, G_IO_ERROR, g_io_error_from_errno (e),
>+              "Unable to delete %s: %s", filename, g_strerror (e));
>+          goto finally;
>+        }
>     }
>-  else
>+
>+  for (iter = g_get_system_data_dirs ();
>+      iter != NULL && *iter != NULL;
>+      iter++)
>     {
>-      g_assert_not_reached ();
>+      gchar *other = account_file_in (*iter, account);
>+      gboolean other_exists = g_file_test (other, G_FILE_TEST_EXISTS);
>+
>+      g_free (other);
>+
>+      if (other_exists)
>+        {
>+          GError *error = NULL;
>+
>+          /* There is a lower-priority file that would provide this
>+           * account. We can't delete a file from XDG_DATA_DIRS which
>+           * are conceptually read-only, but we can mask it with an
>+           * empty file (prior art: systemd) */
>+          if (!g_file_set_contents (filename, "", 0, &error))
>+            {
>+              g_prefix_error (&error,
>+                  "Unable to save empty account file to %s: ", filename);
>+              WARNING ("%s", error->message);
>+              g_task_return_error (task, error);
>+              g_free (filename);
>+              goto finally;
>+            }
>+
>+          break;
>+        }
>     }
> 
>-  return TRUE;
>+  /* clean up the mess */
>+  g_hash_table_remove (amd->accounts, account);
>+  mcp_account_storage_emit_deleted (self, account);
>+
>+  g_task_return_boolean (task, TRUE);
>+
>+finally:
>+  g_free (filename);
>+  g_object_unref (task);
>+}
>+
>+static gboolean
>+delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *res,
>+    GError **error)
>+{
>+  return g_task_propagate_boolean (G_TASK (res), error);
> }
> 
> static gboolean
>@@ -397,58 +452,6 @@ am_default_commit_one (McdAccountManagerDefault *self,
> 
>   filename = account_file_in (g_get_user_data_dir (), account_name);
> 
>-  if (sa->pending_deletion)
>-    {
>-      const gchar * const *iter;
>-
>-      DEBUG ("Deleting account %s from %s", account_name, filename);
>-
>-      if (g_unlink (filename) != 0)
>-        {
>-          int e = errno;
>-
>-          /* ENOENT is OK, anything else is more upsetting */
>-          if (e != ENOENT)
>-            {
>-              WARNING ("Unable to delete %s: %s", filename,
>-                  g_strerror (e));
>-              g_free (filename);
>-              return FALSE;
>-            }
>-        }
>-
>-      for (iter = g_get_system_data_dirs ();
>-          iter != NULL && *iter != NULL;
>-          iter++)
>-        {
>-          gchar *other = account_file_in (*iter, account_name);
>-          gboolean other_exists = g_file_test (other, G_FILE_TEST_EXISTS);
>-
>-          g_free (other);
>-
>-          if (other_exists)
>-            {
>-              /* There is a lower-priority file that would provide this
>-               * account. We can't delete a file from XDG_DATA_DIRS which
>-               * are conceptually read-only, but we can mask it with an
>-               * empty file (prior art: systemd) */
>-              if (!g_file_set_contents (filename, "", 0, &error))
>-                {
>-                  WARNING ("Unable to save empty account file to %s: %s",
>-                      filename, error->message);
>-                  g_clear_error (&error);
>-                  g_free (filename);
>-                  return FALSE;
>-                }
>-
>-              break;
>-            }
>-        }
>-
>-      g_free (filename);
>-      return TRUE;
>-    }
>-
>   DEBUG ("Saving account %s to %s", account_name, filename);
> 
>   g_variant_builder_init (&attrs_builder, G_VARIANT_TYPE_VARDICT);
>@@ -544,17 +547,6 @@ _commit (const McpAccountStorage *self,
>       amd->save = FALSE;
>     }
> 
>-  g_hash_table_iter_init (&outer, amd->accounts);
>-
>-  /* forget about any entirely removed accounts */
>-  while (g_hash_table_iter_next (&outer, NULL, &sa_p))
>-    {
>-      McdDefaultStoredAccount *sa = sa_p;
>-
>-      if (sa->pending_deletion)
>-        g_hash_table_iter_remove (&outer);
>-    }
>-
>   return all_succeeded;
> }
> 
>@@ -982,7 +974,8 @@ account_storage_iface_init (McpAccountStorageIface *iface,
>   iface->set_attribute = set_attribute;
>   iface->set_parameter = set_parameter;
>   iface->create = _create;
>-  iface->delete = _delete;
>+  iface->delete_async = delete_async;
>+  iface->delete_finish = delete_finish;
>   iface->commit = _commit;
>   iface->list = _list;
> 
>diff --git a/src/mcd-account.c b/src/mcd-account.c
>index c1cae82..620487f 100644
>--- a/src/mcd-account.c
>+++ b/src/mcd-account.c
>@@ -765,8 +765,6 @@ mcd_account_delete_async (McdAccount *account,
>         g_free (data_dir_str);
>     }
> 
>-    mcd_storage_commit (priv->storage, name);
>-
>     if (!priv->removed)
>     {
>         DEBUG ("emitting Account.Removed for %s", name);
>diff --git a/src/mcd-storage.c b/src/mcd-storage.c
>index dbc04c3..25194b3 100644
>--- a/src/mcd-storage.c
>+++ b/src/mcd-storage.c
>@@ -1942,6 +1942,28 @@ mcd_storage_create_account (McdStorage *self,
>   return NULL;
> }
> 
>+static void
>+delete_cb (GObject *source,
>+    GAsyncResult *res,
>+    gpointer user_data)
>+{
>+  GError *error = NULL;
>+
>+  if (mcp_account_storage_delete_finish (MCP_ACCOUNT_STORAGE (source),
>+        res, &error))
>+    {
>+      DEBUG ("deleted account %s", (const gchar *) user_data);
>+    }
>+  else
>+    {
>+      DEBUG ("could not delete account %s (but no way to signal that): "
>+          "%s #%d: %s", (const gchar *) user_data,
>+          g_quark_to_string (error->domain), error->code, error->message);
>+      g_error_free (error);
>+    }
>+
>+  g_free (user_data);
>+}
> 
> /*
>  * mcd_storage_delete_account:
>@@ -1969,7 +1991,11 @@ mcd_storage_delete_account (McdStorage *self,
>     {
>       McpAccountStorage *plugin = store->data;
> 
>-      mcp_account_storage_delete (plugin, ma, account, NULL);
>+      /* FIXME: when we know which plugin owns the account, we can stop
>+       * ignoring the error (if any), and make this method async
>+       * in order to pass the error up to McdAccount */
>+      mcp_account_storage_delete_async (plugin, ma, account,
>+          delete_cb, g_strdup (account));
>     }
> }
> 
>diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c
>index 3ea5df9..01410a1 100644
>--- a/tests/twisted/dbus-account-plugin.c
>+++ b/tests/twisted/dbus-account-plugin.c
>@@ -54,7 +54,7 @@ typedef struct {
>     GHashTable *parameter_flags;
>     /* set of strings */
>     GHashTable *uncommitted_parameters;
>-    enum { UNCOMMITTED_CREATION, UNCOMMITTED_DELETION } flags;
>+    enum { UNCOMMITTED_CREATION = 1 } flags;
>     TpStorageRestrictionFlags restrictions;
> } Account;
> 
>@@ -183,7 +183,6 @@ ensure_account (TestDBusAccountPlugin *self,
>       g_hash_table_insert (self->accounts, g_strdup (account_name), account);
>     }
> 
>-  account->flags &= ~UNCOMMITTED_DELETION;
>   return account;
> }
> 
>@@ -211,18 +210,14 @@ service_vanished_cb (GDBusConnection *bus,
> {
>   TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (user_data);
>   GHashTableIter iter;
>-  gpointer k, v;
>+  gpointer k;
> 
>   self->active = FALSE;
>   g_hash_table_iter_init (&iter, self->accounts);
> 
>-  while (g_hash_table_iter_next (&iter, &k, &v))
>+  while (g_hash_table_iter_next (&iter, &k, NULL))
>     {
>-      Account *account = v;
>-
>-      if ((account->flags & UNCOMMITTED_DELETION) == 0)
>-        mcp_account_storage_emit_deleted (MCP_ACCOUNT_STORAGE (self), k);
>-
>+      mcp_account_storage_emit_deleted (MCP_ACCOUNT_STORAGE (self), k);
>       g_hash_table_iter_remove (&iter);
>     }
> 
>@@ -908,44 +903,64 @@ test_dbus_account_plugin_create (const McpAccountStorage *storage,
>   return name;
> }
> 
>-static gboolean
>-test_dbus_account_plugin_delete (const McpAccountStorage *storage,
>-    const McpAccountManager *am,
>+static void delete_account_cb (GObject *source_object,
>+    GAsyncResult *res,
>+    gpointer user_data);
>+
>+static void
>+test_dbus_account_plugin_delete_async (McpAccountStorage *storage,
>+    McpAccountManager *am,
>     const gchar *account_name,
>-    const gchar *key)
>+    GAsyncReadyCallback callback,
>+    gpointer user_data)
> {
>   TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage);
>   Account *account = lookup_account (self, account_name);
>+  GTask *task = g_task_new (self, NULL, callback, user_data);
>+
>+  g_task_set_task_data (task, g_strdup (user_data), g_free);
> 
>   DEBUG ("called");
> 
>   if (account == NULL || !self->active)
>-    return FALSE;
>-
>-  if (key == NULL)
>     {
>-      account->flags |= UNCOMMITTED_DELETION;
>-      g_hash_table_remove_all (account->attributes);
>-      g_hash_table_remove_all (account->parameters);
>-      g_hash_table_remove_all (account->untyped_parameters);
>-      g_hash_table_remove_all (account->attribute_flags);
>-      g_hash_table_remove_all (account->parameter_flags);
>+      /* We were asked to delete an account we don't have. It's
>+       * a bit like success. */
>+      g_task_return_boolean (task, TRUE);
>+      g_object_unref (task);
>+      return;
>+    }
> 
>-      account->flags &= ~UNCOMMITTED_CREATION;
>-      g_hash_table_remove_all (account->uncommitted_attributes);
>-      g_hash_table_remove_all (account->uncommitted_parameters);
>+  /* deletion used to be delayed, so the regression tests will expect this
>+   * to happen - leave them unmodified for now */
>+  g_dbus_connection_emit_signal (self->bus, NULL,
>+      TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
>+      "DeferringDelete", g_variant_new_parsed ("(%o,)", account->path),
>+      NULL);
>+  g_dbus_connection_emit_signal (self->bus, NULL,
>+      TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
>+      "CommittingOne", g_variant_new_parsed ("(%o,)", account->path), NULL);
> 
>-      g_dbus_connection_emit_signal (self->bus, NULL,
>-          TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
>-          "DeferringDelete", g_variant_new_parsed ("(%o,)", account->path),
>-          NULL);
>-    }
>-  else
>-    {
>-      g_assert_not_reached ();
>-    }
>+  g_dbus_connection_call (self->bus,
>+      TEST_DBUS_ACCOUNT_SERVICE,
>+      TEST_DBUS_ACCOUNT_SERVICE_PATH,
>+      TEST_DBUS_ACCOUNT_SERVICE_IFACE,
>+      "DeleteAccount",
>+      g_variant_new_parsed ("(%s,)", account_name),
>+      G_VARIANT_TYPE_UNIT,
>+      G_DBUS_CALL_FLAGS_NONE,
>+      -1,
>+      NULL, /* no cancellable */
>+      delete_account_cb,
>+      task);
>+}
> 
>-  return TRUE;
>+static gboolean
>+test_dbus_account_plugin_delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *res,
>+    GError **error)
>+{
>+  return g_task_propagate_boolean (G_TASK (res), error);
> }
> 
> static gboolean
>@@ -957,7 +972,7 @@ test_dbus_account_plugin_get (const McpAccountStorage *storage,
>   TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage);
>   Account *account = lookup_account (self, account_name);
> 
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return FALSE;
> 
>   if (key == NULL)
>@@ -1083,8 +1098,7 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage,
> 
>   DEBUG ("%s of %s", attribute, account_name);
> 
>-  if (!self->active || account == NULL ||
>-      (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return FALSE;
> 
>   if (value == NULL)
>@@ -1132,8 +1146,7 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage,
> 
>   DEBUG ("%s of %s", parameter, account_name);
> 
>-  if (!self->active || account == NULL ||
>-      (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return FALSE;
> 
>   if (value == NULL)
>@@ -1202,27 +1215,31 @@ delete_account_cb (GObject *source_object,
>     GAsyncResult *res,
>     gpointer user_data)
> {
>-  AsyncData *ad = user_data;
>+  GTask *task = user_data;
>   GVariant *tuple;
>   GError *error = NULL;
>+  TestDBusAccountPlugin *self = g_task_get_source_object (task);
>+  const gchar *account_name = g_task_get_task_data (task);
> 
>-  tuple = g_dbus_connection_call_finish (ad->self->bus, res, &error);
>+  tuple = g_dbus_connection_call_finish (self->bus, res, &error);
> 
>   if (tuple != NULL)
>     {
>-      g_hash_table_remove (ad->self->accounts, ad->account_name);
>+      /* we'll emit ::deleted when we see the signal, which probably
>+       * already happened */
>+      g_hash_table_remove (self->accounts, account_name);
>       g_variant_unref (tuple);
>+      g_task_return_boolean (task, TRUE);
>     }
>   else
>     {
>-      g_warning ("Unable to delete account %s: %s", ad->account_name,
>-          error->message);
>-      g_clear_error (&error);
>-      /* FIXME: we could roll back the deletion by claiming that
>-       * the service re-created the account? */
>+      g_prefix_error (&error, "Unable to delete account %s: ",
>+          account_name);
>+      g_warning ("%s", error->message);
>+      g_task_return_error (task, error);
>     }
> 
>-  async_data_free (ad);
>+  g_object_unref (task);
> }
> 
> static void
>@@ -1355,25 +1372,6 @@ test_dbus_account_plugin_commit (const McpAccountStorage *storage,
>       TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
>       "CommittingOne", g_variant_new_parsed ("(%o,)", account->path), NULL);
> 
>-  if (account->flags & UNCOMMITTED_DELETION)
>-    {
>-      g_dbus_connection_call (self->bus,
>-          TEST_DBUS_ACCOUNT_SERVICE,
>-          TEST_DBUS_ACCOUNT_SERVICE_PATH,
>-          TEST_DBUS_ACCOUNT_SERVICE_IFACE,
>-          "DeleteAccount",
>-          g_variant_new_parsed ("(%s,)", account_name),
>-          G_VARIANT_TYPE_UNIT,
>-          G_DBUS_CALL_FLAGS_NONE,
>-          -1,
>-          NULL, /* no cancellable */
>-          delete_account_cb,
>-          async_data_new (self, account_name));
>-
>-      /* this doesn't mean we succeeded: it means we tried */
>-      return TRUE;
>-    }
>-
>   if (account->flags & UNCOMMITTED_CREATION)
>     {
>       g_dbus_connection_call (self->bus,
>@@ -1509,7 +1507,7 @@ test_dbus_account_plugin_get_identifier (const McpAccountStorage *storage,
> 
>   DEBUG ("%s", account_name);
> 
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return;
> 
>   /* Our "library-specific unique identifier" is just the object-path
>@@ -1528,7 +1526,7 @@ test_dbus_account_plugin_get_additional_info (const McpAccountStorage *storage,
> 
>   DEBUG ("%s", account_name);
> 
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return NULL;
> 
>   ret = g_hash_table_new_full (g_str_hash, g_str_equal,
>@@ -1548,7 +1546,7 @@ test_dbus_account_plugin_get_restrictions (const McpAccountStorage *storage,
> 
>   DEBUG ("%s", account_name);
> 
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return 0;
> 
>   return account->restrictions;
>@@ -1564,7 +1562,7 @@ test_dbus_account_plugin_owns (McpAccountStorage *storage,
> 
>   DEBUG ("%s", account_name);
> 
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>+  if (!self->active || account == NULL)
>     return FALSE;
> 
>   return TRUE;
>@@ -1583,7 +1581,8 @@ account_storage_iface_init (McpAccountStorageIface *iface)
>   iface->set_parameter = test_dbus_account_plugin_set_parameter;
>   iface->list = test_dbus_account_plugin_list;
>   iface->ready = test_dbus_account_plugin_ready;
>-  iface->delete = test_dbus_account_plugin_delete;
>+  iface->delete_async = test_dbus_account_plugin_delete_async;
>+  iface->delete_finish = test_dbus_account_plugin_delete_finish;
>   iface->commit = test_dbus_account_plugin_commit;
>   iface->get_identifier = test_dbus_account_plugin_get_identifier;
>   iface->get_additional_info = test_dbus_account_plugin_get_additional_info;
>diff --git a/tests/twisted/mcp-account-diversion.c b/tests/twisted/mcp-account-diversion.c
>index 9639faf..8e210d6 100644
>--- a/tests/twisted/mcp-account-diversion.c
>+++ b/tests/twisted/mcp-account-diversion.c
>@@ -223,27 +223,45 @@ _get (const McpAccountStorage *self,
>   return TRUE;
> }
> 
>-static gboolean
>-_delete (const McpAccountStorage *self,
>-      const McpAccountManager *am,
>-      const gchar *account,
>-      const gchar *key)
>+static gboolean _commit (const McpAccountStorage *self,
>+    const McpAccountManager *am,
>+    const gchar *account_name);
>+
>+static void
>+delete_async (McpAccountStorage *self,
>+    McpAccountManager *am,
>+    const gchar *account,
>+    GAsyncReadyCallback callback,
>+    gpointer user_data)
> {
>   AccountDiversionPlugin *adp = ACCOUNT_DIVERSION_PLUGIN (self);
>+  GTask *task = g_task_new (adp, NULL, callback, user_data);
> 
>-  if (key == NULL)
>+  if (g_key_file_remove_group (adp->keyfile, account, NULL))
>+    adp->save = TRUE;
>+
>+  if (_commit (self, am, account))
>     {
>-      if (g_key_file_remove_group (adp->keyfile, account, NULL))
>-        adp->save = TRUE;
>+      mcp_account_storage_emit_deleted (self, account);
>+      g_task_return_boolean (task, TRUE);
>     }
>   else
>     {
>-      g_assert_not_reached ();
>+      g_task_return_new_error (task, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
>+          "_commit()'s error handling is not good enough to know why "
>+          "I couldn't commit the deletion of %s", account);
>     }
> 
>-  return TRUE;
>+  g_object_unref (task);
> }
> 
>+static gboolean
>+delete_finish (McpAccountStorage *storage,
>+    GAsyncResult *res,
>+    GError **error)
>+{
>+  return g_task_propagate_boolean (G_TASK (res), error);
>+}
> 
> static gboolean
> _commit (const McpAccountStorage *self,
>@@ -310,7 +328,8 @@ account_storage_iface_init (McpAccountStorageIface *iface,
>   iface->get = _get;
>   iface->set_attribute = _set_attribute;
>   iface->set_parameter = _set_parameter;
>-  iface->delete = _delete;
>+  iface->delete_async = delete_async;
>+  iface->delete_finish = delete_finish;
>   iface->commit = _commit;
>   iface->list = _list;
> }
>-- 
>1.8.4.3
>

Actions: View | Splinter Review

Attachments on bug 27727: 35148 | 89208 | 89209 | 89210 | 89211 | 89212 | 89213 | 89214 | 89215 | 89216 | 89217 | 89218 | 89219 | 89220 | 89221 | 89222 | 89223 | 89224 | 89225 | 89226 | 89227 | 89228 | 89229 | 89230 | 89231 | 89232 | 89233 | 89237

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.