Major architectural points
==========================

Major regression: your gnome-keyring plugin is synchronous, and will block MC completely until the user has responded to the gnome-keyring "unlock?" prompt (if so configured). Sorry, but we can't do this. Blocking in a D-Bus service is very bad - all of MC's users will get D-Bus timeouts while they're waiting. Does your API have any way to implement the gnome-keyring stuff without blocking on it?

I think it would be better if the McdAccountManager had an McdPluginAccount[Manager] in its priv for its entire lifetime, and the key files were in (and owned by) that, rather than in McdAccountManagerPrivate itself; they could be public struct fields within MC itself, so the AM would just have to use self->priv->plugin_api->keyfile and self->priv->plugin_api->secrets. They should be created in the plugin API's _init, so they wouldn't need to be GObject properties at all.

That would avoid the problem we currently have, which is that if a plugin refs the McpPluginAccountManager and hangs onto it, everything will fail horribly.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=ca27379ae53d3c>:

> +#include <src/mcd-signals-marshal.h>
> +#include <src/mcd-debug.h>

libmission-control-plugins is meant to be self-contained (so each plugin is linked against it, and has no undefined symbols). Add any necessary signal marshallers or debug infrastructure to it, rather than parasitizing src/.

Style/bugs/misc
===============

> +#include <glib/gmessages.h>

<glib.h> please. See http://live.gnome.org/GnomeGoals/CleanupGTKIncludes

> +  gboolean (*store) (const McpAccountStorage *,const GKeyFile *, const gchar *);

Space after comma please, and I'd prefer named parameters so they can be documented properly. Talking of which, please document.

> +GList *mcp_account_storage_list (const McpAccountStorage *storage);

Ownership certainly needs documenting here.

> +  guint priority;
...
> +gint mcp_account_storage_priority (const McpAccountStorage *storage);

Is it signed or not? Are numerically-large priorities considered high or low? (From context elsewhere, they're high.)

If the priority is unsigned, then default priority should be positive, so it's possible to implement deliberately-low-priority plugins. I'd prefer it to be signed with 0 as the default, I think - that's nicely obvious.

> +static gint
> +store_prio (gconstpointer a, gconstpointer b)

This would be better called account_store_priority_cmp(), or account_store_cmp(), or account_store_cmp_by_priority() (having strcmp-like functions contain cmp is a good convention).

> +    DEBUG ();

DEBUG ("called") would be more conventional, and almost certainly more portable.

In write_conf, <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=4b213670abff4>:

> +    guint h = 0;

This name isn't clear enough. From usage, it should be called n_handled or num_handled or handled_len or something.

> +    gsize n = 0;

n_groups or num_groups, please.

> +    GList *store;

This could be pushed down into the loop over groups. I like temporary variables to have as small a scope as possible.

> +    for (group = groups[i]; group != NULL; i++)

This isn't right; you're not changing @group for the second and subsequent iterations. You fixed this in the mega-patch at the end.

> +            McpAccountStorage *plugin = store->data;
> +            if (mcp_account_storage_store (plugin, keyfile, group))

Blank line between declarations and code, and between code and if{}, please. In this case the same blank line will do for both :-)

+        for (seen = handled; seen != NULL; seen = g_list_next (seen))
+            g_key_file_remove_group (save, (const gchar *)seen->data, NULL);
+        g_list_free (handled);

Blank line before g_list_free here, for the same reason

>      if (error)
>      {
>         g_warning ("Could not save account data: %s", error->message);
>         g_error_free (error);
> -       return FALSE;
> +        goto finished;
>      }

If you're going to fix the pre-existing bad indentation here (4+3 spaces), please fix it for the other couple of lines too.

>      if (G_UNLIKELY (!ok))
>      {
>         g_warning ("Could not save account data: %s", error->message);
>         g_error_free (error);
> -       return FALSE;
> +           goto finished;
>      }

The existing 4+3 space indentation is wrong. Yours is more wrong :-P (did you insert a tab?)

In mcd_account_manager_init in the same commit:

> +            char *name = account->data;
> +            mcp_account_storage_fetch (plugin, priv->keyfile, name);
> +            g_free (name);

We use the convention that gchar * was allocated with g_malloc, and char * was allocated with "something else" (system malloc, Avahi, whatever), so this should be a gchar *. (Also, blank line.)

Next commit:

> +mcp_account_diversion_la_LDFLAGS = $(mcp_plugin_la_LDFLAGS)

