Attachment 88299 Details for Bug 54875 – Default accounts backend: store attributes in a properly typed form

[patch] Default accounts backend: store attributes in a properly typed form

0009-Default-accounts-backend-store-attributes-in-a-prope.patch (text/plain), 9.56 KB, created by Simon McVittie on 2013-10-29 14:26:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-10-29 14:26:21 UTC

Size: 9.56 KB

patch

obsolete

>From 1c5e5c40a6244febf61f2854dbc60179048103f0 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 29 Oct 2013 13:38:57 +0000
>Subject: [PATCH 09/12] Default accounts backend: store attributes in a
> properly typed form
>
>---
> src/mcd-account-manager-default.c                  | 84 +++++++++++++---------
> .../account-storage/default-keyring-storage.py     | 17 ++---
> 2 files changed, 57 insertions(+), 44 deletions(-)
>
>diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
>index 5da0e50..531b55e 100644
>--- a/src/mcd-account-manager-default.c
>+++ b/src/mcd-account-manager-default.c
>@@ -30,6 +30,7 @@
> 
> #include "mcd-account-manager-default.h"
> #include "mcd-debug.h"
>+#include "mcd-storage.h"
> #include "mcd-misc.h"
> 
> #define PLUGIN_NAME "default"
>@@ -37,7 +38,7 @@
> #define PLUGIN_DESCRIPTION "Default account storage backend"
> 
> typedef struct {
>-    /* owned string, attribute => owned string, value
>+    /* owned string, attribute => owned GVariant, value
>      * attributes to be stored in the variant-file */
>     GHashTable *attributes;
>     /* owned string, parameter (without "param-") => owned string, value
>@@ -67,7 +68,7 @@ ensure_stored_account (McdAccountManagerDefault *self,
>     {
>       sa = g_slice_new0 (McdDefaultStoredAccount);
>       sa->attributes = g_hash_table_new_full (g_str_hash, g_str_equal,
>-          g_free, g_free);
>+          g_free, (GDestroyNotify) g_variant_unref);
>       sa->untyped_parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
>           g_free, g_free);
>       g_hash_table_insert (self->accounts, g_strdup (account), sa);
>@@ -183,13 +184,13 @@ set_parameter (const McpAccountStorage *self,
>   return TRUE;
> }
> 
>-/* As above, the string is escaped for a keyfile. */
> static gboolean
>-set_attribute (const McpAccountStorage *self,
>-    const McpAccountManager *am,
>+set_attribute (McpAccountStorage *self,
>+    McpAccountManager *am,
>     const gchar *account,
>     const gchar *attribute,
>-    const gchar *val)
>+    GVariant *val,
>+    McpAttributeFlags flags)
> {
>   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
>   McdDefaultStoredAccount *sa = ensure_stored_account (amd, account);
>@@ -197,7 +198,8 @@ set_attribute (const McpAccountStorage *self,
>   amd->save = TRUE;
> 
>   if (val != NULL)
>-    g_hash_table_insert (sa->attributes, g_strdup (attribute), g_strdup (val));
>+    g_hash_table_insert (sa->attributes, g_strdup (attribute),
>+        g_variant_ref (val));
>   else
>     g_hash_table_remove (sa->attributes, attribute);
> 
>@@ -217,7 +219,8 @@ _set (const McpAccountStorage *self,
>     }
>   else
>     {
>-      return set_attribute (self, am, account, key, val);
>+      /* we implement set_attribute(), so MC shouldn't call this */
>+      g_assert_not_reached ();
>     }
> }
> 
>@@ -267,7 +270,7 @@ _get (const McpAccountStorage *self,
> 
>   if (key != NULL)
>     {
>-      gchar *v = NULL;
>+      GVariant *v = NULL;
> 
>       if (g_str_has_prefix (key, "param-"))
>         {
>@@ -279,7 +282,8 @@ _get (const McpAccountStorage *self,
>       if (v == NULL)
>         return FALSE;
> 
>-      mcp_account_manager_set_value (am, account, key, v);
>+      mcp_account_manager_set_attribute (am, account, key, v,
>+          MCP_ATTRIBUTE_FLAG_NONE);
>     }
>   else
>     {
>@@ -291,7 +295,8 @@ _get (const McpAccountStorage *self,
>       while (g_hash_table_iter_next (&iter, &k, &v))
>         {
>           if (v != NULL)
>-            mcp_account_manager_set_value (am, account, k, v);
>+            mcp_account_manager_set_attribute (am, account, k,
>+                v, MCP_ATTRIBUTE_FLAG_NONE);
>         }
> 
>       g_hash_table_iter_init (&iter, sa->untyped_parameters);
>@@ -456,11 +461,7 @@ am_default_commit_one (McdAccountManagerDefault *self,
> 
>   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_add (&attrs_builder, "{sv}", k, v);
>     }
> 
>   g_variant_builder_init (&params_builder, G_VARIANT_TYPE ("a{ss}"));
>@@ -599,18 +600,45 @@ am_default_load_keyfile (McdAccountManagerDefault *self,
>       for (j = 0; j < m; j++)
>         {
>           gchar *key = keys[j];
>-          gchar *raw = g_key_file_get_value (keyfile, account, key, NULL);
> 
>           if (g_str_has_prefix (key, "param-"))
>             {
>+              gchar *raw = g_key_file_get_value (keyfile, account, key, NULL);
>+
>               /* steals ownership of raw */
>               g_hash_table_insert (sa->untyped_parameters, g_strdup (key + 6),
>                   raw);
>             }
>           else
>             {
>-              /* steals ownership of raw */
>-              g_hash_table_insert (sa->attributes, g_strdup (key), raw);
>+              const gchar *type = mcd_storage_get_attribute_type (key);
>+              GVariant *variant = NULL;
>+
>+              if (type == NULL)
>+                {
>+                  /* go to the error code path */
>+                  g_set_error (&error, TP_ERROR, TP_ERROR_INVALID_ARGUMENT,
>+                      "unknown attribute");
>+                }
>+              else
>+                {
>+                  variant = mcd_keyfile_get_variant (keyfile,
>+                      account, key, G_VARIANT_TYPE (type), &error);
>+                }
>+
>+              if (variant == NULL)
>+                {
>+                  WARNING ("Unable to migrate %s.%s from keyfile: %s",
>+                      account, key, error->message);
>+                  g_clear_error (&error);
>+                  /* Don't delete the old file, which might be recoverable. */
>+                  all_ok = FALSE;
>+                }
>+              else
>+                {
>+                  g_hash_table_insert (sa->attributes, g_strdup (key),
>+                      g_variant_ref_sink (variant));
>+                }
>             }
>         }
> 
>@@ -708,21 +736,8 @@ am_default_load_variant_file (McdAccountManagerDefault *self,
>       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);
>-            }
>+          g_hash_table_insert (sa->attributes,
>+              g_strdup (k), g_variant_ref (v));
>         }
>     }
> 
>@@ -933,6 +948,7 @@ account_storage_iface_init (McpAccountStorageIface *iface,
> 
>   iface->get = _get;
>   iface->set = _set;
>+  iface->set_attribute = set_attribute;
>   iface->create = _create;
>   iface->delete = _delete;
>   iface->commit_one = _commit;
>diff --git a/tests/twisted/account-storage/default-keyring-storage.py b/tests/twisted/account-storage/default-keyring-storage.py
>index 58ef108..6ed673e 100644
>--- a/tests/twisted/account-storage/default-keyring-storage.py
>+++ b/tests/twisted/account-storage/default-keyring-storage.py
>@@ -118,11 +118,10 @@ def test(q, bus, mc):
>         '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',
>+    assertEquals('true', account_store('get', 'variant-file',
>         'ConnectAutomatically'))
>-    assertEquals("'4;xa;never online;'", account_store('get', 'variant-file',
>-        'AutomaticPresence'))
>+    assertEquals("(uint32 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'",
>@@ -166,13 +165,11 @@ def test(q, bus, mc):
> 
>     # This is deliberately a lower-priority location
>     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;;'>,
>+'AutomaticPresence': <(uint32 2, 'available', '')>,
> 'KeyFileParameters': <{
>     'account': 'dontdivert@example.com',
>     'password': 'password_in_variant_file'
>@@ -186,7 +183,7 @@ def test(q, bus, mc):
> 'manager': <'fakecm'>,
> 'protocol': <'fakeprotocol'>,
> 'DisplayName': <'Visible'>,
>-'AutomaticPresence': <'2;available;;'>,
>+'AutomaticPresence': <(uint32 2, 'available', '')>,
> 'KeyFileParameters': <{'account': 'dontdivert@example.com',
>     'password': 'password_in_variant_file'}>
> }
>@@ -198,7 +195,7 @@ def test(q, bus, mc):
> 'protocol': <'fakeprotocol'>,
> 'DisplayName': <'Hidden'>,
> 'Nickname': <'Hidden'>,
>-'AutomaticPresence': <'2;available;;'>,
>+'AutomaticPresence': <(uint32 2, 'available', '')>,
> 'KeyFileParameters': <{'account': 'dontdivert@example.com',
>     'password': 'password_in_variant_file'}>
> }
>@@ -211,7 +208,7 @@ def test(q, bus, mc):
>             'w').write("""{
> 'manager': <'fakecm'>,
> 'protocol': <'fakeprotocol'>,
>-'AutomaticPresence': <'2;available;;'>,
>+'AutomaticPresence': <(uint32 2, 'available', '')>,
> 'KeyFileParameters': <{'account': 'dontdivert@example.com',
>     'password': 'password_in_variant_file'}>
> }
>-- 
>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.