Bug 27727

Proposed fixes:

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=shortlog;h=refs/heads/keyring-suppress-incomplete-accounts

or 

http://git.collabora.co.uk/?p=user/vivek/telepathy-mission-control;a=shortlog;h=refs/heads/keyring-suppress-incomplete-accounts-B

are up for review.

Regarding keyring-suppress-incomplete-accounts, the virtual method should be called "has_account".

keyring-suppress-incomplete-accounts-B looks OK, although there's a lot of code churn so it's hard to see how much is original.

Sjoerd had some quite strong opinions about this, so I'll talk to him when he's next in the office about which branch to use...

The symptom has been fixed independently of those branches (by having the keyfile and gnome-keyring plugins not be independent), so the remaining bug here is: although in practice accounts are no longer split between plugins by any existing plugin, the API doesn't guarantee that accounts won't get split in this way. Vivek will fix the API eventually.

(In reply to comment #3)
> Vivek will fix the API eventually.

[citation needed]

Xavier might be doing this, though; we've just broken plugin ABI, so it seems a good time.

Created attachment 89208 [details] [review]
01/26] Remove all notion of secret parameter

From: Xavier Claessens <xavier.claessens@collabora.co.uk>

We now depend on SASLAuthentication for handling secret, and MC
does not have gnome-keyring anymore.

[Adjusted to apply before other storage API changes -smcv]

Created attachment 89209 [details] [review]
02/26] McpAccountStorage: merge commit and commit_one into one  function

Based on part of a patch by Xavier Claessens.

Created attachment 89210 [details] [review]
03/26] Remove mcp_account_storage_set()

_set_attribute() and _set_parameter() are now mandatory for writable
storage plugins.

Note that most of the keyfile escaping code is still needed to help
plugins to read their old keyfile values.

[adjusted to apply earlier in the branch; left in the code that
detects whether mcd_storage_set_parameter() is a no-op for an
escaped parameter; took out mcp_account_storage_emit_created
documentation fix -smcv]

Created attachment 89211 [details] [review]
04/26] mcp_account_storage_emit_created: fix doc-comment

Originally part of a commit by Xavier Claessens.

Created attachment 89212 [details] [review]
05/26] McpAccountStorage: have a default implementation for  every method

Based on a patch by Xavier Claessens.

Created attachment 89213 [details] [review]
06/26] Remove unused code

[adjusted to apply at a different position in the branch -smcv]

Created attachment 89214 [details] [review]
07/26] mcp_account_manager_list_keys: remove

This is unused, and plugins don't have any good reason to call it.

Created attachment 89215 [details] [review]
08/26] mcd_storage_dup_attributes: remove

Nothing calls it, and getting rid of it is a step towards having
McdStorage just be a "view".

Created attachment 89216 [details] [review]
09/26] mcd_account_delete: be primarily responsible for  emitting Removed

We still don't get here if disposed early, though.

Created attachment 89217 [details] [review]
10/26] mcd_account_delete: convert into  mcd_account_delete_async

Created attachment 89218 [details] [review]
11/26] mc-debug-server: don't exit when disconnected from  system bus

The session bus is our fake system bus, too. We exit when libdbus tells
us we have disconnected, and ignore both the possible ways in which
GDBus can kill us, in order to get coverage stats.

Created attachment 89219 [details] [review]
12/26] McdStorage: adjust IdentifyAccount error behaviour

We were ignoring failures if they were NotImplemented or ServiceUnknown,
but thinking about it more, we should probably ignore "most" errors
here: the only errors that should abort the account-creation attempt
are those that indicate that the intended parameters are unusable,
namely InvalidArgument and InvalidHandle (in its secondary role as
"invalid identifier-that-corresponds-to-a-handle").

Created attachment 89220 [details] [review]
13/26] Add some missing test coverage: IdentifyAccount failing  hard

Created attachment 89221 [details] [review]
14/26] McdAccountManager: don't double-delete accounts from  storage

McdAccount (via mcd_account_delete_async) is responsible for deleting
itself from storage if necessary. McdAccountManager shouldn't
second-guess it.

Created attachment 89222 [details] [review]
15/26] Call set_attribute, set_parameter to delete them

It really doesn't make a great deal of sense to use the same callback
to delete individual keys, and to delete accounts.

McdAccountManagerDefault already dealt with that case, but
the two test plugins didn't.

Created attachment 89223 [details] [review]
16/26] Make account deletion async (sort of), and imply a  selective commit

This means we don't need to commit separately after each deletion,
and means account plugins don't have to have the concept of flagging
an account for "delete this later" - much rejoicing.

It also has the incidental benefit that we no longer use the C++
reserved word 'delete' in a header file.