Nice trick, but I'd prefer it if you defined $(test_plugin_ldflags) then referenced that for each plugin.

> +#define DIVERTED_CFG "/tmp/mcp-test-diverted-account-plugin.conf"

I don't like symlink attacks, even in regression tests. Please either use g_get_user_cache_dir() or an environment variable, and set the test environment suitably so the XDG_CACHE_DIR is inside the MC tree (if it isn't already - I think it is).

> +#define PLUGIN_PRIORITY (ACCOUNT_STORAGE_PLUGIN_PRIO_DEFAULT + 100)

I don't see where A_S_P_P_D is defined? It should have a MCP_ prefix, anyway.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=151717b754aa0402>:

> +gboolean mcd_account_parameter_is_secret (McdAccount *self,
> +                                              const gchar *name);

Indentation.

> +    return (param != NULL && param->flags & TP_CONN_MGR_PARAM_FLAG_SECRET);

I'd prefer this written like (p != NULL && (p->f & T) != 0).

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=ca27379ae53d3c>:

> +      _mcd_marshal_VOID__STRING, G_TYPE_NONE,

Use g_cclosure_marshal_VOID__STRING. It looks as though only toggled will need its own marshaller.

> +      g_debug ("%s -> %lu", g_type_name (type), type);
...
> +  DEBUG ();

No thankyou.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=ca27379ae53d>:

> +#define ACCOUNT_STORAGE_PLUGIN_PRIO_DEFAULT 0
> +#define ACCOUNT_STORAGE_PLUGIN_PRIO_KEYRING 65536

Ah, so *this* is where that's defined. It should have a MCP_ prefix, and I'd prefer it if the arbitrary numbers were easy decimal numbers rather than easy binary numbers.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=8f5201cb8>:

McpPluginAccount should probably be called McpPluginAccountManager; it corresponds 1:1 to the AM, not to an account.

> +void
> +mcp_account_parameter_make_secret (const McpAccount *mcpa,

What's this for? The CM tells us whether a particular parameter is secret.

> +#include <mission-control-plugins/request.h>

Why is this in account.h? If you want it for its side-effects (e.g. <glib.h>), please include those directly.

> +      case PROP_KEYFILE:
> +        self->file = g_value_get_pointer (value);
> +        break;

I'm uncomfortable with this because it doesn't ref or copy (GKeyFiles aren't refcounted). See "major architectural points" above.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=eece82e7ce88>:

McdAccountManagerDefault should be called McdDefaultAccountStorage or McdKeyFileAccountStorage or McdFallbackAccountStorage or something. Similarly, McdGnomeKeyringAccountStorage or McdGnomeAccountStorage.

> +#define PLUGIN_PRIORITY ACCOUNT_STORAGE_PLUGIN_PRIO_DEFAULT
> +#define PLUGIN_DESCRIPTION "GKeyFile (default) account storage backend"

I sort of think this should have a negative priority, perhaps called PRIO_FALLBACK, so that user-installed plugins can have PRIO_DEFAULT (perhaps renamed to PRIO_NORMAL or something).

+  base = g_getenv ("MC_ACCOUNT_DIR");
+  if (!base)
+       base = ACCOUNTS_DIR;
+  if (!base)
+       return NULL;

Newline style please.

> +mcd_account_manager_default_new ()

(void) please.

In <http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=0b1bed05249>:

Your patches were nicely broken up until this point, but this one seems to be "badger just about everything to make it work"...

> +    for (p = mcp_list_objects(); p != NULL; p = g_list_next (p))
> +        if (MCP_IS_ACCOUNT_STORAGE (p->data))

Curly brackets around all non-trivial blocks, please.

> +    gchar *grp;

Please avoid uncommon abbreviations. "group" isn't really much longer.

WRT the blocking, we only [potentially] block during initialisation: the dbus name hasn't been taken yet at that point, so no-one can be waiting for a response
from us since we haven't claimed we exist yet.

15:04 < smcv> fledermaus: I assume 
              http://bugs.freedesktop.org/show_bug.cgi?id=26772 is ready for 
              re-review?
15:04 < fledermaus> yes

Reviewed up to, but not including, addition of the libaccounts plugin:

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=7a40fef3fddc9
> +VOID:STRING

You should never need this marshaller: GLib provides an assortment of commonly used marshallers, in particular g_cclosure_marshal_VOID__STRING, and guarantees them as API/ABI.

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=bda422af541
> @@ -85,7 +66,9 @@ plugin_account_manager_dispose (GObject *object)
> ...
> +  g_key_file_free (self->keyfile);
> +  g_key_file_free (self->secrets);

These should be in finalize (GKeyFile isn't a GObject).

> +  return g_object_new (MCD_TYPE_PLUGIN_ACCOUNT_MANAGER, NULL);

I'd prefer a newline before the NULL according to our usual g_object_new style,
even though the number of key/value pairs is zero.

>   object_class->set_property = plugin_account_manager_set_property;

There seems to be no point in having this now?

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=7379cfce83e13
> -    stores = g_list_append (stores, default_storage);

What's this doing in this patch? Was it meant to be in the previous one? (If so, don't bother correcting it... when combined with the previous one, it seems right.)

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=b9a93ce1c1
> +        write_conf (g_object_ref (priv->plugin_manager));
...
>              g_timeout_add_full (G_PRIORITY_HIGH, WRITE_CONF_DELAY, write_conf,
> -                                data, NULL);
> +                                g_object_ref (data), NULL);

At the very least, write_conf() deserves a comment explaining that it steals one ref to its argument. However, I'd prefer it if write_conf was ref-neutral, and the ref was released by the GDestroyNotify parameter to g_timeout_add_full instead.

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=7a3e0dac7f3ff
> +account-storage.lo: mcp-signals-marshal.h mcp-signals-marshal.c

This doesn't seem as though it should be necessary: putting the signals-marshal stuff in BUILT_SOURCES (directly or indirectly) should be sufficient. After the first build, the header dependency will be picked up automatically by automake, and the .c dependency is a lie anyway?

I haven't reviewed the libaccounts plugin itself in detail. In a future branch I'd like the libaccounts and gnome-keyring plugins to be genuine plugins in a new /plugins directory, rather than pseudo-plugins.

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commit;h=19ee4fdb0cf44015
> Make libaccounts support optional via configure, update Makefile.am accordingly.

In future I'd prefer a sequence of patches more like:

1) detect libaccounts in configure (even though nothing uses it)
2) add a libaccounts plugin, and in the same commit, add it to Makefile.am

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=39d63c5958fd86b
> hash-include fixes

