Attachment 88297 Details for Bug 54875 – Default account backend: store each account as a stringified GVariant in a file

[patch] Default account backend: store each account as a stringified GVariant in a file

0007-Default-account-backend-store-each-account-as-a-stri.patch (text/plain), 30.70 KB, created by Simon McVittie on 2013-10-29 14:25:47 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-10-29 14:25:47 UTC

Size: 30.70 KB

patch

obsolete

>From 51889443a146519806e7ef801923f0e9d55dff84 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 29 Oct 2013 13:35:43 +0000
>Subject: [PATCH 07/12] Default account backend: store each account as a
> stringified GVariant in a file
>
>---
> src/mcd-account-manager-default.c                  | 510 +++++++++++++++++----
> src/mcd-account-manager-default.h                  |   2 +-
> .../account-storage/default-keyring-storage.py     | 108 +++--
> tests/twisted/account-storage/diverted-storage.py  |   4 -
> 4 files changed, 478 insertions(+), 146 deletions(-)
>
>diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
>index 6bbea0e..5da0e50 100644
>--- a/src/mcd-account-manager-default.c
>+++ b/src/mcd-account-manager-default.c
>@@ -1,8 +1,8 @@
> /*
>- * The default account manager keyfile storage pseudo-plugin
>+ * The default account manager storage pseudo-plugin
>  *
>  * Copyright Â© 2010 Nokia Corporation
>- * Copyright Â© 2010 Collabora Ltd.
>+ * Copyright Â© 2010-2012 Collabora Ltd.
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>@@ -32,20 +32,22 @@
> #include "mcd-debug.h"
> #include "mcd-misc.h"
> 
>-#define PLUGIN_NAME "default-gkeyfile"
>+#define PLUGIN_NAME "default"
> #define PLUGIN_PRIORITY MCP_ACCOUNT_STORAGE_PLUGIN_PRIO_DEFAULT
>-#define PLUGIN_DESCRIPTION "GKeyFile (default) account storage backend"
>-#define INITIAL_CONFIG "# Telepathy accounts\n"
>+#define PLUGIN_DESCRIPTION "Default account storage backend"
> 
> typedef struct {
>     /* owned string, attribute => owned string, value
>-     * attributes to be stored in the keyfile */
>+     * attributes to be stored in the variant-file */
>     GHashTable *attributes;
>     /* owned string, parameter (without "param-") => owned string, value
>-     * parameters to be stored in the keyfile */
>+     * parameters 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;
> } McdDefaultStoredAccount;
> 
> static McdDefaultStoredAccount *
>@@ -72,6 +74,7 @@ ensure_stored_account (McdAccountManagerDefault *self,
>     }
> 
>   sa->pending_deletion = FALSE;
>+  sa->absent = FALSE;
>   return sa;
> }
> 
>@@ -113,17 +116,37 @@ get_old_filename (void)
> }
> 
> static gchar *
>-account_filename_in (const gchar *dir)
>+accounts_cfg_in (const gchar *dir)
> {
>   return g_build_filename (dir, "telepathy", "mission-control", "accounts.cfg",
>       NULL);
> }
> 
>+static gchar *
>+account_directory_in (const gchar *dir)
>+{
>+  return g_build_filename (dir, "telepathy", "mission-control", NULL);
>+}
>+
>+static gchar *
>+account_file_in (const gchar *dir,
>+    const gchar *account)
>+{
>+  gchar *basename = g_strdup_printf ("%s.account", account);
>+  gchar *ret;
>+
>+  g_strdelimit (basename, "/", '-');
>+  ret = g_build_filename (dir, "telepathy", "mission-control",
>+      basename, NULL);
>+  g_free (basename);
>+  return ret;
>+}
>+
> static void
> mcd_account_manager_default_init (McdAccountManagerDefault *self)
> {
>   DEBUG ("mcd_account_manager_default_init");
>-  self->filename = account_filename_in (g_get_user_data_dir ());
>+  self->directory = account_directory_in (g_get_user_data_dir ());
>   self->accounts = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,
>       stored_account_free);
>   self->save = FALSE;
>@@ -136,8 +159,7 @@ mcd_account_manager_default_class_init (McdAccountManagerDefaultClass *cls)
>   DEBUG ("mcd_account_manager_default_class_init");
> }
> 
>-/* We happen to know that the string MC gave us is "sufficiently escaped" to
>- * put it in the keyfile as-is. */
>+/* The value is escaped as if for a keyfile */
> static gboolean
> set_parameter (const McpAccountStorage *self,
>     const McpAccountManager *am,
>@@ -213,7 +235,7 @@ get_parameter (const McpAccountStorage *self,
>     {
>       gchar *v = NULL;
> 
>-      if (sa == NULL)
>+      if (sa == NULL || sa->absent)
>         return FALSE;
> 
>       v = g_hash_table_lookup (sa->untyped_parameters, parameter);
>@@ -222,7 +244,6 @@ get_parameter (const McpAccountStorage *self,
>         return FALSE;
> 
>       mcp_account_manager_set_value (am, account, prefixed, v);
>-      g_free (v);
>     }
>   else
>     {
>@@ -241,7 +262,7 @@ _get (const McpAccountStorage *self,
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>   McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
> 
>-  if (sa == NULL)
>+  if (sa == NULL || sa->absent)
>     return FALSE;
> 
>   if (key != NULL)
>@@ -259,7 +280,6 @@ _get (const McpAccountStorage *self,
>         return FALSE;
> 
>       mcp_account_manager_set_value (am, account, key, v);
>-      g_free (v);
>     }
>   else
>     {
>@@ -321,7 +341,7 @@ _delete (const McpAccountStorage *self,
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>   McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
> 
>-  if (sa == NULL)
>+  if (sa == NULL || sa->absent)
>     {
>       /* Apparently we never had this account anyway. The plugin API
>        * considers this to be "success". */
>@@ -362,84 +382,159 @@ _delete (const McpAccountStorage *self,
>   return TRUE;
> }
> 
>+static gboolean
>+am_default_commit_one (McdAccountManagerDefault *self,
>+    const gchar *account_name,
>+    McdDefaultStoredAccount *sa)
>+{
>+  gchar *filename;
>+  GHashTableIter inner;
>+  gpointer k, v;
>+  GVariantBuilder params_builder;
>+  GVariantBuilder attrs_builder;
>+  GVariant *content;
>+  gchar *content_text;
>+  gboolean ret;
>+  GError *error = NULL;
>+
>+  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);
>+
>+          if (g_file_test (other, G_FILE_TEST_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);
>+
>+  g_hash_table_iter_init (&inner, sa->attributes);
>+
>+  while (g_hash_table_iter_next (&inner, &k, &v))
>+    {
>+      /* FIXME: for now, we put the keyfile-escaped value in a string,
>+       * and store that. This needs fixing to use typed attributes
>+       * before this code gets released. */
>+      g_variant_builder_add (&attrs_builder, "{sv}",
>+          k, g_variant_new_string (v));
>+    }
>+
>+  g_variant_builder_init (&params_builder, G_VARIANT_TYPE ("a{ss}"));
>+  g_hash_table_iter_init (&inner, sa->untyped_parameters);
>+
>+  while (g_hash_table_iter_next (&inner, &k, &v))
>+    {
>+      g_variant_builder_add (&params_builder, "{ss}", k, v);
>+    }
>+
>+  g_variant_builder_add (&attrs_builder, "{sv}",
>+      "KeyFileParameters", g_variant_builder_end (&params_builder));
>+
>+  content = g_variant_ref_sink (g_variant_builder_end (&attrs_builder));
>+  content_text = g_variant_print (content, TRUE);
>+  DEBUG ("%s", content_text);
>+  g_variant_unref (content);
>+
>+  if (g_file_set_contents (filename, content_text, -1, &error))
>+    {
>+      ret = TRUE;
>+    }
>+  else
>+    {
>+      WARNING ("Unable to save account to %s: %s", filename,
>+          error->message);
>+      g_clear_error (&error);
>+      ret = FALSE;
>+    }
>+
>+  g_free (filename);
>+  g_free (content_text);
>+  return ret;
>+}
> 
> static gboolean
> _commit (const McpAccountStorage *self,
>     const McpAccountManager *am,
>     const gchar *account)
> {
>-  gsize n;
>-  gchar *data;
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>-  gboolean rval = FALSE;
>-  gchar *dir;
>+  gboolean all_succeeded = TRUE;
>   GError *error = NULL;
>   GHashTableIter outer;
>   gpointer account_p, sa_p;
>-  GKeyFile *keyfile;
> 
>   if (!amd->save)
>     return TRUE;
> 
>-  dir = g_path_get_dirname (amd->filename);
>+  DEBUG ("Saving accounts to %s", amd->directory);
> 
>-  DEBUG ("Saving accounts to %s", amd->filename);
>-
>-  if (!mcd_ensure_directory (dir, &error))
>+  if (!mcd_ensure_directory (amd->directory, &error))
>     {
>       g_warning ("%s", error->message);
>       g_error_free (error);
>-      /* fall through anyway: writing to the file will fail, but it does
>+      /* fall through anyway: writing to the files will fail, but it does
>        * give us a chance to commit to the keyring too */
>     }
> 
>-  g_free (dir);
>-
>-  keyfile = g_key_file_new ();
>-
>   g_hash_table_iter_init (&outer, amd->accounts);
> 
>   while (g_hash_table_iter_next (&outer, &account_p, &sa_p))
>     {
>-      McdDefaultStoredAccount *sa = sa_p;
>-      GHashTableIter inner;
>-      gpointer k, v;
>-
>-      /* don't save accounts that are being deleted */
>-      if (sa->pending_deletion)
>-        continue;
>-
>-      g_hash_table_iter_init (&inner, sa->attributes);
>-
>-      while (g_hash_table_iter_next (&inner, &k, &v))
>-        g_key_file_set_value (keyfile, account_p, k, v);
>-
>-      g_hash_table_iter_init (&inner, sa->untyped_parameters);
>-
>-      while (g_hash_table_iter_next (&inner, &k, &v))
>+      if (account == NULL || !tp_strdiff (account, account_p))
>         {
>-          gchar *prefixed = g_strdup_printf ("param-%s", (const gchar *) k);
>-
>-          g_key_file_set_value (keyfile, account_p, prefixed, v);
>-          g_free (prefixed);
>+          if (!am_default_commit_one (amd, account_p, sa_p))
>+            all_succeeded = FALSE;
>         }
>     }
> 
>-  data = g_key_file_to_data (keyfile, &n, NULL);
>-  rval = g_file_set_contents (amd->filename, data, n, &error);
>-
>-  if (rval)
>+  if (all_succeeded)
>     {
>       amd->save = FALSE;
>     }
>-  else
>-    {
>-      g_warning ("%s", error->message);
>-      g_error_free (error);
>-    }
>-
>-  g_free (data);
>-  g_key_file_unref (keyfile);
> 
>   g_hash_table_iter_init (&outer, amd->accounts);
> 
>@@ -452,10 +547,10 @@ _commit (const McpAccountStorage *self,
>         g_hash_table_iter_remove (&outer);
>     }
> 
>-  return rval;
>+  return all_succeeded;
> }
> 
>-static void
>+static gboolean
> am_default_load_keyfile (McdAccountManagerDefault *self,
>     const gchar *filename)
> {
>@@ -464,6 +559,13 @@ am_default_load_keyfile (McdAccountManagerDefault *self,
>   gsize i;
>   gsize n = 0;
>   GStrv account_tails;
>+  gboolean all_ok = TRUE;
>+
>+  /* We shouldn't call this function without modification if we think we've
>+   * migrated to a newer storage format, because it doesn't check whether
>+   * each account has already been loaded. */
>+  g_assert (!self->loaded);
>+  g_assert (g_hash_table_size (self->accounts) == 0);
> 
>   if (g_key_file_load_from_file (keyfile, filename,
>         G_KEY_FILE_KEEP_COMMENTS, &error))
>@@ -475,12 +577,10 @@ am_default_load_keyfile (McdAccountManagerDefault *self,
>       DEBUG ("Failed to load accounts from %s: %s", filename, error->message);
>       g_error_free (error);
> 
>-      /* Start with a blank configuration, but do not save straight away;
>-       * we don't want to overwrite a corrupt-but-maybe-recoverable
>-       * configuration file with an empty one until given a reason to
>-       * do so. */
>-      g_key_file_load_from_data (keyfile, INITIAL_CONFIG, -1,
>-          G_KEY_FILE_KEEP_COMMENTS, NULL);
>+      /* Start with a blank configuration. */
>+      g_key_file_load_from_data (keyfile, "", -1, 0, NULL);
>+      /* Don't delete the old file, which might be recoverable. */
>+      all_ok = FALSE;
>     }
> 
>   account_tails = g_key_file_get_groups (keyfile, &n);
>@@ -493,6 +593,9 @@ am_default_load_keyfile (McdAccountManagerDefault *self,
>       gsize m = 0;
>       GStrv keys = g_key_file_get_keys (keyfile, account, &m, NULL);
> 
>+      /* We're going to need to migrate this account. */
>+      self->save = TRUE;
>+
>       for (j = 0; j < m; j++)
>         {
>           gchar *key = keys[j];
>@@ -516,6 +619,179 @@ am_default_load_keyfile (McdAccountManagerDefault *self,
> 
>   g_strfreev (account_tails);
>   g_key_file_unref (keyfile);
>+  return all_ok;
>+}
>+
>+static void
>+am_default_load_variant_file (McdAccountManagerDefault *self,
>+    const gchar *account_tail,
>+    const gchar *full_name)
>+{
>+  McdDefaultStoredAccount *sa;
>+  gchar *text = NULL;
>+  gsize len;
>+  GVariant *contents = NULL;
>+  GVariantIter iter;
>+  const gchar *k;
>+  GVariant *v;
>+  GError *error = NULL;
>+
>+  DEBUG ("%s from %s", account_tail, full_name);
>+
>+  sa = lookup_stored_account (self, account_tail);
>+
>+  if (sa != NULL)
>+    {
>+      DEBUG ("Ignoring %s: account %s already %s",
>+          full_name, account_tail, sa->absent ? "masked" : "loaded");
>+      goto finally;
>+    }
>+
>+  if (!g_file_get_contents (full_name, &text, &len, &error))
>+    {
>+      WARNING ("Unable to read account %s from %s: %s",
>+          account_tail, full_name, error->message);
>+      g_error_free (error);
>+      goto finally;
>+    }
>+
>+  if (len == 0)
>+    {
>+      DEBUG ("Empty file %s masks account %s", full_name, account_tail);
>+      ensure_stored_account (self, account_tail)->absent = TRUE;
>+      goto finally;
>+    }
>+
>+  contents = g_variant_parse (G_VARIANT_TYPE_VARDICT,
>+      text, text + len, NULL, &error);
>+
>+  if (contents == NULL)
>+    {
>+      WARNING ("Unable to parse account %s from %s: %s",
>+          account_tail, full_name, error->message);
>+      g_error_free (error);
>+      goto finally;
>+    }
>+
>+  sa = ensure_stored_account (self, account_tail);
>+
>+  g_variant_iter_init (&iter, contents);
>+
>+  while (g_variant_iter_loop (&iter, "{sv}", &k, &v))
>+    {
>+      if (!tp_strdiff (k, "KeyFileParameters"))
>+        {
>+          GVariantIter param_iter;
>+          gchar *parameter;
>+          gchar *param_value;
>+
>+          if (!g_variant_is_of_type (v, G_VARIANT_TYPE ("a{ss}")))
>+            {
>+              gchar *repr = g_variant_print (v, TRUE);
>+
>+              WARNING ("invalid KeyFileParameters found in %s, "
>+                  "ignoring: %s", full_name, repr);
>+              g_free (repr);
>+              continue;
>+            }
>+
>+          g_variant_iter_init (&param_iter, v);
>+
>+          while (g_variant_iter_next (&param_iter, "{ss}", &parameter,
>+                &param_value))
>+            {
>+              /* steals parameter, param_value */
>+              g_hash_table_insert (sa->untyped_parameters, parameter,
>+                  param_value);
>+            }
>+        }
>+      else
>+        {
>+          /* an ordinary attribute */
>+          /* FIXME: this is temporary and should not be released:
>+           * it assumes that all attributes are keyfile-escaped
>+           * strings */
>+          if (g_variant_is_of_type (v, G_VARIANT_TYPE_STRING))
>+            {
>+              g_hash_table_insert (sa->attributes,
>+                  g_strdup (k), g_variant_dup_string (v, NULL));
>+            }
>+          else
>+            {
>+              gchar *repr = g_variant_print (v, TRUE);
>+
>+              WARNING ("Not a string: %s", repr);
>+              g_free (repr);
>+            }
>+        }
>+    }
>+
>+finally:
>+  tp_clear_pointer (&contents, g_variant_unref);
>+  g_free (text);
>+}
>+
>+static void
>+am_default_load_directory (McdAccountManagerDefault *self,
>+    const gchar *directory)
>+{
>+  GDir *dir_handle;
>+  const gchar *basename;
>+  GRegex *regex;
>+  GError *error = NULL;
>+
>+  dir_handle = g_dir_open (directory, 0, &error);
>+
>+  if (dir_handle == NULL)
>+    {
>+      /* We expect ENOENT. Anything else is a cause for (minor) concern. */
>+      if (g_error_matches (error, G_FILE_ERROR, G_FILE_ERROR_NOENT))
>+        DEBUG ("%s", error->message);
>+      else
>+        WARNING ("%s", error->message);
>+
>+      g_error_free (error);
>+      return;
>+    }
>+
>+  DEBUG ("Looking for accounts in %s", directory);
>+
>+  regex = g_regex_new ("^[A-Za-z][A-Za-z0-9_]*-"  /* CM name */
>+      "[A-Za-z][A-Za-z0-9_]*-"                    /* protocol with s/-/_/ */
>+      "[A-Za-z_][A-Za-z0-9_]*\\.account$",        /* account-specific part */
>+      G_REGEX_DOLLAR_ENDONLY, 0, &error);
>+  g_assert_no_error (error);
>+
>+  while ((basename = g_dir_read_name (dir_handle)) != NULL)
>+    {
>+      gchar *full_name;
>+      gchar *account_tail;
>+
>+      /* skip it silently if it's obviously not an account */
>+      if (!g_str_has_suffix (basename, ".account"))
>+        continue;
>+
>+      /* We consider ourselves to have migrated to the new storage format
>+       * as soon as we find something that looks as though it ought to be an
>+       * account. */
>+      self->loaded = TRUE;
>+
>+      if (!g_regex_match (regex, basename, 0, NULL))
>+        {
>+          WARNING ("Ignoring %s/%s: not a syntactically valid account",
>+              directory, basename);
>+        }
>+
>+      full_name = g_build_filename (directory, basename, NULL);
>+      account_tail = g_strdup (basename);
>+      g_strdelimit (account_tail, "-", '/');
>+      g_strdelimit (account_tail, ".", '\0');
>+
>+      am_default_load_variant_file (self, account_tail, full_name);
>+
>+      g_free (account_tail);
>+      g_free (full_name);
>+    }
> }
> 
> static GList *
>@@ -525,16 +801,47 @@ _list (const McpAccountStorage *self,
>   GList *rval = NULL;
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>   GHashTableIter hash_iter;
>+  gchar *migrate_from = NULL;
>   gpointer k, v;
> 
>-  if (!amd->loaded && g_file_test (amd->filename, G_FILE_TEST_EXISTS))
>+  if (!amd->loaded)
>     {
>-      /* If the file exists, but loading it fails, we deliberately
>-       * do not fall through to the "initial configuration" case,
>-       * because we don't want to overwrite a corrupted file
>-       * with an empty one until an actual write takes place. */
>-      am_default_load_keyfile (amd, amd->filename);
>-      amd->loaded = TRUE;
>+      const gchar * const *iter;
>+
>+      am_default_load_directory (amd, amd->directory);
>+
>+      /* We do this even if am_default_load_directory() succeeded, and
>+       * do not stop when amd->loaded becomes true. If XDG_DATA_HOME
>+       * contains gabble-jabber-example_2eexample_40com.account, that doesn't
>+       * mean a directory in XDG_DATA_DIRS doesn't also contain
>+       * haze-msn-example_2ehotmail_40com.account or something, which
>+       * should also be loaded. */
>+      for (iter = g_get_system_data_dirs ();
>+          iter != NULL && *iter != NULL;
>+          iter++)
>+        {
>+          gchar *dir = account_directory_in (*iter);
>+
>+          am_default_load_directory (amd, dir);
>+          g_free (dir);
>+        }
>+    }
>+
>+  if (!amd->loaded)
>+    {
>+      migrate_from = accounts_cfg_in (g_get_user_data_dir ());
>+
>+      if (g_file_test (migrate_from, G_FILE_TEST_EXISTS))
>+        {
>+          if (!am_default_load_keyfile (amd, migrate_from))
>+            tp_clear_pointer (&migrate_from, g_free);
>+
>+          amd->loaded = TRUE;
>+        }
>+      else
>+        {
>+          tp_clear_pointer (&migrate_from, g_free);
>+        }
>     }
> 
>   if (!amd->loaded)
>@@ -545,14 +852,14 @@ _list (const McpAccountStorage *self,
>           iter != NULL && *iter != NULL;
>           iter++)
>         {
>-          gchar *filename = account_filename_in (*iter);
>+          /* not setting migrate_from here - XDG_DATA_DIRS are conceptually
>+           * read-only, so we don't want to delete these files */
>+          gchar *filename = accounts_cfg_in (*iter);
> 
>           if (g_file_test (filename, G_FILE_TEST_EXISTS))
>             {
>               am_default_load_keyfile (amd, filename);
>               amd->loaded = TRUE;
>-              /* Do not set amd->save: we don't need to write it to a
>-               * higher-priority directory until it actually changes. */
>             }
> 
>           g_free (filename);
>@@ -564,25 +871,19 @@ _list (const McpAccountStorage *self,
> 
>   if (!amd->loaded)
>     {
>-      gchar *old_filename = get_old_filename ();
>+      migrate_from = get_old_filename ();
> 
>-      if (g_file_test (old_filename, G_FILE_TEST_EXISTS))
>+      if (g_file_test (migrate_from, G_FILE_TEST_EXISTS))
>         {
>-          am_default_load_keyfile (amd, old_filename);
>+          if (!am_default_load_keyfile (amd, migrate_from))
>+            tp_clear_pointer (&migrate_from, g_free);
>           amd->loaded = TRUE;
>           amd->save = TRUE;
>-
>-          if (_commit (self, am, NULL))
>-            {
>-              DEBUG ("Migrated %s to new location: deleting old copy",
>-                  old_filename);
>-              if (g_unlink (old_filename) != 0)
>-                g_warning ("Unable to delete %s: %s", old_filename,
>-                    g_strerror (errno));
>-            }
>         }
>-
>-      g_free (old_filename);
>+      else
>+        {
>+          tp_clear_pointer (&migrate_from, g_free);
>+        }
>     }
> 
>   if (!amd->loaded)
>@@ -590,9 +891,28 @@ _list (const McpAccountStorage *self,
>       DEBUG ("Creating initial account data");
>       amd->loaded = TRUE;
>       amd->save = TRUE;
>-      _commit (self, am, NULL);
>     }
> 
>+  if (amd->save)
>+    {
>+      DEBUG ("Saving initial or migrated account data");
>+
>+      if (_commit (self, am, NULL))
>+        {
>+          if (migrate_from != NULL)
>+            {
>+              DEBUG ("Migrated %s to new location: deleting old copy",
>+                  migrate_from);
>+
>+              if (g_unlink (migrate_from) != 0)
>+                WARNING ("Unable to delete %s: %s", migrate_from,
>+                    g_strerror (errno));
>+            }
>+        }
>+    }
>+
>+  tp_clear_pointer (&migrate_from, g_free);
>+
>   g_hash_table_iter_init (&hash_iter, amd->accounts);
> 
>   while (g_hash_table_iter_next (&hash_iter, &k, &v))
>diff --git a/src/mcd-account-manager-default.h b/src/mcd-account-manager-default.h
>index e48a767..e16e062 100644
>--- a/src/mcd-account-manager-default.h
>+++ b/src/mcd-account-manager-default.h
>@@ -50,7 +50,7 @@ G_BEGIN_DECLS
> typedef struct {
>   GObject parent;
>   GHashTable *accounts;
>-  gchar *filename;
>+  gchar *directory;
>   gboolean save;
>   gboolean loaded;
> } _McdAccountManagerDefault;
>diff --git a/tests/twisted/account-storage/default-keyring-storage.py b/tests/twisted/account-storage/default-keyring-storage.py
>index 74f3bd3..c67399e 100644
>--- a/tests/twisted/account-storage/default-keyring-storage.py
>+++ b/tests/twisted/account-storage/default-keyring-storage.py
>@@ -61,8 +61,11 @@ def account_store(op, backend, key=None, value=None,
> def test(q, bus, mc):
>     ctl_dir = os.environ['MC_ACCOUNT_DIR']
>     old_key_file_name = os.path.join(ctl_dir, 'accounts.cfg')
>-    new_key_file_name = os.path.join(os.environ['XDG_DATA_HOME'],
>+    newer_key_file_name = os.path.join(os.environ['XDG_DATA_HOME'],
>             'telepathy', 'mission-control', 'accounts.cfg')
>+    new_variant_file_name = os.path.join(os.environ['XDG_DATA_HOME'],
>+            'telepathy', 'mission-control',
>+            'fakecm-fakeprotocol-dontdivert_40example_2ecom0.account')
>     group = 'fakecm/fakeprotocol/dontdivert_40example_2ecom0'
> 
>     account_manager, properties, interfaces = connect_to_mc(q, bus, mc)
>@@ -95,25 +98,35 @@ def test(q, bus, mc):
>     account_props.Set(cs.ACCOUNT, 'DisplayName', 'Work account')
>     account_props.Set(cs.ACCOUNT, 'Icon', 'im-jabber')
>     account_props.Set(cs.ACCOUNT, 'Nickname', 'Joe Bloggs')
>+    account_props.Set(cs.ACCOUNT, 'ConnectAutomatically', True)
>+    account_props.Set(cs.ACCOUNT, 'AutomaticPresence',
>+            (dbus.UInt32(cs.PRESENCE_EXTENDED_AWAY), 'xa',
>+                'never online'))
> 
>     tell_mc_to_die(q, bus)
> 
>     # .. let's check the keyfile
>     assert not os.path.exists(old_key_file_name)
>-    kf = keyfile_read(new_key_file_name)
>-    assert group in kf, kf
>-    assert kf[group]['manager'] == 'fakecm'
>-    assert kf[group]['protocol'] == 'fakeprotocol'
>-    assert kf[group]['param-account'] == params['account'], kf
>-    assert kf[group]['DisplayName'] == 'Work account', kf
>-    assert kf[group]['Icon'] == 'im-jabber', kf
>-    assert kf[group]['Nickname'] == 'Joe Bloggs', kf
>-
>-    pwd = account_store('get', 'keyfile', 'param-password')
>-    assert pwd == params['password'], pwd
>-
>-    # We no longer use gnome-keyring, so the password is stored as clear-text.
>-    assert kf[group]['param-password'] == params['password'], kf
>+    assert not os.path.exists(newer_key_file_name)
>+    assert 'Joe Bloggs' in open(new_variant_file_name).read()
>+    assertEquals("'fakecm'", account_store('get', 'variant-file', 'manager'))
>+    assertEquals("'fakeprotocol'", account_store('get', 'variant-file',
>+        'protocol'))
>+    assertEquals("'Work account'", account_store('get', 'variant-file',
>+        'DisplayName'))
>+    assertEquals("'im-jabber'", account_store('get', 'variant-file',
>+        'Icon'))
>+    assertEquals("'Joe Bloggs'", account_store('get', 'variant-file',
>+        'Nickname'))
>+    # For now, everything is a keyfile-escaped string
>+    assertEquals("'true'", account_store('get', 'variant-file',
>+        'ConnectAutomatically'))
>+    assertEquals("'4;xa;never online;'", account_store('get', 'variant-file',
>+        'AutomaticPresence'))
>+    assertEquals("keyfile-escaped 'dontdivert@example.com'",
>+            account_store('get', 'variant-file', 'param-account'))
>+    assertEquals("keyfile-escaped 'secrecy'",
>+            account_store('get', 'variant-file', 'param-password'))
> 
>     # Reactivate MC
>     account_manager, properties, interfaces = resuscitate_mc(q, bus, mc)
>@@ -139,29 +152,33 @@ def test(q, bus, mc):
> 
>     # Check the account is correctly deleted
>     assert not os.path.exists(old_key_file_name)
>-    kf = keyfile_read(new_key_file_name)
>-    assert group not in kf, kf
>+    assert not os.path.exists(newer_key_file_name)
>+    assert not os.path.exists(new_variant_file_name)
> 
>     # Tell MC to die, again
>     tell_mc_to_die(q, bus)
> 
>-    low_prio_key_file_name = os.path.join(
>+    low_prio_variant_file_name = os.path.join(
>             os.environ['XDG_DATA_DIRS'].split(':')[0],
>-            'telepathy', 'mission-control', 'accounts.cfg')
>-    os.makedirs(os.path.dirname(low_prio_key_file_name), 0700)
>+            'telepathy', 'mission-control',
>+            'fakecm-fakeprotocol-dontdivert_40example_2ecom0.account')
>+    os.makedirs(os.path.dirname(low_prio_variant_file_name), 0700)
> 
>     # This is deliberately a lower-priority location
>-    os.remove(new_key_file_name)
>-    open(low_prio_key_file_name, 'w').write(
>-r"""# Telepathy accounts
>-[%s]
>-manager=fakecm
>-protocol=fakeprotocol
>-param-account=dontdivert@example.com
>-param-password=password_in_keyfile
>-DisplayName=New and improved account
>-AutomaticPresence=2;available;;
>-""" % group)
>+    open(low_prio_variant_file_name, 'w').write(
>+    # For now, everything is a keyfile-escaped string, so AutomaticPresence
>+    # is weird
>+"""{
>+'manager': <'fakecm'>,
>+'protocol': <'fakeprotocol'>,
>+'DisplayName': <'New and improved account'>,
>+'AutomaticPresence': <'2;available;;'>,
>+'KeyFileParameters': <{
>+    'account': 'dontdivert@example.com',
>+    'password': 'password_in_variant_file'
>+    }>
>+}
>+""")
> 
>     account_manager, properties, interfaces = resuscitate_mc(q, bus, mc)
>     account = get_fakecm_account(bus, mc, account_path)
>@@ -169,8 +186,8 @@ AutomaticPresence=2;available;;
> 
>     # Files in lower-priority XDG locations aren't copied until something
>     # actually changes, and they aren't deleted.
>-    assert not os.path.exists(new_key_file_name)
>-    assert os.path.exists(low_prio_key_file_name)
>+    assert not os.path.exists(new_variant_file_name)
>+    assert os.path.exists(low_prio_variant_file_name)
> 
>     # Delete the password (only), like Empathy 3.0-3.4 do when migrating
>     account_iface.UpdateParameters({}, ['password'])
>@@ -188,16 +205,16 @@ AutomaticPresence=2;available;;
>     # Check the account has copied (not moved! XDG_DATA_DIRS are,
>     # conceptually, read-only) from the old to the new name
>     assert not os.path.exists(old_key_file_name)
>-    assert os.path.exists(low_prio_key_file_name)
>-    kf = keyfile_read(new_key_file_name)
>-    assert 'param-password' not in kf[group]
>-    pwd = account_store('get', 'keyfile', 'param-password')
>+    assert not os.path.exists(newer_key_file_name)
>+    assert os.path.exists(low_prio_variant_file_name)
>+    assert os.path.exists(new_variant_file_name)
>+    pwd = account_store('get', 'variant-file', 'param-password')
>     assertEquals(None, pwd)
> 
>     # Write out an account configuration in the old keyfile, to test
>-    # migration
>-    os.remove(new_key_file_name)
>-    os.remove(low_prio_key_file_name)
>+    # migration from there
>+    os.remove(new_variant_file_name)
>+    os.remove(low_prio_variant_file_name)
>     open(old_key_file_name, 'w').write(
> r"""# Telepathy accounts
> [%s]
>@@ -212,13 +229,12 @@ AutomaticPresence=2;available;;
>     account = get_fakecm_account(bus, mc, account_path)
>     account_iface = dbus.Interface(account, cs.ACCOUNT)
> 
>-    # This time it *does* get moved (really copied+deleted) automatically
>-    # during MC startup
>+    # This time it *does* get deleted automatically during MC startup,
>+    # after copying its contents to the new name/format
>     assert not os.path.exists(old_key_file_name)
>-    assert not os.path.exists(low_prio_key_file_name)
>-    kf = keyfile_read(new_key_file_name)
>-    assert 'param-password' not in kf[group]
>-    assertEquals('Ye olde account', kf[group]['DisplayName'])
>+    assert not os.path.exists(low_prio_variant_file_name)
>+    assertEquals("'Ye olde account'",
>+            account_store('get', 'variant-file', 'DisplayName'))
> 
> if __name__ == '__main__':
>     ctl_dir = os.environ['MC_ACCOUNT_DIR']
>diff --git a/tests/twisted/account-storage/diverted-storage.py b/tests/twisted/account-storage/diverted-storage.py
>index 2346b85..7aa011e 100644
>--- a/tests/twisted/account-storage/diverted-storage.py
>+++ b/tests/twisted/account-storage/diverted-storage.py
>@@ -88,10 +88,6 @@ def test(q, bus, mc):
>     assert kf[group]['Icon'] == 'im-jabber', kf
>     assert kf[group]['Nickname'] == 'Joe Bloggs', kf
> 
>-    # default keyfile should be empty
>-    ekf = keyfile_read(empty_key_file_name)
>-    assert ekf == { None: {} }, ekf
>-
>     # Reactivate MC
>     account_manager, properties, interfaces = resuscitate_mc(q, bus, mc)
>     account = get_fakecm_account(bus, mc, account_path)
>-- 
>1.8.4.rc3
>

Actions: View | Splinter Review

Attachments on bug 54875: 88294 | 88295 | 88296 | 88297 | 88298 | 88299 | 88300 | 88305

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.