Attachment 93138 Details for Bug 71093 – [MC 9/9] Opportunistically migrate accounts from untyped to typed Parameters

[patch] [MC 9/9] Opportunistically migrate accounts from untyped to typed Parameters

0009-Opportunistically-migrate-accounts-from-untyped-to-t.patch (text/plain), 11.37 KB, created by Simon McVittie on 2014-01-31 12:53:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-01-31 12:53:35 UTC

Size: 11.37 KB

patch

obsolete

>From 51b68f4920222150513cac5beaed47f97cde23f9 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 31 Jan 2014 12:31:48 +0000
>Subject: [PATCH 9/9] Opportunistically migrate accounts from untyped to typed
> Parameters
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=71093
>---
> src/mcd-account.c                               |  39 +++++++-
> src/mcd-storage.c                               | 121 ++++++++++++++++++++++++
> src/mcd-storage.h                               |   4 +
> tests/twisted/account-storage/load-keyfiles.py  |  31 +++---
> tests/twisted/account-storage/storage_helper.py |   8 +-
> 5 files changed, 185 insertions(+), 18 deletions(-)
>
>diff --git a/src/mcd-account.c b/src/mcd-account.c
>index 0d2bca4..d28fd05 100644
>--- a/src/mcd-account.c
>+++ b/src/mcd-account.c
>@@ -194,6 +194,9 @@ _mcd_account_connection_context_free (McdAccountConnectionContext *c)
> static guint _mcd_account_signals[LAST_SIGNAL] = { 0 };
> static GQuark account_ready_quark = 0;
> 
>+static void mcd_account_changed_property (McdAccount *account,
>+    const gchar *key, const GValue *value);
>+
> GQuark
> mcd_account_error_quark (void)
> {
>@@ -466,6 +469,7 @@ static void on_manager_ready (McdManager *manager, const GError *error,
> {
>     McdAccount *account = MCD_ACCOUNT (user_data);
>     GError *invalid_reason = NULL;
>+    TpProtocol *protocol;
> 
>     if (error)
>     {
>@@ -481,6 +485,38 @@ static void on_manager_ready (McdManager *manager, const GError *error,
>         g_clear_error (&account->priv->invalid_reason);
>     }
> 
>+    protocol = _mcd_manager_dup_protocol (account->priv->manager,
>+            account->priv->protocol_name);
>+
>+    if (protocol != NULL)
>+    {
>+        if (mcd_storage_maybe_migrate_parameters (
>+                account->priv->storage,
>+                account->priv->unique_name,
>+                protocol))
>+        {
>+            GHashTable *params = _mcd_account_dup_parameters (account);
>+
>+            if (params != NULL)
>+            {
>+                GValue value = G_VALUE_INIT;
>+
>+                g_value_init (&value, TP_HASH_TYPE_STRING_VARIANT_MAP);
>+                g_value_take_boxed (&value, params);
>+                mcd_account_changed_property (account, "Parameters", &value);
>+                g_value_unset (&value);
>+            }
>+            else
>+            {
>+                WARNING ("somehow managed to migrate parameters without "
>+                    "being able to emit change notification");
>+            }
>+        }
>+
>+
>+        g_object_unref (protocol);
>+    }
>+
>     mcd_account_loaded (account);
> }
> 
>@@ -655,9 +691,6 @@ on_connection_abort (McdConnection *connection, McdAccount *account)
>     _mcd_account_set_connection (account, NULL);
> }
> 
>-static void mcd_account_changed_property (McdAccount *account,
>-    const gchar *key, const GValue *value);
>-
> static void
> mcd_account_request_presence_int (McdAccount *account,
>                                   TpConnectionPresenceType type,
>diff --git a/src/mcd-storage.c b/src/mcd-storage.c
>index 7e503a8..092165e 100644
>--- a/src/mcd-storage.c
>+++ b/src/mcd-storage.c
>@@ -2001,3 +2001,124 @@ mcd_storage_add_account_from_plugin (McdStorage *self,
> 
>   return TRUE;
> }
>+
>+/* See whether we can migrate the parameters from being stored without
>+ * their types, to being stored with their types.
>+ * Commit changes and return TRUE if anything happened. */
>+gboolean
>+mcd_storage_maybe_migrate_parameters (McdStorage *self,
>+    const gchar *account_name,
>+    TpProtocol *protocol)
>+{
>+  McpAccountManager *api = MCP_ACCOUNT_MANAGER (self);
>+  McpAccountStorage *plugin;
>+  gchar **untyped_parameters = NULL;
>+  gsize i;
>+  gboolean ret = FALSE;
>+
>+  plugin = g_hash_table_lookup (self->accounts, account_name);
>+  g_return_if_fail (plugin != NULL);
>+
>+  /* If the storage backend can't store typed parameters, there's no point. */
>+  if (!(mcp_account_storage_get_features (plugin, account_name) &
>+       MCP_ACCOUNT_STORAGE_FEATURE_STORE_TYPES))
>+    goto finally;
>+
>+  untyped_parameters = mcp_account_storage_get_untyped_parameters (
>+      plugin, api, account_name);
>+
>+  /* If there's nothing to migrate, there's also no point. */
>+  if (untyped_parameters == NULL || untyped_parameters[0] == NULL)
>+    goto finally;
>+
>+  DEBUG ("trying to migrate %s", account_name);
>+
>+  for (i = 0; untyped_parameters[i] != NULL; i++)
>+    {
>+      const gchar *param_name = untyped_parameters[i];
>+      const TpConnectionManagerParam *param = tp_protocol_get_param (protocol,
>+          param_name);
>+      GError *error = NULL;
>+      GVariantType *type = NULL;
>+      GVariant *value;
>+      McpAccountStorageSetResult res;
>+
>+      if (param == NULL)
>+        {
>+          DEBUG ("cannot migrate parameter '%s': not supported by %s/%s",
>+              param_name, tp_protocol_get_cm_name (protocol),
>+              tp_protocol_get_name (protocol));
>+          goto next_param;
>+        }
>+
>+      type = tp_connection_manager_param_dup_variant_type (param);
>+
>+      DEBUG ("Migrating parameter '%s' of type '%.*s'",
>+          param_name,
>+          (gint) g_variant_type_get_string_length (type),
>+          g_variant_type_peek_string (type));
>+
>+      value = mcp_account_storage_get_parameter (plugin, api,
>+          account_name, param_name, type, NULL);
>+
>+      if (value == NULL)
>+        {
>+          DEBUG ("cannot migrate parameter '%s': %s #%d: %s",
>+              param_name, g_quark_to_string (error->domain), error->code,
>+              error->message);
>+          g_error_free (error);
>+          goto next_param;
>+        }
>+
>+      if (!g_variant_is_of_type (value, type))
>+        {
>+          DEBUG ("trying to convert parameter from type '%s'",
>+              g_variant_get_type_string (value));
>+
>+          /* consumes parameter */
>+          value = tp_variant_convert (value, type);
>+
>+          if (value == NULL)
>+            {
>+              DEBUG ("could not convert parameter to desired type");
>+              goto next_param;
>+            }
>+        }
>+
>+      res = mcp_account_storage_set_parameter (plugin, api,
>+          account_name, param_name, value, MCP_PARAMETER_FLAG_NONE);
>+
>+      switch (res)
>+        {
>+          case MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED:
>+            /* it really ought to be CHANGED, surely? */
>+            DEBUG ("Tried to upgrade parameter %s but the "
>+                "storage backend claims not to have changed it? "
>+                "Not sure I really believe that", param_name);
>+            /* fall through to the CHANGED case */
>+
>+          case MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED:
>+            ret = TRUE;
>+            break;
>+
>+          case MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED:
>+            WARNING ("Failed to set parameter %s", param_name);
>+            break;
>+
>+          default:
>+            WARNING ("set_parameter returned invalid result code %d "
>+                "for parameter %s", res, param_name);
>+        }
>+
>+next_param:
>+      if (type != NULL)
>+        g_variant_type_free (type);
>+    }
>+
>+  if (ret)
>+    mcp_account_storage_commit (plugin, api, account_name);
>+
>+finally:
>+  g_strfreev (untyped_parameters);
>+  return ret;
>+}
>diff --git a/src/mcd-storage.h b/src/mcd-storage.h
>index c2624a4..b073ebc 100644
>--- a/src/mcd-storage.h
>+++ b/src/mcd-storage.h
>@@ -163,6 +163,10 @@ gboolean mcd_storage_init_value_for_attribute (GValue *value,
>     const gchar *attribute,
>     const GVariantType **variant_type);
> 
>+gboolean mcd_storage_maybe_migrate_parameters (McdStorage *self,
>+    const gchar *account_name,
>+    TpProtocol *protocol);
>+
> G_END_DECLS
> 
> #endif /* MCD_STORAGE_H */
>diff --git a/tests/twisted/account-storage/load-keyfiles.py b/tests/twisted/account-storage/load-keyfiles.py
>index dfba53b..b6c9d2e 100644
>--- a/tests/twisted/account-storage/load-keyfiles.py
>+++ b/tests/twisted/account-storage/load-keyfiles.py
>@@ -240,8 +240,22 @@ def test(q, bus, mc):
>     # The masked account is still masked
>     assert open(variant_file_names['masked'], 'r').read() == ''
> 
>-    # Teach the one that knows its CM that the 'password' is a string.
>-    # This results in the higher-priority file being written out.
>+    # Because the CM exists, we can work out the correct types
>+    # for the 'migration' account's parameters. This triggers a commit
>+    # even though nothing has conceptually changed, so we have the type
>+    # for later. The file is copied, not moved, because XDG_DATA_DIRS are,
>+    # conceptually, read-only.
>+    assert not os.path.exists(old_key_file_name)
>+    assert not os.path.exists(newer_key_file_name)
>+    assert os.path.exists(low_prio_variant_file_names['migration'])
>+    assert os.path.exists(variant_file_names['migration'])
>+    assertEquals("'password_in_variant_file'",
>+        account_store('get', 'variant-file', 'param-password',
>+            account=tails['migration']))
>+    assertEquals("uint32 42", account_store('get', 'variant-file',
>+        'param-snakes', account=tails['migration']))
>+
>+    # Setting the password still does the right thing.
>     account_ifaces['migration'].UpdateParameters({'password': 'hello'}, [])
>     q.expect('dbus-signal',
>             path=account_paths['migration'],
>@@ -250,21 +264,16 @@ def test(q, bus, mc):
>             predicate=(lambda e:
>                 'Parameters' in e.args[0]),
>             )
>-    # Check the account has copied (not moved! XDG_DATA_DIRS are,
>-    # conceptually, read-only) 'migration' from the old to the new name
>     assert not os.path.exists(old_key_file_name)
>     assert not os.path.exists(newer_key_file_name)
>     assert os.path.exists(low_prio_variant_file_names['migration'])
>     assert os.path.exists(variant_file_names['migration'])
>-    assertEquals("'hello'", account_store('get', 'variant-file',
>-        'param-password', account=tails['migration']))
>-    # Parameters whose types are still unknown are copied too, but their
>-    # types are still unknown
>-    assertEquals("keyfile-escaped '42'", account_store('get', 'variant-file',
>-        'param-snakes', account=tails['migration']))
>+    assertEquals("'hello'",
>+        account_store('get', 'variant-file', 'param-password',
>+            account=tails['migration']))
> 
>     # 'absentcm' is still only in the low-priority location: we can't
>-    # known the types of its parameters
>+    # known the types of its parameters, so it doesn't get migrated.
>     assert not os.path.exists(old_key_file_name)
>     assert not os.path.exists(newer_key_file_name)
>     assert os.path.exists(low_prio_variant_file_names['absentcm'])
>diff --git a/tests/twisted/account-storage/storage_helper.py b/tests/twisted/account-storage/storage_helper.py
>index ebbe78e..0f67df9 100644
>--- a/tests/twisted/account-storage/storage_helper.py
>+++ b/tests/twisted/account-storage/storage_helper.py
>@@ -141,12 +141,12 @@ AutomaticPresence=2;available;;
>     assertEquals("'Second to none'",
>             account_store('get', 'variant-file', 'DisplayName',
>                 account=a2_tail))
>-    # MC doesn't currently ensure that parameters are stored with their
>-    # proper types.
>-    assertEquals("keyfile-escaped 'dontdivert1@example.com'",
>+    # Because the CM is installed, we can work out the right types
>+    # for the parameters, too.
>+    assertEquals("'dontdivert1@example.com'",
>             account_store('get', 'variant-file', 'param-account',
>                 account=a1_tail))
>-    assertEquals("keyfile-escaped 'dontdivert2@example.com'",
>+    assertEquals("'dontdivert2@example.com'",
>             account_store('get', 'variant-file', 'param-account',
>                 account=a2_tail))
> 
>-- 
>1.9.rc1
>

Actions: View | Splinter Review

Attachments on bug 71093: 93126 | 93127 | 93128 | 93129 | 93130 | 93131 | 93132 | 93133 | 93134 | 93137 | 93138 | 93316 | 93317 | 93318 | 93319 | 93320 | 93476 | 93477

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.