I'd have squashed this into the previous patch.

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=cedf80751d6
> +  self->id_name_map =
> +    g_hash_table_new_full (g_direct_hash, g_int_equal, NULL, g_free);

Those don't look right. g_direct_hash means the key is either a pointer compared by equality, or G[INT|UINT|SIZE]_TO_POINTER(x) compared by equality. 

Meanwhile, g_int_equal is usually a trap: it means the key is a pointer to (a struct that starts with) int, compared by equality of *((int *) key).

Since you seem to be using GFOO_TO_POINTER, you want g_direct_hash and g_direct_equal, which can both be abbreviated as NULL.

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=commitdiff;h=fa2264f176ebd
> +      if (strcmp (key, "Enabled") == 0)

Please use tp_strdiff or g_str_equal, to avoid the usual cmp() non-obviousness.

Updated repo with fixes for comments.

Looks good to merge, except that you seem to have left plugin_account_manager_dispose empty, apart from chaining up to the parent implementation - that means there's no point in having it, and you might as well just not have it (so, delete the function, and the line in class_init that puts it in the vtable). Consider that change insta-reviewed.

Comments + response from sso plugin review (work underway to fix some of these)

> General comments:
> 
> libaccounts-glib makes a distinction between global account
> settings, such as username, and service specific settings, such as
> the name of the connection manager. Use
> 
>    ag_account_select_service(account, NULL)
> 
> to select the global settings, and
> 
>    ag_account_select_service(account, "IM");
> 
> for the IM related settings. Also, better to add a #define for the
> "IM" string, in case it changes later (or we might define it in
> libaccounts-glib at a certain point)

I believe the correct service is selected before calls to set parameters 
are made - are there specific cases where this is not the case? If so,
where?

> libaccounts does not modify the accounts DB unless you call
> ag_account_store(); this also implies that you must call it after
> deleting an account, too.

The storage plugins only ever call commit when MC schedules a write, 
the delete operation is only required to delete from the plugins local
cache.

> static gchar *
> _gvalue_to_string (const GValue *val)

> I'm not sure you need this function. maybe enclose it in #ifdef ENABLE_DEBUG ?

The values that come back from libaccounts are GValues of various types,
but the layer above in the mcd_account_manager stores everything as 
strings (this is existing code which I have not altered, so as to present 
effectively no change to the code which ineracts with it, to try to make
the addision of storage plugins as uninvasive as possible)

