mcp_account_storage_commit: guarantee that we commit one at a time

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=27727

4 files changed, 36 insertions, 78 deletions

diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c
index 338003c0..e30bb51e 100644
--- a/mission-control-plugins/account-storage.c
+++ b/mission-control-plugins/account-storage.c
@@ -743,8 +743,7 @@ mcp_account_storage_delete_finish (McpAccountStorage *storage,
  * McpAccountStorageCommitFunc:
  * @storage: an #McpAccountStorage instance
  * @am: an #McpAccountManager instance
- * @account: (allow-none): the unique suffix of an account's object path,
- *  or %NULL to commit all accounts
+ * @account: the unique suffix of an account's object path
  *
  * An implementation of mcp_account_storage_commit().
  *
@@ -755,8 +754,7 @@ mcp_account_storage_delete_finish (McpAccountStorage *storage,
  * mcp_account_storage_commit:
  * @storage: an #McpAccountStorage instance
  * @am: an #McpAccountManager instance
- * @account: (allow-none): the unique suffix of an account's object path,
- *  or %NULL if all accounts are to be committed
+ * @account: the unique suffix of an account's object path
  *
  * The plugin is expected to write its cache to long term storage,
  * deleting, adding or updating entries in said storage as needed.
@@ -768,6 +766,9 @@ mcp_account_storage_delete_finish (McpAccountStorage *storage,
  * The default implementation just returns %FALSE, and is appropriate for
  * read-only storage.
  *
+ * Mission Control 5.17+ no longer requires plugins to cope with
+ * account == NULL.
+ *
  * Returns: %TRUE if the commit process was started (but not necessarily
  * completed) successfully; %FALSE if there was a problem that was immediately
  * obvious.
diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
index fed283b1..efcb19f4 100644
--- a/src/mcd-account-manager-default.c
+++ b/src/mcd-account-manager-default.c
@@ -454,6 +454,13 @@ am_default_commit_one (McdAccountManagerDefault *self,
   if (!sa->dirty)
     return TRUE;
 
+  if (!mcd_ensure_directory (self->directory, &error))
+    {
+      g_warning ("%s", error->message);
+      g_error_free (error);
+      return FALSE;
+    }
+
   filename = account_file_in (g_get_user_data_dir (), account_name);
 
   DEBUG ("Saving account %s to %s", account_name, filename);
@@ -518,48 +525,14 @@ _commit (const McpAccountStorage *self,
     const gchar *account)
 {
   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
-  gboolean all_succeeded = TRUE;
-  GError *error = NULL;
-  GHashTableIter outer;
-  gpointer account_p, sa_p;
-
-  DEBUG ("Saving accounts to %s", amd->directory);
-
-  if (!mcd_ensure_directory (amd->directory, &error))
-    {
-      g_warning ("%s", error->message);
-      g_clear_error (&error);
-      /* fall through anyway: writing to the files will fail, but it does
-       * give us a chance to commit to the keyring too */
-    }
-
-  if (account != NULL)
-    {
-      McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
-
-      g_return_val_if_fail (sa != NULL, FALSE);
-      g_return_val_if_fail (!sa->absent, FALSE);
-
-      return am_default_commit_one (amd, account, sa);
-    }
-
-  g_hash_table_iter_init (&outer, amd->accounts);
-
-  while (g_hash_table_iter_next (&outer, &account_p, &sa_p))
-    {
-      if (account == NULL || !tp_strdiff (account, account_p))
-        {
-          McdDefaultStoredAccount *sa = sa_p;
+  McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
 
-          if (sa->absent)
-            continue;
+  g_return_val_if_fail (sa != NULL, FALSE);
+  g_return_val_if_fail (!sa->absent, FALSE);
 
-          if (!am_default_commit_one (amd, account_p, sa_p))
-            all_succeeded = FALSE;
-        }
-    }
+  DEBUG ("Saving account %s to %s", account, amd->directory);
 
-  return all_succeeded;
+  return am_default_commit_one (amd, account, sa);
 }
 
 static gboolean
@@ -963,9 +936,24 @@ _list (const McpAccountStorage *self,
 
   if (save)
     {
+      gboolean all_succeeded = TRUE;
+
       DEBUG ("Saving initial or migrated account data");
 
-      if (_commit (self, am, NULL))
+      g_hash_table_iter_init (&hash_iter, amd->accounts);
+
+      while (g_hash_table_iter_next (&hash_iter, &k, &v))
+        {
+          McdDefaultStoredAccount *sa = v;
+
+          if (sa->absent)
+            continue;
+
+          if (!am_default_commit_one (amd, k, v))
+            all_succeeded = FALSE;
+        }
+
+      if (all_succeeded)
         {
           if (migrate_from != NULL)
             {
diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c
index c7c5dbde..2e66179e 100644
--- a/tests/twisted/dbus-account-plugin.c
+++ b/tests/twisted/dbus-account-plugin.c
@@ -1151,36 +1151,6 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage,
   return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED;
 }
 
-static gboolean
-test_dbus_account_plugin_commit_all (const McpAccountStorage *storage,
-    const McpAccountManager *am)
-{
-  TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage);
-  GHashTableIter iter;
-  gpointer k;
-
-  DEBUG ("called");
-
-  if (!self->active)
-    return FALSE;
-
-  g_dbus_connection_emit_signal (self->bus, NULL,
-      TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
-      "CommittingAll", NULL, NULL);
-
-  g_hash_table_iter_init (&iter, self->accounts);
-
-  while (g_hash_table_iter_next (&iter, &k, NULL))
-    {
-      if (!mcp_account_storage_commit (storage, am, k))
-        {
-          g_warning ("declined to commit account %s", (const gchar *) k);
-        }
-    }
-
-  return TRUE;
-}
-
 static void
 delete_account_cb (GObject *source_object,
     GAsyncResult *res,
@@ -1331,9 +1301,6 @@ test_dbus_account_plugin_commit (const McpAccountStorage *storage,
 
   DEBUG ("%s", account_name);
 
-  if (account_name == NULL)
-    return test_dbus_account_plugin_commit_all (storage, am);
-
   account = lookup_account (self, account_name);
 
   g_return_val_if_fail (self->active, FALSE);
diff --git a/tests/twisted/mcp-account-diversion.c b/tests/twisted/mcp-account-diversion.c
index bcb8930c..02775b4c 100644
--- a/tests/twisted/mcp-account-diversion.c
+++ b/tests/twisted/mcp-account-diversion.c
@@ -302,7 +302,9 @@ _commit (const McpAccountStorage *self,
   gboolean rval = FALSE;
 
   /* This simple implementation ignores account_name and commits everything:
-   * we're writing out the whole keyfile anyway */
+   * we're writing out the whole keyfile anyway. If MC is looping over
+   * accounts, the second and subsequent accounts will find that
+   * adp->save is false, so there's no write-amplification. */
 
   if (!adp->save)
     return TRUE;