[Bug 35896] Use XDG directories

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=35896

--- Comment #16 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-08-27 13:30:24 UTC ---
> account-storage: add a fallback for a missing set_variant vfunc

I don't think this commit is right - it doesn't account for escaping?

I would expect set_variant() to take a GVariant, whereas set() gets the
escaped-for-a-key-file equivalent as per _storage_set_value() in
plugin-account.c (which you could factor out into a GVariant-to-string
function). For instance, if some attribute is a TAB character (a string of
length 1), then set_variant() should see g_variant_new_string ("\t"), whereas
set() should see "\\t" (a string of length 2) because that's what you have to
write in your GKeyFile.

String lists in keyfiles are separated by ";", not "," as in this commit, and
if a list element contains ";" it has to be escaped to "\;". Same for object
path lists.

To go the other way, see _storage_dup_value() and telepathy-glib/util.c. GLib
itself has g_key_file_get_[u]int64 these days, too.

> keyfile storage: store everything as strings, not values
> Is this going to break everything? I hope not.

Yes, this is probably going to break the GOA and UOA plugins I'm afraid. We
should keep the "escape it"-based API for set() and get(), and do it Right™ for
set_variant() and get_variant().

Alternatively, break plugin ABI and bump the SONAME.

> plugin-account: stop storing everything in a keyfile in memory

TODO: not reviewed in detail since it seems to rely on the above change. I
approve of this in principle, but if we're not breaking the plugin ABI, we
should keep the escaping.

> +'AutomaticPresenceType': <2>,

Isn't this meant to be a uint32 in GVariant-land?

>   rval = g_file_set_contents (a->filename, str, -1, NULL);
> + _mcd_chmod_private (a->filename);

If the file wasn't already restricted, its contents can be stolen by a very
fast attacker - g_file_set_contents() respects the umask, unfortunately. Either
chmod the parent directory before writing into it, or have MC force its umask
to 077 on startup so all files it creates are private-by-default.

Actually, now I say that, I don't know why we didn't use the umask in the first
place - it would clearly be correct. Oh yeah, it was because MC used to be some
bizarre daemon/library hybrid, but thankfully that's no longer the case...

> + priv->account_connections_dir = g_build_filename (g_get_user_cache_dir (),
> + "mission-control", "accounts", NULL);

I'd prefer this to end up in g_get_user_cache_dir()/telepathy/mc_connections
(i.e. not a dotfile, and share a directory with tp-glib and the logger).

> +/* man I can't wait until this file can be deleted */
> +static void
> +move_everything_else (void)

I'm not convinced that .mc_connections needs migrating at all, I think deletion
would be sufficient - that's why it's in a cache directory. The only time
migration would be useful is if you upgrade from < this version to >= this
version, and then crash/kill MC (without ending your desktop session) with at
least one account connected.

If you do care enough to migrate it, just move it as a special case, rather
than looping over a speculatively general "everything else".

> default keyring storage test: skip for now

I'd be tempted to delete the default backend and just rely on migration. If
not, we should at least delete the gnome-keyring bits.

> + * This file is part of account_migrator-control

Ahem.

> + /* TODO */
> + g_file_set_contents (filename,
> + "# Accounts have been migrated", -1, NULL);

I would prefer less data loss: when an account is migrated fully, delete its
group from the GKeyFile; if one cannot be migrated (usually because the CM
wasn't installed any more), leave it in in the hope that we can do better
later.


> +mcp_account_storage_set_variant (const McpAccountStorage *storage,
> + const McpAccountManager *am,

Const pointers to GObjects are pretty counterproductive (you can't even ref
them).

> +GVariant *
> +mcp_account_manager_get_variant (const McpAccountManager *mcpa,
> + const gchar *account,
> + const gchar *key)
> +{
...
> + g_return_val_if_fail (iface->set_variant != NULL, NULL);

Should say "get".

>   tmp = escape_path (account);
>   path = g_build_filename (self->directory, tmp, NULL);
> - g_hash_table_insert (self->accounts_to_paths,
> - g_strdup (account), path);
> +
> + a = account_new (self, account, path);
> +
> + g_free (path);
>   g_free (tmp);

If you made account_new() calculate tmp and path, that'd save a malloc/free
cycle. No big deal though.

(In reply to comment #14)
> > +_delete (const McpAccountStorage *storage,
> key is ignored, the entire account is deleted. Is this how it's meant to work?

Fixed later anyway.

> It's (becoming) conventional to represent a set as a hash table (key => key).

Fixed later anyway.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.