>       ag_account_get_value (acct, MC_CMANAGER_KEY, &cmanager);
>       ag_account_get_value (acct, MC_PROTOCOL_KEY, &protocol);
> 
>       cman  = _gvalue_to_string (&cmanager);
>       proto = _gvalue_to_string (&protocol);

> No need, just use g_value_get_string()

As I understand it, the GValues that come back don't have to be strings,
so this seems safer.

>         else
>           g_string_append_printf (key, "_%02x", *c);
>       g_string_append_c (key, '0');

> I think tp-glib has a utility function for the escaping.

Arguably the account unique name generation used by the account manager should
be exposed to the plugins - I'll do that instead.

> static const gchar *
> _account_provider_name (const McdAccountManagerSso *sso,
>     const gchar *acct)
> {
>   /* placeholder: we currently only support GTalk, so no logic is required */
>   return "google";
> }

> No, either return NULL or your own string (e.g. "mission-control"):
> because if you use "google" (which is an existing provider) these
> accounts created by MC will show up in the accounts-ui as google
> accounts, which probably is not good.

I see. I will alter this so that only account originating from libaccounts
ever get claimed by the plugin, which means that this won't be a problem.

> static AgService *
> _provider_get_service (const McdAccountManagerSso *sso,
>     const gchar *provider)
> {
>   GList *nth;
> 
>   for (nth = sso->services; nth != NULL; nth = g_list_next (nth))
>     {
>       if (strcmp (ag_service_get_provider (nth->data), provider) == 0)
>         return ag_service_ref (nth->data);
>     }
> 
>   return NULL;
> }

> I can't find any initialization of sso->services.  But instead of
> this function, you could call ag_account_list_services_by_type().

Hmm. Must have dropped a patch on the floor at some point. Will fix.

>    AgAccount *account = ag_manager_get_account (sso->ag_manager, id);
>    const gchar *enabled = ag_account_get_enabled (account) ? "true": "false";

> Note that there are two "enabled" flags that you need to consider:
> one for the glocal account (NULL service) and one for the "IM"
> service.

So they both have to be set for the account ot be considered enabled?

>           mcp_account_manager_set_value (am, name, "Enabled", enabled);
>           mcp_account_manager_set_value (am, name, LIBACCT_ID_KEY, ident);
>           mcp_account_manager_set_value (am, name, MC_CMANAGER_KEY, mc_id[0]);
>           mcp_account_manager_set_value (am, name, MC_PROTOCOL_KEY, mc_id[1]);
>           mcp_account_manager_set_value (am, name, MC_IDENTITY_KEY, name);

> Why are you doing all this? We are the account manager, aren't we?

The manager and protocol, once decided on, shouldn't change. Since some 
plugins might not know about these things, they are placed into the account's
settings so that whichever plugin grbas the account preserves the information
for the whole save/reload round trip, otherwise we might get stuck trying
to deduce this information next time we load an account.

(In reply to comment #8)
> I believe the correct service is selected before calls to set parameters 
> are made - are there specific cases where this is not the case? If so,
> where?

Not really, I just wanted to stress that. :-)

> > libaccounts does not modify the accounts DB unless you call
> > ag_account_store(); this also implies that you must call it after
> > deleting an account, too.
> 
> The storage plugins only ever call commit when MC schedules a write, 
> the delete operation is only required to delete from the plugins local
> cache.

Ah, now I see: I didn't notice the commit() method on the interface.


> > static gchar *
> > _gvalue_to_string (const GValue *val)
> 
> > I'm not sure you need this function. maybe enclose it in #ifdef ENABLE_DEBUG ?
> 
> The values that come back from libaccounts are GValues of various types,
> but the layer above in the mcd_account_manager stores everything as 
> strings (this is existing code which I have not altered, so as to present 
> effectively no change to the code which ineracts with it, to try to make
> the addision of storage plugins as uninvasive as possible)

Ah, I see.
But if you simply initialize your values to G_TYPE_STRING, libaccounts will call g_value_transform() -- I guess that should do the trick.

You'll be out of luck when iterating the account settings, though, because in that case you get the GValue with the type as set in the DB. But I don't think you need to iterate through the settings anyways.

