From 9668ba65c927a168e62b41fb19f1945bba868b66 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 3 Feb 2014 20:18:46 +0000 Subject: [PATCH 5/5] 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 9374751..0cfcdab 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 3bd21de..68ccd43 100644 --- a/src/mcd-storage.c +++ b/src/mcd-storage.c @@ -2060,3 +2060,124 @@ mcd_storage_dup_typed_parameters (McdStorage *self, return params; } + +/* 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_val_if_fail (plugin != NULL, FALSE); + + /* If the storage backend can't store typed parameters, there's no point. */ + if (!mcp_account_storage_has_any_flag (plugin, account_name, + MCP_ACCOUNT_STORAGE_FLAG_STORES_TYPES)) + goto finally; + + untyped_parameters = mcp_account_storage_list_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 16f112b..56732f1 100644 --- a/src/mcd-storage.h +++ b/src/mcd-storage.h @@ -166,6 +166,10 @@ gboolean mcd_storage_init_value_for_attribute (GValue *value, GHashTable *mcd_storage_dup_typed_parameters (McdStorage *self, const gchar *account); +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