Attachment 88886 Details for Bug 71384 – Simplify a bit storage API

[patch] Simplify a bit storage API

account-storage.patch (text/plain), 37.00 KB, created by Simon McVittie on 2013-11-08 12:25:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-11-08 12:25:01 UTC

Size: 37.00 KB

patch

obsolete

>From 5ab77e7a3f9f7ea4345ef22b9ca0e538d0da7052 Mon Sep 17 00:00:00 2001
>From: Xavier Claessens <xavier.claessens@collabora.co.uk>
>Date: Thu, 07 Nov 2013 20:07:56 +0000
>Subject: Simplify a bit storage API
>
>This is an API break, but we already did some since last release.
>
>This removes mcp_account_storage_commit() because it is redundant with
>mcp_account_storage_commit_one (plugin, am, NULL);
>
>This removes mcp_account_storage_owns() because an account is now
>owned by one and only one storage plugin and MC now keeps track of
>which storage plugin each account comes from.
>
>Finally this adds default implementation on most iface methods to
>make read-only plugins easier to implement. Only _get() and _list()
>and mandatory.
>---
>diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c
>index b51c126..28ca4cf 100644
>--- a/mission-control-plugins/account-storage.c
>+++ b/mission-control-plugins/account-storage.c
>@@ -59,7 +59,6 @@
>  *   iface->get = foo_plugin_get;
>  *   iface->set = foo_plugin_get;
>  *   iface->delete = foo_plugin_delete;
>- *   iface->commit = foo_plugin_commit;
>  *   iface->commit_one = foo_plugin_commit_one;
>  *   iface->list = foo_plugin_list;
>  *   iface->ready = foo_plugin_ready;
>@@ -67,7 +66,6 @@
>  *   iface->get_additional_info = foo_plugin_get_additional_info;
>  *   iface->get_restrictions = foo_plugin_get_restrictions;
>  *   iface->create = foo_plugin_create;
>- *   iface->owns = foo_plugin_owns;
>  *   iface->set_attribute = foo_plugin_set_attribute;
>  *   iface->set_parameter = foo_plugin_set_parameter;
>  * }
>@@ -144,17 +142,63 @@ default_set_parameter (McpAccountStorage *storage,
>   return FALSE;
> }
> 
>+static gchar *
>+default_create (const McpAccountStorage *storage,
>+    const McpAccountManager *am,
>+    const gchar *manager,
>+    const gchar *protocol,
>+    const gchar *identification,
>+    GError **error)
>+{
>+  g_set_error (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
>+      "This storage does not implement create function");
>+  return NULL;
>+}
>+
> static gboolean
>-default_owns (McpAccountStorage *storage,
>-    McpAccountManager *am,
>+default_delete (const McpAccountStorage *storage,
>+    const McpAccountManager *am,
>+    const gchar *account,
>+    const gchar *key)
>+{
>+  return FALSE;
>+}
>+
>+static gboolean
>+default_commit_one (const McpAccountStorage *storage,
>+    const McpAccountManager *am,
>     const gchar *account)
> {
>-  /* This has the side-effect of pushing the "manager" key back into @am,
>-   * but that should be a no-op in practice: we always call this
>-   * method in priority order and stop at the first one that says "yes",
>-   * and @am's idea of what "manager" is should have come from that same
>-   * plugin anyway. */
>-  return mcp_account_storage_get (storage, am, account, "manager");
>+  return FALSE;
>+}
>+
>+static void
>+default_ready (const McpAccountStorage *storage,
>+    const McpAccountManager *am)
>+{
>+}
>+
>+static void
>+default_get_identifier (const McpAccountStorage *storage,
>+    const gchar *account,
>+    GValue *identifier)
>+{
>+  g_value_init (identifier, G_TYPE_STRING);
>+  g_value_set_string (identifier, account);
>+}
>+
>+static GHashTable *
>+default_get_additional_info (const McpAccountStorage *storage,
>+    const gchar *account)
>+{
>+  return g_hash_table_new (g_str_hash, g_str_equal);
>+}
>+
>+static TpStorageRestrictionFlags
>+default_get_restrictions (const McpAccountStorage *storage,
>+    const gchar *account)
>+{
>+  return 0;
> }
> 
> static void
>@@ -164,10 +208,16 @@ class_init (gpointer klass,
>   GType type = G_TYPE_FROM_CLASS (klass);
>   McpAccountStorageIface *iface = klass;
> 
>-  iface->owns = default_owns;
>   iface->set = default_set;
>   iface->set_attribute = default_set_attribute;
>   iface->set_parameter = default_set_parameter;
>+  iface->create = default_create;
>+  iface->delete = default_delete;
>+  iface->commit_one = default_commit_one;
>+  iface->ready = default_ready;
>+  iface->get_identifier = default_get_identifier;
>+  iface->get_additional_info = default_get_additional_info;
>+  iface->get_restrictions = default_get_restrictions;
> 
>   if (signals[CREATED] != 0)
>     {
>@@ -313,7 +363,6 @@ mcp_account_storage_get_type (void)
>  * @set: implementation of mcp_account_storage_set()
>  * @get: implementation of mcp_account_storage_get()
>  * @delete: implementation of mcp_account_storage_delete()
>- * @commit: implementation of mcp_account_storage_commit()
>  * @list: implementation of mcp_account_storage_list()
>  * @ready: implementation of mcp_account_storage_ready()
>  * @commit_one: implementation of mcp_account_storage_commit_one()
>@@ -453,9 +502,8 @@ mcp_account_storage_get (const McpAccountStorage *storage,
>  * decline to store the setting.
>  *
>  * The plugin is not expected to write to its long term storage
>- * at this point. It can expect Mission Control to call either
>- * mcp_account_storage_commit() or mcp_account_storage_commit_one()
>- * after a short delay.
>+ * at this point. It can expect Mission Control to call
>+ * mcp_account_storage_commit_one() after a short delay.
>  *
>  * Plugins that implement mcp_storage_set_attribute() and
>  * mcp_account_storage_set_parameter() can just return %FALSE here.
>@@ -474,6 +522,7 @@ mcp_account_storage_set (const McpAccountStorage *storage,
> 
>   SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, FALSE);
>+  g_return_val_if_fail (iface->set != NULL, FALSE);
> 
>   return iface->set (storage, am, account, key, value);
> }
>@@ -593,7 +642,7 @@ mcp_account_storage_set_parameter (McpAccountStorage *storage,
>  * Inform the plugin that a new account is being created. @manager, @protocol
>  * and @identification are given to help determining the account's unique name,
>  * but does not need to be stored on the account yet, mcp_account_storage_set()
>- * and mcp_account_storage_commit() will be called later.
>+ * and mcp_account_storage_commit_one() will be called later.
>  *
>  * It is recommended to use mcp_account_manager_get_unique_name() to create the
>  * unique name, but it's not mandatory. One could base the unique name on an
>@@ -601,7 +650,10 @@ mcp_account_storage_set_parameter (McpAccountStorage *storage,
>  * (e.g. goa__1234).
>  *
>  * #McpAccountStorage::created signal should not be emitted for this account,
>- * not even when mcp_account_storage_commit() will be called.
>+ * not even when mcp_account_storage_commit_one() will be called.
>+ *
>+ * There is a default implementation, which just returns %NULL and raise an
>+ * error.
>  *
>  * Returns: (transfer full): the newly allocated account name, which should
>  *  be freed once the caller is done with it, or %NULL if that couldn't
>@@ -617,14 +669,9 @@ mcp_account_storage_create (const McpAccountStorage *storage,
> {
>   McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> 
>+  SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, NULL);
>-
>-  if (iface->create == NULL)
>-    {
>-      g_set_error (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
>-          "This storage does not implement create function");
>-      return NULL;
>-    }
>+  g_return_val_if_fail (iface->create != NULL, NULL);
> 
>   return iface->create (storage, am, manager, protocol, identification, error);
> }
>@@ -664,6 +711,9 @@ mcp_account_storage_create (const McpAccountStorage *storage,
>  * The plugin is not expected to update its long term storage at
>  * this point.
>  *
>+ * There is a default implementation, which just returns %FALSE, for read-only
>+ * plugins.
>+ *
>  * Returns: %TRUE if the setting or settings are not
>  * the plugin's cache after this operation, %FALSE otherwise.
>  * This is very unlikely to ever be %FALSE, as a plugin is always
>@@ -679,24 +729,29 @@ mcp_account_storage_delete (const McpAccountStorage *storage,
> 
>   SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, FALSE);
>+  g_return_val_if_fail (iface->delete != NULL, FALSE);
> 
>   return iface->delete (storage, am, account, key);
> }
> 
> /**
>- * McpAccountStorageCommitFunc:
>+ * McpAccountStorageCommitOneFunc:
>  * @storage: an #McpAccountStorage instance
>  * @am: an #McpAccountManager instance
>+ * @account: (allow-none): the unique suffix of an account's object path,
>+ *  or %NULL
>  *
>- * An implementation of mcp_account_storage_commit().
>+ * An implementation of mcp_account_storage_commit_one().
>  *
>  * Returns: %TRUE if the commit process was started successfully
>  */
> 
> /**
>- * mcp_account_storage_commit:
>+ * mcp_account_storage_commit_one:
>  * @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
>  *
>  * The plugin is expected to write its cache to long term storage,
>  * deleting, adding or updating entries in said storage as needed.
>@@ -705,67 +760,11 @@ mcp_account_storage_delete (const McpAccountStorage *storage,
>  * not required to have finished its commit operation when it returns,
>  * merely to have started the operation.
>  *
>- * If the @commit_one method is implemented, it will be called preferentially
>- * if only one account is to be committed. If the @commit_one method is
>- * implemented but @commit is not, @commit_one will be called with
>- * @account_name = %NULL to commit all accounts.
>+ * If @account = %NULL it means that it should commit all accounts owned by the
>+ * storage plugin.
>  *
>- * Returns: %TRUE if the commit process was started (but not necessarily
>- * completed) successfully; %FALSE if there was a problem that was immediately
>- * obvious.
>- */
>-gboolean
>-mcp_account_storage_commit (const McpAccountStorage *storage,
>-    const McpAccountManager *am)
>-{
>-  McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>-
>-  SDEBUG (storage, "committing all accounts");
>-  g_return_val_if_fail (iface != NULL, FALSE);
>-
>-  if (iface->commit != NULL)
>-    {
>-      return iface->commit (storage, am);
>-    }
>-  else if (iface->commit_one != NULL)
>-    {
>-      return iface->commit_one (storage, am, NULL);
>-    }
>-  else
>-    {
>-      SDEBUG (storage,
>-          "neither commit nor commit_one is implemented; cannot save accounts");
>-      return FALSE;
>-    }
>-}
>-
>-/**
>- * McpAccountStorageCommitOneFunc:
>- * @storage: an #McpAccountStorage instance
>- * @am: an #McpAccountManager instance
>- * @account: (allow-none): the unique suffix of an account's object path,
>- *  or %NULL
>- *
>- * An implementation of mcp_account_storage_commit_one().
>- *
>- * Returns: %TRUE if the commit process was started successfully
>- */
>-
>-/**
>- * mcp_account_storage_commit_one:
>- * @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 and
>- *  mcp_account_storage_commit() is unimplemented
>- *
>- * The same as mcp_account_storage_commit(), but only commit the given
>- * account. This is optional to implement; the default implementation
>- * is to call @commit.
>- *
>- * If both mcp_account_storage_commit_one() and mcp_account_storage_commit()
>- * are implemented, Mission Control will never pass @account = %NULL to
>- * this method.
>+ * A default implementation that simply returns %FALSE is provided for read-only
>+ * plugins.
>  *
>  * Returns: %TRUE if the commit process was started (but not necessarily
>  * completed) successfully; %FALSE if there was a problem that was immediately
>@@ -780,12 +779,9 @@ mcp_account_storage_commit_one (const McpAccountStorage *storage,
> 
>   SDEBUG (storage, "called for %s", account ? account : "<all accounts>");
>   g_return_val_if_fail (iface != NULL, FALSE);
>+  g_return_val_if_fail (iface->commit_one != NULL, FALSE);
> 
>-  if (iface->commit_one != NULL)
>-    return iface->commit_one (storage, am, account);
>-  else
>-    /* Fall back to plain ->commit() */
>-    return mcp_account_storage_commit (storage, am);
>+  return iface->commit_one (storage, am, account);
> }
> 
> /**
>@@ -822,6 +818,7 @@ mcp_account_storage_list (const McpAccountStorage *storage,
> 
>   SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, NULL);
>+  g_return_val_if_fail (iface->list != NULL, NULL);
> 
>   return iface->list (storage, am);
> }
>@@ -842,6 +839,9 @@ mcp_account_storage_list (const McpAccountStorage *storage,
>  * Informs the plugin that it is now permitted to create new accounts,
>  * ie it can now fire its "created", "altered-one", "toggled" and "deleted"
>  * signals.
>+ *
>+ * There is a default implementation for plugins that can't create accounts from
>+ * external sources, as they can never fire the async account change signals.
>  */
> void
> mcp_account_storage_ready (const McpAccountStorage *storage,
>@@ -850,12 +850,9 @@ mcp_account_storage_ready (const McpAccountStorage *storage,
>   McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> 
>   g_return_if_fail (iface != NULL);
>+  g_return_if_fail (iface->ready != NULL);
> 
>-  /* plugins that can't create accounts from external sources don't  *
>-   * need to implement this method, as they can never fire the async *
>-   * account change signals:                                         */
>-  if (iface->ready != NULL)
>-    iface->ready (storage, am);
>+  iface->ready (storage, am);
> }
> 
> /**
>@@ -880,6 +877,8 @@ mcp_account_storage_ready (const McpAccountStorage *storage,
>  *
>  * This method will only be called for the storage plugin that "owns"
>  * the account.
>+ *
>+ * There is default implementation that sets @identifier to @account string.
>  */
> void
> mcp_account_storage_get_identifier (const McpAccountStorage *storage,
>@@ -890,18 +889,11 @@ mcp_account_storage_get_identifier (const McpAccountStorage *storage,
> 
>   SDEBUG (storage, "");
>   g_return_if_fail (iface != NULL);
>+  g_return_if_fail (iface->get_identifier != NULL);
>   g_return_if_fail (identifier != NULL);
>   g_return_if_fail (!G_IS_VALUE (identifier));
> 
>-  if (iface->get_identifier == NULL)
>-    {
>-      g_value_init (identifier, G_TYPE_STRING);
>-      g_value_set_string (identifier, account);
>-    }
>-  else
>-    {
>-      iface->get_identifier (storage, account, identifier);
>-    }
>+  iface->get_identifier (storage, account, identifier);
> }
> 
> /**
>@@ -926,6 +918,8 @@ mcp_account_storage_get_identifier (const McpAccountStorage *storage,
>  * This method will only be called for the storage plugin that "owns"
>  * the account.
>  *
>+ * There is a default implementation that return an empty table.
>+ *
>  * Returns: (transfer container) (element-type utf8 GObject.Value): additional
>  *  storage-specific information
>  */
>@@ -934,18 +928,12 @@ mcp_account_storage_get_additional_info (const McpAccountStorage *storage,
>     const gchar *account)
> {
>   McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>-  GHashTable *ret = NULL;
> 
>   SDEBUG (storage, "");
>   g_return_val_if_fail (iface != NULL, FALSE);
>+  g_return_val_if_fail (iface->get_additional_info != NULL, FALSE);
> 
>-  if (iface->get_additional_info != NULL)
>-    ret = iface->get_additional_info (storage, account);
>-
>-  if (ret == NULL)
>-    ret = g_hash_table_new (g_str_hash, g_str_equal);
>-
>-  return ret;
>+  return iface->get_additional_info (storage, account);
> }
> 
> /**
>@@ -966,6 +954,8 @@ mcp_account_storage_get_additional_info (const McpAccountStorage *storage,
>  * This method will only be called for the storage plugin that "owns"
>  * the account.
>  *
>+ * There is a default implementation that just return 0 (no restrictions).
>+ *
>  * Returns: a bitmask of %TpStorageRestrictionFlags with the restrictions to
>  *  account storage.
>  */
>@@ -976,11 +966,9 @@ mcp_account_storage_get_restrictions (const McpAccountStorage *storage,
>   McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> 
>   g_return_val_if_fail (iface != NULL, 0);
>+  g_return_val_if_fail (iface->get_restrictions != NULL, 0);
> 
>-  if (iface->get_restrictions == NULL)
>-    return 0;
>-  else
>-    return iface->get_restrictions (storage, account);
>+  return iface->get_restrictions (storage, account);
> }
> 
> /**
>@@ -1112,33 +1100,3 @@ mcp_account_storage_emit_reconnect (McpAccountStorage *storage,
> {
>   g_signal_emit (storage, signals[RECONNECT], 0, account);
> }
>-
>-/**
>- * mcp_account_storage_owns:
>- * @storage: an #McpAccountStorage instance
>- * @am: an #McpAccountManager instance
>- * @account: the unique name (object-path tail) of an account
>- *
>- * Check whether @account is stored in @storage. The highest-priority
>- * plugin for which this function returns %TRUE is considered to be
>- * responsible for @account.
>- *
>- * There is a default implementation, which calls mcp_account_storage_get()
>- * for the well-known key "manager".
>- *
>- * Returns: %TRUE if @account is stored in @storage
>- *
>- * Since: 5.15.0
>- */
>-gboolean
>-mcp_account_storage_owns (McpAccountStorage *storage,
>-    McpAccountManager *am,
>-    const gchar *account)
>-{
>-  McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>-
>-  g_return_val_if_fail (iface != NULL, FALSE);
>-  g_return_val_if_fail (iface->owns != NULL, FALSE);
>-
>-  return iface->owns (storage, am, account);
>-}
>diff --git a/mission-control-plugins/account-storage.h b/mission-control-plugins/account-storage.h
>index 5c11102..fc0cc8f 100644
>--- a/mission-control-plugins/account-storage.h
>+++ b/mission-control-plugins/account-storage.h
>@@ -85,9 +85,6 @@ typedef gboolean (*McpAccountStorageDeleteFunc) (
> typedef GList * (*McpAccountStorageListFunc) (
>     const McpAccountStorage *storage,
>     const McpAccountManager *am);
>-typedef gboolean (*McpAccountStorageCommitFunc) (
>-    const McpAccountStorage *storage,
>-    const McpAccountManager *am);
> typedef gboolean (*McpAccountStorageCommitOneFunc) (
>     const McpAccountStorage *storage,
>     const McpAccountManager *am,
>@@ -118,7 +115,6 @@ struct _McpAccountStorageIface
>   McpAccountStorageSetFunc set;
>   McpAccountStorageGetFunc get;
>   McpAccountStorageDeleteFunc delete;
>-  McpAccountStorageCommitFunc commit;
>   McpAccountStorageListFunc list;
>   McpAccountStorageReadyFunc ready;
>   McpAccountStorageCommitOneFunc commit_one;
>@@ -128,9 +124,6 @@ struct _McpAccountStorageIface
>   McpAccountStorageCreate create;
> 
>   /* Since 5.15.0 */
>-  gboolean (*owns) (McpAccountStorage *storage,
>-      McpAccountManager *am,
>-      const gchar *account);
>   gboolean (*set_attribute) (McpAccountStorage *storage,
>       McpAccountManager *am,
>       const gchar *account,
>@@ -175,10 +168,6 @@ void mcp_account_storage_ready (const McpAccountStorage *storage,
>     const McpAccountManager *am);
> 
> gboolean
>-mcp_account_storage_commit (const McpAccountStorage *storage,
>-    const McpAccountManager *am);
>-
>-gboolean
> mcp_account_storage_commit_one (const McpAccountStorage *storage,
>     const McpAccountManager *am,
>     const gchar *account);
>@@ -203,10 +192,6 @@ const gchar *mcp_account_storage_name (const McpAccountStorage *storage);
> const gchar *mcp_account_storage_description (const McpAccountStorage *storage);
> const gchar *mcp_account_storage_provider (const McpAccountStorage *storage);
> 
>-gboolean mcp_account_storage_owns (McpAccountStorage *storage,
>-    McpAccountManager *am,
>-    const gchar *account);
>-
> gboolean mcp_account_storage_set_attribute (McpAccountStorage *storage,
>     McpAccountManager *am,
>     const gchar *account,
>diff --git a/src/mcd-account-manager.c b/src/mcd-account-manager.c
>index cd10461..0a64aaf 100644
>--- a/src/mcd-account-manager.c
>+++ b/src/mcd-account-manager.c
>@@ -272,16 +272,9 @@ created_cb (GObject *storage_plugin_obj,
>     lad->account_lock = 1; /* will be released at the end of this function */
> 
>     /* actually fetch the data into our cache from the plugin: */
>-    if (mcd_storage_add_account_from_plugin (storage, plugin, name))
>-    {
>-        account = mcd_account_new (am, name, priv->minotaur);
>-        lad->account = account;
>-    }
>-    else
>-    {
>-        /* that function already warned about it */
>-        goto finish;
>-    }
>+    mcd_storage_add_account_from_plugin (storage, plugin, name);
>+    account = mcd_account_new (am, name, priv->minotaur);
>+    lad->account = account;
> 
>     if (G_UNLIKELY (!account))
>     {
>diff --git a/src/mcd-storage.c b/src/mcd-storage.c
>index 323a287..0bb8e29 100644
>--- a/src/mcd-storage.c
>+++ b/src/mcd-storage.c
>@@ -79,8 +79,30 @@ typedef struct {
>     /* set of owned strings
>      * e.g. { 'password': 'password' } */
>     GHashTable *secrets;
>+
>+    /* owned storage plugin owning this account */
>+    McpAccountStorage *storage;
> } McdStorageAccount;
> 
>+static McdStorageAccount *
>+mcd_storage_account_new (McpAccountStorage *storage)
>+{
>+  McdStorageAccount *sa;
>+
>+  sa = g_slice_new (McdStorageAccount);
>+  sa->attributes = g_hash_table_new_full (g_str_hash, g_str_equal,
>+      g_free, (GDestroyNotify) g_variant_unref);
>+  sa->parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
>+      g_free, (GDestroyNotify) g_variant_unref);
>+  sa->escaped_parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
>+      g_free, g_free);
>+  sa->secrets = g_hash_table_new_full (g_str_hash, g_str_equal,
>+      g_free, NULL);
>+  sa->storage = g_object_ref (storage);
>+
>+  return sa;
>+}
>+
> static void
> mcd_storage_account_free (gpointer p)
> {
>@@ -90,6 +112,7 @@ mcd_storage_account_free (gpointer p)
>   g_hash_table_unref (sa->parameters);
>   g_hash_table_unref (sa->escaped_parameters);
>   g_hash_table_unref (sa->secrets);
>+  g_object_unref (sa->storage);
>   g_slice_free (McdStorageAccount, sa);
> }
> 
>@@ -211,29 +234,6 @@ lookup_account (McdStorage *self,
>   return g_hash_table_lookup (self->accounts, account);
> }
> 
>-static McdStorageAccount *
>-ensure_account (McdStorage *self,
>-    const gchar *account)
>-{
>-  McdStorageAccount *sa = lookup_account (self, account);
>-
>-  if (sa == NULL)
>-    {
>-      sa = g_slice_new (McdStorageAccount);
>-      sa->attributes = g_hash_table_new_full (g_str_hash, g_str_equal,
>-          g_free, (GDestroyNotify) g_variant_unref);
>-      sa->parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
>-          g_free, (GDestroyNotify) g_variant_unref);
>-      sa->escaped_parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
>-          g_free, g_free);
>-      sa->secrets = g_hash_table_new_full (g_str_hash, g_str_equal,
>-          g_free, NULL);
>-      g_hash_table_insert (self->accounts, g_strdup (account), sa);
>-    }
>-
>-  return sa;
>-}
>-
> static gchar *
> get_value (const McpAccountManager *ma,
>     const gchar *account,
>@@ -405,7 +405,9 @@ mcpa_set_attribute (const McpAccountManager *ma,
>     McpAttributeFlags flags)
> {
>   McdStorage *self = MCD_STORAGE (ma);
>-  McdStorageAccount *sa = ensure_account (self, account);
>+  McdStorageAccount *sa = lookup_account (self, account);
>+
>+  g_return_if_fail (sa != NULL);
> 
>   if (value != NULL)
>     {
>@@ -426,7 +428,9 @@ mcpa_set_parameter (const McpAccountManager *ma,
>     McpParameterFlags flags)
> {
>   McdStorage *self = MCD_STORAGE (ma);
>-  McdStorageAccount *sa = ensure_account (self, account);
>+  McdStorageAccount *sa = lookup_account (self, account);
>+
>+  g_return_if_fail (sa != NULL);
> 
>   g_hash_table_remove (sa->parameters, parameter);
>   g_hash_table_remove (sa->escaped_parameters, parameter);
>@@ -449,7 +453,9 @@ set_value (const McpAccountManager *ma,
>     const gchar *value)
> {
>   McdStorage *self = MCD_STORAGE (ma);
>-  McdStorageAccount *sa = ensure_account (self, account);
>+  McdStorageAccount *sa = lookup_account (self, account);
>+
>+  g_return_if_fail (sa != NULL);
> 
>   if (g_str_has_prefix (key, "param-"))
>     {
>@@ -551,8 +557,10 @@ mcd_storage_make_secret (McdStorage *self,
>   if (!g_str_has_prefix (key, "param-"))
>     return;
> 
>+  sa = lookup_account (self, account);
>+  g_return_if_fail (sa != NULL);
>+
>   DEBUG ("flagging %s parameter %s as secret", account, key + 6);
>-  sa = ensure_account (self, account);
>   g_hash_table_add (sa->secrets, g_strdup (key + 6));
> }
> 
>@@ -887,22 +895,15 @@ McpAccountStorage *
> mcd_storage_get_plugin (McdStorage *self,
>     const gchar *account)
> {
>-  GList *store = stores;
>-  McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
>-  McpAccountStorage *owner = NULL;
>+  McdStorageAccount *sa;
> 
>   g_return_val_if_fail (MCD_IS_STORAGE (self), NULL);
>   g_return_val_if_fail (account != NULL, NULL);
> 
>-  for (; store != NULL && owner == NULL; store = g_list_next (store))
>-    {
>-      McpAccountStorage *plugin = store->data;
>-
>-      if (mcp_account_storage_owns (plugin, ma, account))
>-        owner = plugin;
>-    }
>+  sa = lookup_account (self, account);
>+  g_return_val_if_fail (sa != NULL, NULL);
> 
>-  return owner;
>+  return sa->storage;
> }
> 
> /*
>@@ -1563,50 +1564,42 @@ update_storage (McdStorage *self,
>     const gchar *escaped,
>     gboolean secret)
> {
>-  GList *store;
>-  gboolean done = FALSE;
>-  gboolean parameter = g_str_has_prefix (key, "param-");
>   McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
>+  gboolean parameter = g_str_has_prefix (key, "param-");
>+  McdStorageAccount *sa;
>+  const gchar *pn;
> 
>   if (secret)
>     mcd_storage_make_secret (self, account, key);
> 
>-  /* we're deleting, which is unconditional, no need to check if anyone *
>-   * claims this setting for themselves                                 */
>-  if (escaped == NULL)
>-    done = TRUE;
>+  sa = lookup_account (self, account);
>+  g_return_if_fail (sa != NULL);
> 
>-  for (store = stores; store != NULL; store = g_list_next (store))
>+  pn = mcp_account_storage_name (sa->storage);
>+  if (escaped == NULL)
>     {
>-      McpAccountStorage *plugin = store->data;
>-      const gchar *pn = mcp_account_storage_name (plugin);
>+      DEBUG ("MCP:%s -> delete %s.%s", pn, account, key);
>+      mcp_account_storage_delete (sa->storage, ma, account, key);
>+    }
>+  else if (variant != NULL && !parameter &&
>+      mcp_account_storage_set_attribute (sa->storage, ma, account, key, variant,
>+          MCP_ATTRIBUTE_FLAG_NONE))
>+    {
>+      DEBUG ("MCP:%s -> store attribute %s.%s", pn, account, key);
>+    }
>+  else if (variant != NULL && parameter &&
>+      mcp_account_storage_set_parameter (sa->storage, ma, account, key + 6,
>+          variant,
>+          secret ? MCP_PARAMETER_FLAG_SECRET : MCP_PARAMETER_FLAG_NONE))
>+    {
>+      DEBUG ("MCP:%s -> store parameter %s.%s", pn, account, key);
>+    }
>+  else
>+    {
>+      gboolean done;
> 
>-      if (done)
>-        {
>-          DEBUG ("MCP:%s -> delete %s.%s", pn, account, key);
>-          mcp_account_storage_delete (plugin, ma, account, key);
>-        }
>-      else if (variant != NULL && !parameter &&
>-          mcp_account_storage_set_attribute (plugin, ma, account, key, variant,
>-            MCP_ATTRIBUTE_FLAG_NONE))
>-        {
>-          done = TRUE;
>-          DEBUG ("MCP:%s -> store attribute %s.%s", pn, account, key);
>-        }
>-      else if (variant != NULL && parameter &&
>-          mcp_account_storage_set_parameter (plugin, ma, account, key + 6,
>-            variant,
>-            secret ? MCP_PARAMETER_FLAG_SECRET : MCP_PARAMETER_FLAG_NONE))
>-        {
>-          done = TRUE;
>-          DEBUG ("MCP:%s -> store parameter %s.%s", pn, account, key);
>-        }
>-      else
>-        {
>-          done = mcp_account_storage_set (plugin, ma, account, key, escaped);
>-          DEBUG ("MCP:%s -> %s %s.%s",
>-              pn, done ? "store" : "ignore", account, key);
>-        }
>+      done = mcp_account_storage_set (sa->storage, ma, account, key, escaped);
>+      DEBUG ("MCP:%s -> %s %s.%s", pn, done ? "store" : "ignore", account, key);
>     }
> }
> 
>@@ -1688,7 +1681,8 @@ mcd_storage_set_attribute (McdStorage *self,
>   g_return_val_if_fail (attribute != NULL, FALSE);
>   g_return_val_if_fail (!g_str_has_prefix (attribute, "param-"), FALSE);
> 
>-  sa = ensure_account (self, account);
>+  sa = lookup_account (self, account);
>+  g_return_val_if_fail (sa != NULL, FALSE);
> 
>   if (value != NULL)
>     new_v = g_variant_ref_sink (dbus_g_value_build_g_variant (value));
>@@ -1758,7 +1752,8 @@ mcd_storage_set_parameter (McdStorage *self,
>   g_return_val_if_fail (account != NULL, FALSE);
>   g_return_val_if_fail (parameter != NULL, FALSE);
> 
>-  sa = ensure_account (self, account);
>+  sa = lookup_account (self, account);
>+  g_return_val_if_fail (sa != NULL, FALSE);
> 
>   if (value != NULL)
>     {
>@@ -2077,8 +2072,9 @@ mcd_storage_create_account (McdStorage *self,
>     const gchar *identification,
>     GError **error)
> {
>-  GList *store;
>   McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
>+  GList *store;
>+  gchar *account;
> 
>   g_return_val_if_fail (MCD_IS_STORAGE (self), NULL);
>   g_return_val_if_fail (!tp_str_empty (manager), NULL);
>@@ -2093,8 +2089,10 @@ mcd_storage_create_account (McdStorage *self,
> 
>           if (!tp_strdiff (mcp_account_storage_provider (plugin), provider))
>             {
>-              return mcp_account_storage_create (plugin, ma, manager,
>+              account = mcp_account_storage_create (plugin, ma, manager,
>                   protocol, identification, error);
>+              mcd_storage_add_account_from_plugin (self, plugin, account);
>+              return account;
>             }
>         }
> 
>@@ -2106,50 +2104,19 @@ mcd_storage_create_account (McdStorage *self,
> 
>   /* No provider specified, let's pick the first plugin able to create this
>    * account in priority order.
>-   *
>-   * FIXME: This is rather subtle, and relies on the fact that accounts
>-   * aren't always strongly tied to a single plugin.
>-   *
>-   * For plugins that only store their accounts set up specifically
>-   * through them (like the libaccounts/SSO pseudo-plugin,
>-   * McdAccountManagerSSO), create() will fail as unimplemented,
>-   * and we'll fall through to the next plugin. Eventually we'll
>-   * reach the default keyfile+gnome-keyring plugin, or another
>-   * plugin that accepts arbitrary accounts. When set() is called,
>-   * the libaccounts/SSO plugin will reject that too, and again,
>-   * we'll fall through to a plugin that accepts arbitrary
>-   * accounts.
>-   *
>-   * Plugins that will accept arbitrary accounts being created
>-   * via D-Bus (like the default keyfile+gnome-keyring plugin,
>-   * and the account-diversion plugin in tests/twisted)
>-   * should, in principle, implement create() to be successful.
>-   * If they do, their create() will succeed, and later, so will
>-   * their set().
>-   *
>-   * We can't necessarily rely on all such plugins implementing
>-   * create(), because it isn't a mandatory part of the plugin
>-   * API (it was added later). However, as it happens, the
>-   * default plugin returns successfully from create() without
>-   * really doing anything. When we iterate through the accounts again
>-   * to call set(), higher-priority plugins are given a second
>-   * chance to intercept that; so we end up with create() in
>-   * the default plugin being followed by set() from the
>-   * higher-priority plugin. In theory that's bad because it
>-   * splits the account across two plugins, but in practice
>-   * it isn't a problem because the default plugin's create()
>-   * doesn't really do anything anyway.
>    */
>   for (store = stores; store != NULL; store = g_list_next (store))
>     {
>       McpAccountStorage *plugin = store->data;
>-      gchar *ret;
> 
>-      ret = mcp_account_storage_create (plugin, ma, manager, protocol,
>+      account = mcp_account_storage_create (plugin, ma, manager, protocol,
>           identification, error);
> 
>-      if (ret != NULL)
>-        return ret;
>+      if (account != NULL)
>+        {
>+          mcd_storage_add_account_from_plugin (self, plugin, account);
>+          return account;
>+        }
> 
>       g_clear_error (error);
>     }
>@@ -2178,20 +2145,17 @@ void
> mcd_storage_delete_account (McdStorage *self,
>     const gchar *account)
> {
>-  GList *store;
>   McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
>+  McdStorageAccount *sa;
> 
>   g_return_if_fail (MCD_IS_STORAGE (self));
>   g_return_if_fail (account != NULL);
> 
>-  g_hash_table_remove (self->accounts, account);
>-
>-  for (store = stores; store != NULL; store = g_list_next (store))
>-    {
>-      McpAccountStorage *plugin = store->data;
>+  sa = lookup_account (self, account);
>+  g_return_if_fail (sa != NULL);
> 
>-      mcp_account_storage_delete (plugin, ma, account, NULL);
>-    }
>+  mcp_account_storage_delete (sa->storage, ma, account, NULL);
>+  g_hash_table_remove (self->accounts, account);
> }
> 
> /*
>@@ -2205,26 +2169,30 @@ mcd_storage_delete_account (McdStorage *self,
> void
> mcd_storage_commit (McdStorage *self, const gchar *account)
> {
>-  GList *store;
>   McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
>+  GList *store;
> 
>   g_return_if_fail (MCD_IS_STORAGE (self));
> 
>+  if (account != NULL)
>+    {
>+      McdStorageAccount *sa = lookup_account (self, account);
>+
>+      g_return_if_fail (sa != NULL);
>+
>+      DEBUG ("flushing plugin %s %s to long term storage",
>+          mcp_account_storage_name (sa->storage), account);
>+      mcp_account_storage_commit_one (sa->storage, ma, account);
>+      return;
>+    }
>+
>   for (store = stores; store != NULL; store = g_list_next (store))
>     {
>       McpAccountStorage *plugin = store->data;
>-      const gchar *pname = mcp_account_storage_name (plugin);
> 
>-      if (account != NULL)
>-        {
>-          DEBUG ("flushing plugin %s %s to long term storage", pname, account);
>-          mcp_account_storage_commit_one (plugin, ma, account);
>-        }
>-      else
>-        {
>-          DEBUG ("flushing plugin %s to long term storage", pname);
>-          mcp_account_storage_commit (plugin, ma);
>-        }
>+      DEBUG ("flushing plugin %s to long term storage",
>+          mcp_account_storage_name (plugin));
>+      mcp_account_storage_commit_one (plugin, ma, NULL);
>     }
> }
> 
>@@ -2304,18 +2272,19 @@ plugin_iface_init (McpAccountManagerIface *iface,
>   iface->init_value_for_attribute = mcpa_init_value_for_attribute;
> }
> 
>-gboolean
>+void
> mcd_storage_add_account_from_plugin (McdStorage *self,
>     McpAccountStorage *plugin,
>     const gchar *account)
> {
>-  if (!mcp_account_storage_get (plugin, MCP_ACCOUNT_MANAGER (self),
>-      account, NULL))
>-    {
>-      g_warning ("plugin %s disowned account %s",
>-                 mcp_account_storage_name (plugin), account);
>-      return FALSE;
>-    }
>+  g_return_if_fail (MCD_IS_STORAGE (self));
>+  g_return_if_fail (MCP_IS_ACCOUNT_STORAGE (plugin));
>+  g_return_if_fail (account != NULL);
>+  g_return_if_fail (!g_hash_table_contains (self->accounts, account));
> 
>-  return TRUE;
>+  g_hash_table_insert (self->accounts, g_strdup (account),
>+      mcd_storage_account_new (plugin));
>+
>+  /* This will fill our parameters/attributes tables */
>+  mcp_account_storage_get (plugin, MCP_ACCOUNT_MANAGER (self), account, NULL);
> }
>diff --git a/src/mcd-storage.h b/src/mcd-storage.h
>index e440845..dc2435f 100644
>--- a/src/mcd-storage.h
>+++ b/src/mcd-storage.h
>@@ -131,7 +131,7 @@ McpAccountStorage * mcd_storage_get_plugin (McdStorage *storage,
> 
> G_GNUC_INTERNAL void _mcd_storage_store_connections (McdStorage *storage);
> 
>-gboolean mcd_storage_add_account_from_plugin (McdStorage *storage,
>+void mcd_storage_add_account_from_plugin (McdStorage *storage,
>     McpAccountStorage *plugin,
>     const gchar *account);
> 
>diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c
>index eb727a3..7228421 100644
>--- a/tests/twisted/dbus-account-plugin.c
>+++ b/tests/twisted/dbus-account-plugin.c
>@@ -1363,9 +1363,8 @@ test_dbus_account_plugin_commit_one (const McpAccountStorage *storage,
> 
>   DEBUG ("%s", account_name);
> 
>-  /* MC does not call @commit_one with parameter %NULL (meaning "all accounts")
>-   * if we also implement commit(), which, as it happens, we do */
>-  g_return_val_if_fail (account_name != NULL, FALSE);
>+  if (account_name == NULL)
>+    return test_dbus_account_plugin_commit (storage, am);
> 
>   if (!self->active || account == NULL)
>     return FALSE;
>@@ -1574,22 +1573,6 @@ test_dbus_account_plugin_get_restrictions (const McpAccountStorage *storage,
>   return TP_STORAGE_RESTRICTION_FLAG_CANNOT_SET_SERVICE;
> }
> 
>-static gboolean
>-test_dbus_account_plugin_owns (McpAccountStorage *storage,
>-    McpAccountManager *am,
>-    const gchar *account_name)
>-{
>-  TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage);
>-  Account *account = lookup_account (self, account_name);
>-
>-  DEBUG ("%s", account_name);
>-
>-  if (!self->active || account == NULL || (account->flags & UNCOMMITTED_DELETION))
>-    return FALSE;
>-
>-  return TRUE;
>-}
>-
> static void
> account_storage_iface_init (McpAccountStorageIface *iface)
> {
>@@ -1605,11 +1588,9 @@ account_storage_iface_init (McpAccountStorageIface *iface)
>   iface->list = test_dbus_account_plugin_list;
>   iface->ready = test_dbus_account_plugin_ready;
>   iface->delete = test_dbus_account_plugin_delete;
>-  iface->commit = test_dbus_account_plugin_commit;
>   iface->commit_one = test_dbus_account_plugin_commit_one;
>   iface->get_identifier = test_dbus_account_plugin_get_identifier;
>   iface->get_additional_info = test_dbus_account_plugin_get_additional_info;
>   iface->get_restrictions = test_dbus_account_plugin_get_restrictions;
>   iface->create = test_dbus_account_plugin_create;
>-  iface->owns = test_dbus_account_plugin_owns;
> }
>diff --git a/tests/twisted/mcp-account-diversion.c b/tests/twisted/mcp-account-diversion.c
>index 923f51b..9e8bb86 100644
>--- a/tests/twisted/mcp-account-diversion.c
>+++ b/tests/twisted/mcp-account-diversion.c
>@@ -206,8 +206,9 @@ _delete (const McpAccountStorage *self,
> 
> 
> static gboolean
>-_commit (const McpAccountStorage *self,
>-    const McpAccountManager *am)
>+_commit_one (const McpAccountStorage *self,
>+    const McpAccountManager *am,
>+    const gchar *account_name)
> {
>   gsize n;
>   gchar *data;
>@@ -266,7 +267,7 @@ account_storage_iface_init (McpAccountStorageIface *iface,
>   iface->get = _get;
>   iface->set = _set;
>   iface->delete = _delete;
>-  iface->commit = _commit;
>+  iface->commit_one = _commit_one;
>   iface->list = _list;
> }
> 
>--
>cgit v0.9.0.3

Actions: View | Splinter Review

Attachments on bug 71384: 88886 | 93543 | 93548 | 93549

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.