> >       ag_account_get_value (acct, MC_CMANAGER_KEY, &cmanager);
> >       ag_account_get_value (acct, MC_PROTOCOL_KEY, &protocol);
> > 
> >       cman  = _gvalue_to_string (&cmanager);
> >       proto = _gvalue_to_string (&protocol);
> 
> > No need, just use g_value_get_string()
> 
> As I understand it, the GValues that come back don't have to be strings,
> so this seems safer.

If you initialize (like you are doing) the GValue as G_TYPE_STRING, you will definitely get a G_TYPE_STRING; it could be NULL in case something goes wrong, but you can count on it not altering the GValue type.

> I see. I will alter this so that only account originating from libaccounts
> ever get claimed by the plugin, which means that this won't be a problem.

Yes, this seems wise.

> >    AgAccount *account = ag_manager_get_account (sso->ag_manager, id);
> >    const gchar *enabled = ag_account_get_enabled (account) ? "true": "false";
> 
> > Note that there are two "enabled" flags that you need to consider:
> > one for the glocal account (NULL service) and one for the "IM"
> > service.
> 
> So they both have to be set for the account ot be considered enabled?

Yes.

> >           mcp_account_manager_set_value (am, name, "Enabled", enabled);
> >           mcp_account_manager_set_value (am, name, LIBACCT_ID_KEY, ident);
> >           mcp_account_manager_set_value (am, name, MC_CMANAGER_KEY, mc_id[0]);
> >           mcp_account_manager_set_value (am, name, MC_PROTOCOL_KEY, mc_id[1]);
> >           mcp_account_manager_set_value (am, name, MC_IDENTITY_KEY, name);
> 
> > Why are you doing all this? We are the account manager, aren't we?
> 
> The manager and protocol, once decided on, shouldn't change. Since some 
> plugins might not know about these things, they are placed into the account's
> settings so that whichever plugin grbas the account preserves the information
> for the whole save/reload round trip, otherwise we might get stuck trying
> to deduce this information next time we load an account.

I think I'm missing the point here. If one account storage plugin goes missing, all the accounts it provided should disappear, shouldn't they?
Why do you want to retain the manager/protocol information in MC too?


BTW, the documentation strings for mcp_account_manager_parameter_is_secret and mcp_account_manager_get_value are swapped.

(In reply to comment #9)

> If you initialize (like you are doing) the GValue as G_TYPE_STRING, you will
> definitely get a G_TYPE_STRING; it could be NULL in case something goes wrong,
> but you can count on it not altering the GValue type.

But parameters which are initialised from configured defaults (eg the port number) arrive as the configured type (integer, in this case) so I still 
need to 'cast' the GValue. (I discovered this during testing).

> I think I'm missing the point here. If one account storage plugin goes missing,
> all the accounts it provided should disappear, shouldn't they?
> Why do you want to retain the manager/protocol information in MC too?

Accounts can migrate up to higher priority plugins, and plugins can claim
only part of an account, eg when the default storage and the gnome keyring
plugin are th eonly ones loaded, the keyring claims only the secret
parameters and leaves the rest in the default storage - so it's important
for the naming to be consistent. MC keeps the information because it means
that whichever plugin ends up claiming the account details for storage
keeps track of this information - it's basically a way of ensuring that
we never lose track through the save/shutdown/reload cycle.

> BTW, the documentation strings for mcp_account_manager_parameter_is_secret and
> mcp_account_manager_get_value are swapped.

Oops. Will fix.

Ok, I think we're ready to run through review again, issues from last review round should be fixed.

=== unique_name() stuff (these are probably preexisting): ===

it could use tp_asv_get_string()

+  /* add two chars for the "/" */
+  len = strlen (esc_manager) + strlen (esc_protocol) + strlen (esc_base)
+    + base_len + 2;
+  path = g_malloc (len + 5);
+  sprintf (path, "%s%s/%s/%s", MC_ACCOUNT_DBUS_OBJECT_BASE,
+      esc_manager, esc_protocol, esc_base);

what's wrong with g_strdup_printf() ?

=== Other stuff: ===

It'd be nice to squash commits like 8b6c509 and 4b8b72c, where one introduces a typo and the next fixes it, together.

5be4467: when is the ref to sso->manager_interface dropped? if the answer is "never", that's a bug :)

3fe0325: might be nice to say what the type error is in the commit message. it's actually casting const away afaict. Maybe squash this into the commit that introduced that type error? Ditto a8dd436 and 
236b156

AIUI this is merged and will be in 5.3.3, although we might call it 5.5.0 to make it easier to make bugfix releases of 5.3.2 (in practice many distros are using that version, making it a de facto stable branch).