Got it. Bunch pf places in the code assumed that it was Ok to steal the 
AM's internal data structures, change them, and put them back without
notifying the AM in any way. 

Branch adds proper (or at least _some_) abstraction instead.

Does this need backporting to 5.4?

> +            buf = g_strdup_printf ("%u", g_value_get_uint (value));
> +            g_key_file_set_string (keyfile, name, key, buf);

> +gboolean
> +mcd_account_manager_set_account_value (McdAccountManager *account_manager,
> +                                       McdAccount *account,
> +                                       const gchar *key,
> +                                       const GValue *value)

It might be useful to replace @account with the string "unique name" (object path tail), or the object path, of the account (whichever is more conventional).

Could this usefully be a method on the McdPluginAccountManager, or even a non-virtual convenience method on the McpAccountManager GInterface? That would break the circular dependency between McdAccountManager and McdAccount.

(At some point McdPluginAccountManager should probably be called McdStoredAccounts or something; that's what it does, really.)

If these methods are needed by plugins they should probably go in McpAccountManager; otherwise, they should be G_GNUC_INTERNAL'd, underscore-prefixed, and moved to mcd-account-manager-priv.h, so that they're not API for libmissioncontrol-server.

> +    /* NOTE: we used to purge the account from all stores here but *
> +     * that's handled in mcd_account_delete() by a call to the new *
> +     * mcd_account_manager_forget_account() helper now.            */

I'm not sure that this comment adds anything to people's understanding of the current code. It'd be better as part of the git commit message, perhaps?

I'd really prefer it if you hadn't re-indented match_account_parameter(), or at least if you'd made that into a separate commit. Keep it now, though.

> Remove direct AM access from account-compat

Can we delete this code yet? :-P

Quick answer to q1: Does not need backporting to 5.4

(In reply to comment #2)
> Does this need backporting to 5.4?

Nope, we're good there. The plugin stuff doesn't happen in 5.4, so 
although the bug can theoretically exist, nothing exposes it.

> It might be useful to replace @account with the string "unique name" (object
> path tail), or the object path, of the account (whichever is more
> conventional).

All the things that end up fiddling with the internal structure (in the 
existing code) start by calling a method on the McdAccount, so it's really 
6 of 1 as to who gets the unique name from the account - if you think it 
needs changing, I can change it but I don't think it makes much difference.

--

> Could this usefully be a method on the McdPluginAccountManager, or even a
> non-virtual convenience method on the McpAccountManager GInterface? That would
> break the circular dependency between McdAccountManager and McdAccount.

At the moment, the account manager is the thing that wrangles the storage
plugins and makes sure they're called in the right order and so forth:
Since the storage plugins don't know about each other, they can't (currently)
handle things where a decision needs to be made about who gets the account
setting, (or the account), it has to be the AM for now.

--

> (At some point McdPluginAccountManager should probably be called
> McdStoredAccounts or something; that's what it does, really.)

Noted for future work.

--

> If these methods are needed by plugins they should probably go in
> McpAccountManager; otherwise, they should be G_GNUC_INTERNAL'd,
> underscore-prefixed, and moved to mcd-account-manager-priv.h, so that they're
> not API for libmissioncontrol-server.

They're not needed for plugins but there do appear to be bits of 
libmcserver's users that also steal the AMs storage and fiddle with 
them, I'll need to patch them up to use the new abstractions (in 
progress), and bump their dependencies.

--

> I'm not sure that this comment adds anything to people's understanding of the
> current code. It'd be better as part of the git commit message, perhaps?

Ok.

> I'd really prefer it if you hadn't re-indented match_account_parameter(),
> or at least if you'd made that into a separate commit. Keep it now, though.

Oops, hadn't noticed that.

> > Remove direct AM access from account-compat
> Can we delete this code yet? :-P

Good question. There are chunks of libmcserver's users which use 
some of this stuff, but some of those chunks can go away now: I'm 
just not sure exactly how much of those use cases is dead code at 
this point. We can probably start pruning it rather than adding to 
it.

(In reply to comment #4)
> > Could this usefully be a method on the McdPluginAccountManager, or even a
> > non-virtual convenience method on the McpAccountManager GInterface? That would
> > break the circular dependency between McdAccountManager and McdAccount.
> 
> At the moment, the account manager is the thing that wrangles the storage
> plugins and makes sure they're called in the right order and so forth:
> Since the storage plugins don't know about each other, they can't (currently)
> handle things where a decision needs to be made about who gets the account
> setting, (or the account), it has to be the AM for now.

The McdPluginAccountManager (which is the implementation of McpAccountManager) isn't a plugin; it's the thing in src/ that plugins see when they think they're talking to the account manager. As such, it could reasonably know about the plugins.

An example of breaking a similar cycle: we used to have:

    McdDispatcher
    ... owns several McdDispatcherContext instances
    ... which each have a pointer back to the McdDispatcher

but by splitting some of the McdDispatcher functionality away into smaller classes, we now have:

    McdDispatcher
    ... owns several McdDispatchOperation instances
    .... which each have a pointer to the McdHandlerMap and McdClientRegistry

The AccountManager could reasonably be split into:

    D-Bus AccountManager (McdAccountManager
    ... owns some Account instances
    ... which each point to McdAccountStorageThing [1]

Historically, the closest equivalent to [1] is the big GKeyFile that everyone shares :-)

Ok, I see what you mean about the manager, I did a similar thing with the dbus ACL plugins... I guess the answer is yes, yes it could: Do you want it made so?

Updated branch. Ready for re-review. (Will be adding some docs shortly)

(In reply to comment #7)
> Updated branch. Ready for re-review. (Will be adding some docs shortly)

The param-register flag is being removed here correctly after a successful connection.

"Implement the new McdStorage interface" duplicates a lot of existing code, and then "Tidy up most of the mcd-account value getting/setting" deletes the original versions. I'd have preferred it if there was one commit per virtual method (or group of closely-related virtual methods) which *moved* the original implementation into the McdStorage implementation; as it stands at the moment, it's rather difficult to review.

> void mcd_storage_store_connections (McdStorage *storage)

Looks like this should be internal (G_GNUC_INTERNAL, _ prefix, -priv.h).

> +  g_object_get (master, "account-manager", &account_manager, NULL);

account_manager is leaked. Coding style: new line before "account-manager" and before NULL.

McdStorage calls g_assert a lot. All checks on its arguments should be g_return[_val]_if_fail instead.

It appears that the set_string and set_value methods return whether the value changed, rather than whether setting succeeded. This is unconventional, so it deserves a (skeletal) doc-comment.

Could you rename list_accounts to dup_accounts, and likewise for list_settings, to indicate that they return a new strv?

get_string should be dup_string, because it g_strdups.

get_value should be dup_value. It deserves a comment saying that it returns a slice-allocated GValue.

The indentation in the default case of _storage_get_value is strange; please fix it (Telepathy style preferred, historical MC style tolerated).

In several places in _storage_get_value, a non-NULL GValue is returned with @error set, which isn't right (admittedly, this is not a regression, since keyfile_get_value had this bug too).

> +void mcd_plugin_account_manager_ready (McdPluginAccountManager *self);
> +
> +void mcd_plugin_account_manager_connect_signal (const gchar *signal,

These should be underscore-prefixed and/or G_GNUC_INTERNAL; ideally, so should everything else in that header, which isn't installed.

> +_storage_set_string (McdStorage *storage,

I'd prefer tp_strdiff instead of a cmp-style function.

You should be passing an escaped form of @val (the result of g_key_file_get_value) to update_storage; please test this with strings containing escape characters.

> +_storage_set_value (McdStorage *storage,

Again, I'd prefer tp_strdiff.

Why are you NIHing g_key_file_set_boolean et al? I can see that the code structure is vaguely attractive, but...

The code to set GStrv values will fall apart if the strings contain escaped semicolons: by calling g_key_file_set_string_list followed by g_key_file_get_string, you're losing the distinction between ; and \; (or possibly between \; and \\;, just pile on extra escapes until you find a failing case). Instead, you should be passing the raw value (result of g_key_file_get_value) to update_storage.

> +  /* special case, might be synthesised by the backend: */
> +  if (value == NULL && g_strcmp0 ("sso-services", key));

I'd prefer get_sso_services to be a separate method, if it needs this special behaviour.

> +    g_debug ("FETCHED AUTO PRESENCE: %u %s %s", presence_type, presence, message);

Delete?

> +        g_warning("\nsetting "MC_ACCOUNTS_KEY_AUTO_PRESENCE_MESSAGE" to %s", new_message);

Delete!

> -        mcd_account_changed_property (account, "Enabled", &value);
> +        mcd_account_changed_property (account, MC_ACCOUNTS_KEY_ENABLED, &value);

I'd prefer to reserve MC_ACCOUNTS_KEY_ENABLED for things written into the account's (pseudo-)keyfile, and stick to "Enabled" when targeting D-Bus (like here), even though MC_ACCOUNTS_KEY_ENABLED happens to also be "Enabled".

> +mcd_account_get_storage (McdAccount *account)

Can this be internal within src (underscore prefix, G_GNUC_INTERNAL, -priv header)?

Non-blockers
============

McdStorage could use G_DEFINE_INTERFACE, with a GLib 2.24 dependency.

The function currently called get_value, which I suggested you rename to dup_value, might make more sense if it returned void and either took a zero-filled GValue as argument, or took an initialized GValue of type @type as argument. This would make mcd_account_get_string_val() trivial.

> +        if (message && message[0] != 0)

tp_str_empty()?

> +        /* don't keep a reference to the storage: we can safely assume
>           * its lifetime is longer than the McdAccount's */

Now that the reference isn't circular, I'd prefer this to be a real ref, released in dispose.

> +      case PROP_DBUS_DAEMON:
> +        /* as above, the dbus daemon is held by the account manager *
> +         * which must live longer than the account:                 */

Likewise.

> Remove direct access to AM internals from account-manager-query

One day, please close Bug #24914 by deleting this entire interface. As I understand it, telepathy-qt4 supports filtering accounts at the client side, which is the way forward really.

> Remove direct access to account manager from mcd-account-compat,c

This interface is a dumping ground for misc from Maemo 5, which we're now designing replacements for. telepathy-qt4 doesn't offer access to any of this interface's functionality. Bug #24899 asks for part of it to be deleted, but to be honest all of it should be deleted.

(Stable-branch blocker, IMO.)

Blockers should be dealt with.

wrt: Why are you NIHing g_key_file_set_boolean et al? I can see 
that the code structure is vaguely attractive, but...

Largely so there's an identical path through the code for all types that 
lets us compare the old value with the new value so we can signal back to 
the caller whether anything actually changed (at which point we have the 
raw string we want to stuff into the store anyway, so we might as well use
it)

Sliced and diced the history into smaller chunks, hopefully
this makes it much easier to review.

Trivial
=======

> +/**
> + * mcd_storage_get_plugin:
> ...
> +void
> +mcd_storage_delete_account (McdStorage *storage, const gchar *account)

Mismatched.

Serious
=======

> +static GValue *
> +_storage_dup_value (McdStorage *storage,
...
> +  if (value != NULL)
> +    g_clear_error (error);

This results in any non-integer, or missing integer, silently being interpreted as 0: in particular, the integer accessors can only distinguish between 0-but-no-error and error by having a GError argument initially pointing to NULL, and testing whether it has been set.

You want to have a temporary GError * variable (keyfile_error or something), use that as the argument to g_key_file_get_thing(), discard the GValue if keyfile_error is not NULL, and propagate keyfile_error into error.

You still mishandle string escapes here: the object path and string cases both use g_key_file_get_value when they should use g_key_file_get_string. (The difference is that if the file contains "a\tb", get_value returns 'a backslash tb', whereas get_string returns 'a TAB b').

The double and boolean cases have no error handling, not even for "not present".

The object path case can pile up GErrors (if g_key_file_get_value fails, then its return is not a valid object path, and g_set_error is called again on a non-empty GError). That's an assertion failure. It's also not valid to call tp_dbus_check_valid_object_path on NULL. You narrowly avoided this failure mode for integer range-checking, because 0 happens to be in range.

The g_warning case returns NULL without setting @error.

> +static gboolean
> +_storage_set_value (McdStorage *storage,

When writing a string into the file, you use g_key_file_set_value. You should use g_key_file_set_string, which escapes special characters (test with "a\tb" for instance).

Last time we mixed up the value and string versions, there were major regressions involving people's disks filling up with backslashes. Let's not go there again immediately before a stable branch.

> wrt: Why are you NIHing g_key_file_set_boolean et al?
> Largely so there's an identical path through the code for all types that 
> lets us compare the old value with the new value so we can signal back to 
> the caller whether anything actually changed (at which point we have the 
> raw string we want to stuff into the store anyway, so we might as well use
> it)

I think better logic for this would be:

* dup the old raw value
* set the new value using a more appropriate API:
  in other words, the code from keyfile_[gs]et_value, which we have
  already debugged!
* dup the new raw value
* if they differ, signal that it changed and write it into backends

> -    AccountDeleteData *delete_data;

It looks as though this struct is no longer used? If so, please delete.

> The McdStorage layer uses get/set _value, the actual plugins need to use _string

No, I think this commit should be reverted. The correct escaping to use depends on the data type (string lists and strings are not the same escaping - string lists additionally escape ";" within items as "\;"), and interpreting a string list as a string or vice versa causes non-reversible information loss.

One day, we should probably change the on-disc format to not be GKeyFile. When we're trying to make a stable branch yesterday is not that day.

>  match_account_parameter (McdAccount *account, const gchar *name,

You should check that value and conf are of compatible types. If you use g_value_get_uint (conf) when conf contains anything except a uint (even an int!), that's considered an error.

Not blockers, at this stage
===========================

Can any more of the McdStorage API become internal? I don't see why an Mcd embedder should ever be calling mcd_storage_load()?

> @@ -1683,68 +1620,33 @@ mcd_account_manager_write_conf_async (McdAccountManager *account_manager,
...
> +        groups = mcd_storage_dup_accounts (storage, &n_accounts);

groups doesn't seem to be used in this method any more: you call this method just to get n_accounts, emit a debug message with n_accounts in, then free groups. I'd drop it and just say "all accounts".

Notes for the future
====================

"Move account load + initialisation into plugin-account" (also, at least, the one for deletion) adds a static function but doesn't add it to the vtable. It somewhat defeats the object of having small, digestible commits if the tests wouldn't have passed after each one: reviewers still have to hold extra state in their heads ("these changes will be needed to make it valid again") and bisecting won't work.

Updated branch,

Looks good to merge. I'll handle the release, I want to get at least some of ptlo's account storage regression testing in first.

Fixed in git for 5.5.4