Created attachment 89224 [details] [review]
17/26] Make mcp_account_storage_create() mandatory

This means we can (finally) track which plugin "owns" which account
in a reliable way.

Created attachment 89225 [details] [review]
18/26] McdStorage: remove "owns" method

We now know whose account it is, without having to do this.

Created attachment 89226 [details] [review]
19/26] mcp_account_storage_set_*: return whether anything  changed

The plugins are better-placed to do this than McdStorage: they know
their own storage format, after all.

Created attachment 89227 [details] [review]
20/26] Thread expected GVariant types through to  mcd_storage_get_*

We're going to need this soon, in order to advise plugins how best
to unescape the value. The result is still a GValue, for now.

Created attachment 89228 [details] [review]
21/26] mcp_account_manager_unescape_variant_from_keyfile: add

I want to push responsibility for unescaping into storage plugins.

Created attachment 89229 [details] [review]
22/26] dbus-account-plugin: update internal cache correctly

The backend-makes-changes test replaces an typed parameter with an
untyped parameter. We correctly signalled the change to MC,
but didn't update the internal cache; the rather strange way in
which MC receives attributes/parameters masked this bug.

Created attachment 89230 [details] [review]
23/26] mcp_account_storage_get: replace with get_attribute,  get_parameter

The old API in which plugins poked new values into the McdStorage
was non-obvious, and also fundamentally incompatible with the idea
that each account is owned by at most one plugin: if an account
in a high-priority plugin is masked by one in a low-priority plugin,
the McdStorage can't prevent the low-priority plugin from changing
its effective attribute and parameter values.

Created attachment 89231 [details] [review]
24/26] McdStorage: when acting on one account, only store it  in its plugin

Created attachment 89232 [details] [review]
25/26] McdAccount: have a ref to the storage plugin from  construct time onwards

This will make the account more self-contained.

Created attachment 89233 [details] [review]
[26/26] McdAccountManager: ignore altered-one, deleted, toggled  from wrong plugin

Now that the account knows its own storage plugin, this is pretty easy.

(In reply to comment #9)
> Created attachment 89212 [details] [review] [review]
> 05/26] McpAccountStorage: have a default implementation for  every method
> 
> Based on a patch by Xavier Claessens.

I don't think we want default implementation for list() and get(), they are still needed for any useful storage, no?

(In reply to comment #14)
> Created attachment 89217 [details] [review] [review]
> 10/26] mcd_account_delete: convert into  mcd_account_delete_async

migrate_delete_account_cb() looks weird, why don't you put directly mcd_account_delete_debug_cb as callback and free the MigrateCtx before the async call?

While we are adding async API, why omitting the GCancellable? I think you can just have one and put it on your GTask, it will do the right thing for you.

(In reply to comment #15)
> Created attachment 89218 [details] [review] [review]
> 11/26] mc-debug-server: don't exit when disconnected from  system bus
> 
> The session bus is our fake system bus, too. We exit when libdbus tells
> us we have disconnected, and ignore both the possible ways in which
> GDBus can kill us, in order to get coverage stats.

Not sure to understand why MC would ever create a system bus, modules doing it behind our back? You can also just use g_assert_no_error() instead of more complex error handling, no?

(In reply to comment #20)
> Created attachment 89223 [details] [review] [review]
> 16/26] Make account deletion async (sort of), and imply a  selective commit
> 
> This means we don't need to commit separately after each deletion,
> and means account plugins don't have to have the concept of flagging
> an account for "delete this later" - much rejoicing.
> 
> It also has the incidental benefit that we no longer use the C++
> reserved word 'delete' in a header file.

mcp_account_storage_delete_finish()'s doc should tell about the default implementation. Do we recommend to override _finish() when _async is overriden, or can we state that if _async is implemented using GTask then the default _finish is good? I can guess your answer on this, but better to say it explicitly in the doc, no?

(In reply to comment #21)
> Created attachment 89224 [details] [review] [review]
> 17/26] Make mcp_account_storage_create() mandatory
> 
> This means we can (finally) track which plugin "owns" which account
> in a reliable way.

In mcd_storage_add_account_from_plugin() I would keep the comment my branch adds:
/* This will fill our parameters/attributes tables */ because it's nowhere close to obvious.

Comment on attachment 89226 [details] [review]
19/26] mcp_account_storage_set_*: return whether anything  changed

Review of attachment 89226 [details] [review]:
-----------------------------------------------------------------

::: src/mcd-account-manager-default.c
@@ +182,3 @@
>  
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

I would make it g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because we are no longer setting on all backends. So when MC tells a plugin to set a value, the storage must know about that account.

@@ +194,5 @@
> +  else
> +    {
> +      GVariant *old;
> +
> +      sa = ensure_stored_account (amd, account);

ensure_stored_account() should only be called from _create(), right? Same as above, I would g_return_val_if_fail().

@@ +203,5 @@
> +          /* it might still be in untyped_parameters? */
> +          const gchar *escaped = g_hash_table_lookup (sa->untyped_parameters,
> +              parameter);
> +          gchar *new = mcp_account_manager_escape_variant_for_keyfile (
> +              am, val);

If we have an untyped param, now is a good time to move to to sa->parameters and consider that to be a change.

@@ +243,5 @@
> +    {
> +      sa = lookup_stored_account (amd, account);
> +
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

same as above, g_return_val_if_fail()

@@ +255,2 @@
>  
> +      sa = ensure_stored_account (amd, account);

same as above: s/ensure/lookup/ and g_return_val_if_fail()

Created attachment 89237 [details] [review]
diff between master-without-all-revert and Simon's branch

Easier to review than having to re-review the changes I already did.

Comment on attachment 89237 [details] [review]
diff between master-without-all-revert and Simon's branch

Review of attachment 89237 [details] [review]:
-----------------------------------------------------------------

Here is my full review, it repeat what I did in patch-by-patch, but add a few stuff I think.

::: mission-control-plugins/account-storage.c
@@ +59,1 @@
>   *   iface->delete = foo_plugin_delete;

Needs to be updated for delete_async/finish

@@ +157,2 @@
>  {
> +  return NULL;

I don't think we want a default for list()

@@ +207,5 @@
> +    const GVariantType *type,
> +    McpParameterFlags *flags)
> +{
> +  return NULL;
> +}

I'm not sure if we want a default for get_attribute/parameter. Does it make sense for any storage to have no attribute and/or no param?

@@ +470,3 @@
>   *
> + * There is a default implementation, which just returns %NULL.
> + * All account storage plugins must override this method.

If all plugins must override, better not have a default then, so we get a warn in g_return_val_if_fail (iface->get_attribute != NULL, FALSE); below.

@@ +508,5 @@
> + *
> + * Retrieve a parameter.
> + *
> + * There is a default implementation, which just returns %NULL.
> + * All account storage plugins must override this method.

likewise.

@@ -603,4 @@
>  {
>    McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
>  
> -  SDEBUG (storage, "");

why removing this debug? All other methods still have it.

@@ +739,3 @@
>   */
>  gboolean
> +mcp_account_storage_delete_finish (McpAccountStorage *storage,

Doc should tell about default implementation? Probably saying that it should not reply on it if _async() has been overridden. Or tell it's to not override if _async() uses a GTask?

::: src/mcd-account-manager-default.c
@@ +186,5 @@
> +
> +      sa = lookup_stored_account (amd, account);
> +
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

I would g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because MC now guarantees that it calls _set_parameter() on the right storage only.

@@ +200,5 @@
> +  else
> +    {
> +      GVariant *old;
> +
> +      sa = ensure_stored_account (amd, account);

ensure_stored_account() should be called only from _create().

I would do:

sa = lookup_stored_account (amd, account);
g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);

if (val == NULL)
  {
    ...
  }
else
  {
    ...
  }

@@ +214,5 @@
> +
> +          if (!tp_strdiff (escaped, new))
> +            {
> +              g_free (new);
> +              return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

Can't we move the untyped param to sa->parameters now, and return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED?

@@ +251,3 @@
>  
> +      if (sa == NULL)
> +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;

same as above

@@ +258,5 @@
>    else
>      {
> +      GVariant *old;
> +
> +      sa = ensure_stored_account (amd, account);

same as above

@@ +290,1 @@
>    if (sa == NULL || sa->absent)

Not sure what is sa->absent, but sa==NULL should be a g_return_val_if_fail (sa != NULL, NULL)

@@ +290,2 @@
>    if (sa == NULL || sa->absent)
>      return FALSE;

s/FALSE/NULL/

@@ +313,3 @@
>  
> +  if (sa == NULL || sa->absent)
> +    return FALSE;

same as above

@@ +366,5 @@
>    if (sa == NULL || sa->absent)
>      {
>        /* Apparently we never had this account anyway. The plugin API
>         * considers this to be "success". */
> +      g_task_return_boolean (task, TRUE);

Not sure... that would be a sign that something is broken in MC's representation of accounts. I would add a g_warning() but still return success for the operation because there is no reason to fail the dbus call initiated from client side.

@@ +392,2 @@
>  
> +  for (iter = g_get_system_data_dirs ();

we have distro-installed accounts?

@@ +422,5 @@
>      }
>  
> +  /* clean up the mess */
> +  g_hash_table_remove (amd->accounts, account);
> +  mcp_account_storage_emit_deleted (self, account);

I'm not sure, but shouldn't this be under the finally: ?

::: src/mcd-account-manager.c
@@ +294,5 @@
> +    }
> +    else
> +    {
> +        WARNING ("%s", error->message);
> +        goto finish;

error is leaked

@@ +1233,5 @@
>      DEBUG ("Account %s migrated, removing it",
>             mcd_account_get_unique_name (ctx->account));
>  
> +    mcd_account_delete_async (ctx->account, MCD_DBUS_PROP_SET_FLAG_NONE,
> +                              migrate_delete_account_cb, ctx);

Free ctx from here and use mcd_account_delete_debug_cb

::: src/mcd-storage.c
@@ +505,1 @@
>    while (store != NULL)

Now that we have a normal loop, I think it would look even better with:
for (store = stores; store != NULL; store = store->next)

@@ +547,2 @@
>  {
> +  return g_hash_table_ref (self->accounts);

tbh the ref here is just asking for leaks. Either deep-copy or return internal pointer with (transfer none).

@@ +1274,4 @@
>      }
>    else
>      {
> +      g_assert_not_reached ();

isn't it easier to have g_return_val_if_fail(plugin != NULL, FALSE); to remove a big if block?

@@ +1782,5 @@
> +
> +  if (mcp_account_storage_delete_finish (MCP_ACCOUNT_STORAGE (source),
> +        res, &error))
> +    {
> +      DEBUG ("deleted account %s", (const gchar *) user_data);

I would add a gchar *account_name = user_data; declaration.

@@ +1956,5 @@
> +    const gchar *escaped,
> +    const GVariantType *type,
> +    GError **error)
> +{
> +  return mcd_keyfile_unescape_variant (escaped, type, error);

That function can be inlined here, it's the only user.

(In reply to comment #31)
> > 05/26] McpAccountStorage: have a default implementation for  every method
> > 
> > Based on a patch by Xavier Claessens.
> 
> I don't think we want default implementation for list() and get(), they are
> still needed for any useful storage, no?

Fair point. I can delete the default implementations for things that any useful plugin must override, if you'd prefer.

I think that means:

* list
* owns (but I deleted that later in the branch anyway)
* get (but I deleted that later)
* get_attribute (must be able to get at least "manager" and "protocol")
* get_parameter (well I suppose in theory a CM might not need parameters but... no)

(In reply to comment #32)
> (In reply to comment #14)
> > 10/26] mcd_account_delete: convert into  mcd_account_delete_async
> 
> migrate_delete_account_cb() looks weird, why don't you put directly
> mcd_account_delete_debug_cb as callback and free the MigrateCtx before the
> async call?

Because migrate_ctx_free() isn't 100% a free-function: it also releases a "lock" that prevents MC from saying it's ready until all migrations have completed.

> While we are adding async API, why omitting the GCancellable? I think you
> can just have one and put it on your GTask, it will do the right thing for
> you.

Hmm. I'm not sure what our rule should be on this: you can't *actually* cancel deletion of an account, only the callback. Then again, g_dbus_connection_call() takes a cancellable, and "send a D-Bus message" is pretty much the archetype of things you can't cancel, so... yeah, OK, you're right.

(In reply to comment #33)
> Not sure to understand why MC would ever create a system bus, modules doing
> it behind our back?

GNetworkMonitor might use the system bus, and McdConnectivityMonitor certainly does (to (try to) talk to systemd-logind).

> You can also just use g_assert_no_error() instead of
> more complex error handling, no?

Yeah, I suppose so, since it's just for the tests.

(In reply to comment #34)
> mcp_account_storage_delete_finish()'s doc should tell about the default
> implementation.

Sure.

> Do we recommend to override _finish() when _async is
> overriden, or can we state that if _async is implemented using GTask then
> the default _finish is good? I can guess your answer on this, but better to
> say it explicitly in the doc, no?

I think we should say you have to override neither or both: in some older Telepathy APIs we had "the default uses a GSimpleAsyncResult", and now we're regretting that because of GTask :-)

(In reply to comment #35)
> > 17/26] Make mcp_account_storage_create() mandatory
> 
> In mcd_storage_add_account_from_plugin() I would keep the comment my branch
> adds:
> /* This will fill our parameters/attributes tables */ because it's nowhere
> close to obvious.

OK, but only if I'm rebasing these changes in - later in the branch, that strange setup disappears anyway.

(In reply to comment #38)
> ::: mission-control-plugins/account-storage.c
> @@ +59,1 @@
> >   *   iface->delete = foo_plugin_delete;
> 
> Needs to be updated for delete_async/finish

Yeah.

> I don't think we want a default for list()
...
> I'm not sure if we want a default for get_attribute/parameter. Does it make
> sense for any storage to have no attribute and/or no param?

Already noted

> @@ -603,4 @@
> >  {
> >    McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage);
> >  
> > -  SDEBUG (storage, "");
> 
> why removing this debug? All other methods still have it.

Your "simplify a bit plugin API" added it, so it was lost in the revert. I'll put it back.

> I would g_return_val_if_fail (sa != NULL,
> MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because MC now guarantees that it
> calls _set_parameter() on the right storage only.

Yeah, makes sense. Or I was wondering whether to change the signature of most McdStorage methods so they take the plugin as an argument, now that McdAccount always knows its own plugin; then we could get rid of McdStorage->accounts altogether.

> > +      GVariant *old;
> > +
> > +      sa = ensure_stored_account (amd, account);
> 
> ensure_stored_account() should be called only from _create().

Makes sense. I'll see whether the tests still pass...

> > +          if (!tp_strdiff (escaped, new))
> > +            {
> > +              g_free (new);
> > +              return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> Can't we move the untyped param to sa->parameters now, and return
> MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED?

One of the regression tests asserts that this won't be considered to be a write - but you're probably right, we should consider changing that test.

> >    if (sa == NULL || sa->absent)
> 
> Not sure what is sa->absent, but sa==NULL should be a g_return_val_if_fail
> (sa != NULL, NULL)

sa->absent means the account is an empty file in a "higher-priority" directory, created in order to "mask" an account in a "lower-priority" directory (because we can't/shouldn't write to lower-priority directories - see my answer to your query about g_get_system_data_dirs() below).

Yes, now that McdStorage tracks plugin "ownership" better, this sort of thing can turn into a g_return*if_fail. I'd prefer to append that to the branch rather than rebasing it into the middle, because we only got that guarantee a couple of commits from the end.

> @@ +290,2 @@
> >    if (sa == NULL || sa->absent)
> >      return FALSE;
> 
> s/FALSE/NULL/

Good point.

> @@ +366,5 @@
> >    if (sa == NULL || sa->absent)
> >      {
> >        /* Apparently we never had this account anyway. The plugin API
> >         * considers this to be "success". */
> > +      g_task_return_boolean (task, TRUE);
> 
> Not sure... that would be a sign that something is broken in MC's
> representation of accounts. I would add a g_warning() but still return
> success for the operation because there is no reason to fail the dbus call
> initiated from client side.

This does happen in the regression tests, now that delete_async is expected to emit ::deleted (which it naturally ought to, I think - I don't like signals that are emitted or not depending on the reason for a change). ::deleted ends up calling back into delete_async.

We might be able to get rid of that by obeying the "already in storage" flag better?

> @@ +392,2 @@
> >  
> > +  for (iter = g_get_system_data_dirs ();
> 
> we have distro-installed accounts?

I hope not, but that's not actually all that g_get_system_data_dirs() returns: it also returns anything the user has put in $XDG_DATA_DIRS. So for instance you could have:

    XDG_DATA_HOME=/home/me/.local/share
    XDG_DATA_DIRS=/nfs/home/me/.local/share:/usr/local/share:/usr/share

I realize that doesn't necessarily make a huge amount of sense for accounts either, but I think a lot of the value of the XDG base directories is that you can rely on them being the same everywhere, so I'd prefer to do this Right™.

> > +  /* clean up the mess */
> > +  g_hash_table_remove (amd->accounts, account);
> > +  mcp_account_storage_emit_deleted (self, account);
> 
> I'm not sure, but shouldn't this be under the finally: ?

No, I don't think it should. If we fail to unlink the file representing the account, and thus fail the async operation, I think we should continue to believe that the account exists. The McdAccount will continue to exist, too, and I don't think we want those getting out of sync.

> > +        WARNING ("%s", error->message);
> > +        goto finish;
> 
> error is leaked

Good catch, will fix.

> @@ +1233,5 @@
> >      DEBUG ("Account %s migrated, removing it",
> >             mcd_account_get_unique_name (ctx->account));
> >  
> > +    mcd_account_delete_async (ctx->account, MCD_DBUS_PROP_SET_FLAG_NONE,
> > +                              migrate_delete_account_cb, ctx);
> 
> Free ctx from here and use mcd_account_delete_debug_cb

As noted above: good guess, but no.

> ::: src/mcd-storage.c
> @@ +505,1 @@
> >    while (store != NULL)
> 
> Now that we have a normal loop, I think it would look even better with:
> for (store = stores; store != NULL; store = store->next)

Yes.

> > +  return g_hash_table_ref (self->accounts);
> 
> tbh the ref here is just asking for leaks. Either deep-copy or return
> internal pointer with (transfer none).

OK, if you say so.

> isn't it easier to have g_return_val_if_fail(plugin != NULL, FALSE); to
> remove a big if block?

I didn't want to re-indent too much, to avoid making review even harder than it was already going to be. I'll fix the coding style next.

> > +  if (mcp_account_storage_delete_finish (MCP_ACCOUNT_STORAGE (source),
> > +        res, &error))
> > +    {
> > +      DEBUG ("deleted account %s", (const gchar *) user_data);
> 
> I would add a gchar *account_name = user_data; declaration.

Reasonable.

> @@ +1956,5 @@
> > +    const gchar *escaped,
> > +    const GVariantType *type,
> > +    GError **error)
> > +{
> > +  return mcd_keyfile_unescape_variant (escaped, type, error);
> 
> That function can be inlined here, it's the only user.

It is? OK, fine. I think MC used it internally in some intermediate version of this branch.

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=account-rework-27727-round2 fixes some of the simpler things you pointed out. Still looking into the more involved stuff.

round2 looks good so far.

I think <http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=account-rework-27727-round3> fixes all your review comments.

Looks as though there are still a couple of things I missed.

(In reply to comment #32)
> migrate_delete_account_cb() looks weird, why don't you put directly
> mcd_account_delete_debug_cb as callback and free the MigrateCtx before the
> async call?

Not done, I explained why above.

> While we are adding async API, why omitting the GCancellable? I think you
> can just have one and put it on your GTask, it will do the right thing for
> you.

Done.

(In reply to comment #33)
> Not sure to understand why MC would ever create a system bus, modules doing
> it behind our back?

I explained why above.

> You can also just use g_assert_no_error() instead of
> more complex error handling, no?

Done.

(In reply to comment #34)
> mcp_account_storage_delete_finish()'s doc should tell about the default
> implementation.

I said in delete_async that if you override one, you need to override both. Is that sufficient?

(In reply to comment #35)
> In mcd_storage_add_account_from_plugin() I would keep the comment my branch
> adds:
> /* This will fill our parameters/attributes tables */ because it's nowhere
> close to obvious.

The relevant code went away, so, not needed.

(In reply to comment #36)
> I would make it g_return_val_if_fail (sa != NULL,
> MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because we are no longer setting on
> all backends.

Done.

> ensure_stored_account() should only be called from _create(), right? Same as
> above, I would g_return_val_if_fail().

Done; it's only called from create() and load() now.

> If we have an untyped param, now is a good time to move to to sa->parameters
> and consider that to be a change.

**** not done yet ****

> > +      if (sa == NULL)
> > +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> same as above, g_return_val_if_fail()

Done

> > +      sa = ensure_stored_account (amd, account);
> 
> same as above: s/ensure/lookup/ and g_return_val_if_fail()

Done

(In reply to comment #38)
> >   *   iface->delete = foo_plugin_delete;
> 
> Needs to be updated for delete_async/finish

Done

> I don't think we want a default for list()

Fixed

> I'm not sure if we want a default for get_attribute/parameter

Deleted

> If all plugins must override, better not have a default then

Fixed

> > + * All account storage plugins must override this method.
> 
> likewise.

Fixed

> why removing this debug? All other methods still have it.

Added (and improved)

> Doc should tell about default implementation? Probably saying that it should
> not reply on it if _async() has been overridden.

Done

> I would g_return_val_if_fail (sa != NULL,
> MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); because MC now guarantees that it
> calls _set_parameter() on the right storage only.

Done

> ensure_stored_account() should be called only from _create().

Done (well, it's also called from load())

> sa = lookup_stored_account (amd, account);
> g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);
> 
> if (val == NULL)
>   {
>     ...
>   }
> else
>   {
>     ...
>   }

**** not yet done ****

> Can't we move the untyped param to sa->parameters now, and return
> MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED?

**** not yet done ****

> > +      if (sa == NULL)
> > +        return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED;
> 
> same as above

Done

> > +      sa = ensure_stored_account (amd, account);
> 
> same as above

Done

> >    if (sa == NULL || sa->absent)
> 
> Not sure what is sa->absent, but sa==NULL should be a g_return_val_if_fail
> (sa != NULL, NULL)

Done

> >    if (sa == NULL || sa->absent)
> >      return FALSE;
> 
> s/FALSE/NULL/

I think I fixed all these, let me know if there are more

> >        /* Apparently we never had this account anyway. The plugin API
> >         * considers this to be "success". */
> > +      g_task_return_boolean (task, TRUE);
> 
> Not sure... that would be a sign that something is broken in MC's
> representation of accounts

Fixed!

> @@ +392,2 @@
> >  
> > +  for (iter = g_get_system_data_dirs ();
> 
> we have distro-installed accounts?

Explained above, no action taken

> > +  /* clean up the mess */
> > +  g_hash_table_remove (amd->accounts, account);
> > +  mcp_account_storage_emit_deleted (self, account);
> 
> I'm not sure, but shouldn't this be under the finally

Explained above why not

> > +        WARNING ("%s", error->message);
> > +        goto finish;
> 
> error is leaked

Fixed

> > +    mcd_account_delete_async (ctx->account, MCD_DBUS_PROP_SET_FLAG_NONE,
> > +                              migrate_delete_account_cb, ctx);
> 
> Free ctx from here and use mcd_account_delete_debug_cb

Explained why not

> Now that we have a normal loop, I think it would look even better with:
> for (store = stores; store != NULL; store = store->next)

Done

> tbh the ref here is just asking for leaks. Either deep-copy or return
> internal pointer with (transfer none).

Done, (transfer none)

> isn't it easier to have g_return_val_if_fail(plugin != NULL, FALSE); to
> remove a big if block?

Re-indented

> > +      DEBUG ("deleted account %s", (const gchar *) user_data);
> 
> I would add a gchar *account_name = user_data; declaration.

Done

> > +  return mcd_keyfile_unescape_variant (escaped, type, error);
> 
> That function can be inlined here, it's the only user.

Done

Missing things are:

> If we have an untyped param, now is a good time to move to to sa->parameters
> and consider that to be a change.

It breaks a regression test, which is trying to assert that if you have an account in a low-priority location, just starting MC isn't enough to get it copied into a higher-priority location. The same test verifies that keyfile-escaped parameters (UntypedParameters) are unescaped correctly.

For that test to check what it's meant to check, I need to split it into "the one that is in a low-priority location" and "the one that has old-style parameters".

and...

> sa = lookup_stored_account (amd, account);
> g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);
> 
> if (val == NULL)
>   {
>     ...
>   }
> else
>   {
>     ...
>   }

Which plugin is this? If it's McdAccountManagerDefault, I think I might have actually fixed that while fixing your other review feedback.

(In reply to comment #45)
> > sa = lookup_stored_account (amd, account);
> > g_return_val_if_fail (sa != NULL, MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED);
> > 
> > if (val == NULL)
> >   {
> >     ...
> >   }
> > else
> >   {
> >     ...
> >   }
> 
> Which plugin is this? If it's McdAccountManagerDefault, I think I might have
> actually fixed that while fixing your other review feedback.

You fixed it already ;-)

0e7da13f3d4a1607372420fedca1124046b8716c _mcd_account_manager_setup:
Nitpicking: you iterate on hash table values, but the key is already the unique_name, no? so you could just take the key instead of calling mcd_account_get_unique_name().

The rest looks good to me.

a231756bd832064d531195656df1110c63575717, commit msg says: "We should never have get_something(), commit() with a non-NULL account"
s/non-// or s/never/always/ if I understand correctly, right?

Added 2 trivial patches on top of you -round3 branch:
http://cgit.collabora.com/git/user/xclaesse/telepathy-mission-control.git/log/?h=account-rework-27727-round3

(In reply to comment #48)
> Added 2 trivial patches on top of you -round3 branch:
> http://cgit.collabora.com/git/user/xclaesse/telepathy-mission-control.git/
> log/?h=account-rework-27727-round3

They look fine.

Still to be done for this branch, then:

(In reply to comment #45)
> > If we have an untyped param, now is a good time to move to to sa->parameters
> > and consider that to be a change.
> 
> It breaks a regression test, which is trying to assert that if you have an
> account in a low-priority location, just starting MC isn't enough to get it
> copied into a higher-priority location. The same test verifies that
> keyfile-escaped parameters (UntypedParameters) are unescaped correctly.
> 
> For that test to check what it's meant to check, I need to split it into
> "the one that is in a low-priority location" and "the one that has old-style
> parameters".

Still to do

(In reply to comment #46)
> 0e7da13f3d4a1607372420fedca1124046b8716c _mcd_account_manager_setup:
> Nitpicking: you iterate on hash table values, but the key is already the
> unique_name, no? so you could just take the key instead of calling
> mcd_account_get_unique_name().

Yes, I think so. Still to do.

(In reply to comment #47)
> a231756bd832064d531195656df1110c63575717, commit msg says: "We should never
> have get_something(), commit() with a non-NULL account"
> s/non-// or s/never/always/ if I understand correctly, right?

No, I think I was right in the commit message. Read it as:

we should never have [any of]
  * get_something()
  * commit with a non-NULL account, or
  * set_something()
called on us while [either of]
  * inactive, or
  * with a nonexistent account

Here is (maybe) a better phrasing:

Methods that act on a single account should always be called with an account that actually exists - if it didn't, where did you get its name from? - so treat it as an error if they are called with a nonexistent account.

commit() has a dual role here: the method that takes a non-NULL account (which acts on a single account, as described) and the method that takes NULL for the account name (which is OK to call at any time, to commit all of our 0-or-more accounts).

While we are not active, we don't claim to have any accounts at all, so treat it as an error if any such method is called while inactive.

One exception (for now) is that it's less bad if delete_async() is called for an account that doesn't exist, because in that case, the desired state "that account doesn't exist" has already been reached. A later commit changes this to be an error.

(In reply to comment #49)
> > For that test to check what it's meant to check, I need to split it into
> > "the one that is in a low-priority location" and "the one that has old-style
> > parameters".

Some progress on chopping up the tests:

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=account-rework-27727-round4

(rebased, commits since yours are new)

I haven't implemented automatic migration from untyped to typed parameters yet; that's next.

I also have a sketch of an Empathy GOA plugin that should be compilable against either 5.16 or "5.18" (this), although I haven't actually tried compiling it yet, much less using it!

> > Nitpicking: you iterate on hash table values, but the key is already the
> > unique_name, no? so you could just take the key instead of calling
> > mcd_account_get_unique_name().

Done, http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=4690efe7ecc874b1149f79118b4e9aa735d71b4e

> (In reply to comment #47)
> > a231756bd832064d531195656df1110c63575717, commit msg says: "We should never
> > have get_something(), commit() with a non-NULL account"
> > s/non-// or s/never/always/ if I understand correctly, right?

Rewrote the commit message to be clearer, I hope it's OK now?

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=a240b95d030be02c9df1be3d7eb678e966ea60ac

Added a commit to that MC branch, http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=c9c370d972e8ee127e1a0bfd7e94a3f2244e4efa

I also have a proof-of-concept branch for Empathy's GOA backend, which I think demonstrates that for something that simple, it's entirely feasible to support both 5.16 and 5.18:

ssh://people.freedesktop.org/~smcv/empathy.git 38-mc-517
http://cgit.freedesktop.org/~smcv/empathy/log?h=38-mc-517 (when it updates)

so I don't think merging this needs to be blocked by implementations in Empathy or telepathy-ring.

For the more complex case of the UOA backend (also in Empathy), Xavier mentioned that he had an implementation.

I think this is ready for review again:

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=d7b6b895f65

I'd also like to add more mandatory API to account storage plugins ("list keys", basically) but I'll do that on another bug.

(In reply to comment #52)
> I think this is ready for review again:
> 
> http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/
> ?h=d7b6b895f65

The unreviewed commits are those since Xavier's last commit.

Re-re-rebased this... tests pass after each commit now (assuming "fixup!" commits are squashed).

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=account-rework-27727-round4

Commits that have not been reviewed are:

* http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=a3521e21d9534bb169c4d5361537656af3b06376 fixes a compiler warning and is to be squashed into the previous

* http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=5633f3ae27a762444bb7a487abfd1f4b2aeacbaf fixes a compiler warning and is to be squashed into the previous

* http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=026cbb87190680f53a9a3382ed65f6e31cd9a4e5 fixes a compiler error and is to be squashed into the previous

* http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=8e5e39aa29a833be16598b2392539af07808a85d needed to be amended while rebasing

* so did http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=e7ba5a86df4e90ec334a4c8ff9a11f9305458e00

* commits since Xavier's "Be consistent in the constness of arguments" are new

The first two of those were fixed in one of Xavier's extra commits, but I moved them earlier in the branch.

It would be good if Xavier could look at <http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=eb7dc4a25c602de15f87fc63f56610ba5a7a05f8> and opine whether that rewritten commit message is enough to satisfy Comment #47. (No logic change, as described in Comment #49.)

My next thing on this topic is hopefully going to be Bug #71093.

It looks fine to me, nice work.

I'm not a big fan of the FIXME in http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/?h=account-rework-27727-round4&id=8e5e39aa29a833be16598b2392539af07808a85d, if we don't find the reason now we'll never do. otoh, it doesn't hurt to commit when in doubt. I'm not considering this as merge blocker.

Ugh, accidentally pushed this without squashing the fixup commits. Never mind, too late now...

Fixed in git for 5.17.0.

(In reply to comment #55)
> I'm not a big fan of the FIXME in
> http://cgit.freedesktop.org/~smcv/telepathy-mission-control/commit/
> ?h=account-rework-27727-round4&id=8e5e39aa29a833be16598b2392539af07808a85d,
> if we don't find the reason now we'll never do.

I'll ask Vivek whether he can remember.