From 221a995afaf999eedbc23b86cc8684fcfca75e2e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 4 Sep 2012 15:43:33 +0100 Subject: [PATCH 4/4] McdAccount: migrate avatars to a XDG path This is the easy part of XDGification: there is no plugin involvement or anything for avatars (yet!), it's just hard-coded. Changing the avatar location from a single directory to a search path results in some new corner cases, so test them. Signed-off-by: Simon McVittie --- src/mcd-account.c | 328 ++++++++++++++++++++--- tests/twisted/Makefile.am | 2 +- tests/twisted/account-manager/avatar-persist.py | 48 +++- tests/twisted/account-manager/avatar-refresh.py | 12 +- tests/twisted/account-manager/avatar.py | 23 +- 5 files changed, 364 insertions(+), 49 deletions(-) diff --git a/src/mcd-account.c b/src/mcd-account.c index c07ba67..9d8adeb 100644 --- a/src/mcd-account.c +++ b/src/mcd-account.c @@ -24,6 +24,7 @@ #include "config.h" #include "mcd-account.h" +#include #include #include @@ -51,7 +52,7 @@ #include "_gen/cli-Connection_Manager_Interface_Account_Storage-body.h" #define MAX_KEY_LENGTH (DBUS_MAXIMUM_NAME_LENGTH + 6) -#define MC_AVATAR_FILENAME "avatar.bin" +#define MC_OLD_AVATAR_FILENAME "avatar.bin" #define MCD_ACCOUNT_PRIV(account) (MCD_ACCOUNT (account)->priv) @@ -661,7 +662,7 @@ load_manager (McdAccount *account) /* Returns the data dir for the given account name. * Returned string must be freed by caller. */ static gchar * -get_account_data_path (McdAccountPrivate *priv) +get_old_account_data_path (McdAccountPrivate *priv) { const gchar *base; @@ -747,7 +748,7 @@ mcd_account_delete (McdAccount *account, mcd_storage_delete_account (priv->storage, name); - data_dir_str = get_account_data_path (priv); + data_dir_str = get_old_account_data_path (priv); if (data_dir_str != NULL) { @@ -2792,6 +2793,213 @@ register_dbus_service (McdAccount *self, (GObject *) self); } +/* + * @account: (allow-none): + * @dir_out: (out): e.g. ~/.local/share/telepathy/mission-control + * @basename_out: (out): e.g. gabble-jabber-fred_40example_2ecom.avatar + * @file_out: (out): @dir_out + "/" + @basename_out + */ +static void +get_avatar_paths (McdAccount *account, + gchar **dir_out, + gchar **basename_out, + gchar **file_out) +{ + gchar *dir = NULL; + + dir = g_build_filename (g_get_user_data_dir (), + "telepathy", "mission-control", NULL); + + if (account == NULL) + { + if (file_out != NULL) + *file_out = NULL; + + if (basename_out != NULL) + *basename_out = NULL; + } + else if (basename_out != NULL || file_out != NULL) + { + gchar *basename = NULL; + + basename = g_strdup_printf ("%s.avatar", account->priv->unique_name); + g_strdelimit (basename, "/", '-'); + + if (file_out != NULL) + *file_out = g_build_filename (dir, basename, NULL); + + if (basename_out != NULL) + *basename_out = basename; + else + g_free (basename); + } + + if (dir_out != NULL) + *dir_out = dir; + else + g_free (dir); +} + +static gboolean +save_avatar (McdAccount *self, + gpointer data, + gssize len, + GError **error) +{ + gchar *dir = NULL; + gchar *file = NULL; + gboolean ret = FALSE; + + get_avatar_paths (self, &dir, NULL, &file); + + if (mcd_ensure_directory (dir, error) && + g_file_set_contents (file, data, len, error)) + { + DEBUG ("Saved avatar to %s", file); + ret = TRUE; + } + else if (len == 0) + { + GArray *avatar = NULL; + + /* It failed, but maybe that's OK, since we didn't really want + * an avatar anyway. */ + _mcd_account_get_avatar (self, &avatar, NULL); + + if (avatar == NULL) + { + /* Creating the empty file failed, but it's fine, since what's + * on disk correctly indicates that we have no avatar. */ + DEBUG ("Ignoring failure to write empty avatar"); + + if (error != NULL) + g_clear_error (error); + } + else + { + /* Continue to raise the error: we failed to write a 0-byte + * file into the highest-priority avatar directory, and we do + * need it, since there is a non-empty avatar in either that + * directory or a lower-priority directory */ + g_array_free (avatar, TRUE); + } + } + + g_free (dir); + g_free (file); + return ret; +} + +static gchar *_mcd_account_get_old_avatar_filename (McdAccount *account, + gchar **old_dir); + +static void +mcd_account_migrate_avatar (McdAccount *account) +{ + GError *error = NULL; + gchar *old_file; + gchar *old_dir = NULL; + gchar *new_dir = NULL; + gchar *basename = NULL; + gchar *new_file = NULL; + gchar *contents = NULL; + guint i; + + /* Try to migrate the avatar to a better location */ + old_file = _mcd_account_get_old_avatar_filename (account, &old_dir); + + if (!g_file_test (old_file, G_FILE_TEST_EXISTS)) + { + /* nothing to do */ + goto finally; + } + + DEBUG ("Migrating avatar from %s", old_file); + + get_avatar_paths (account, &new_dir, &basename, &new_file); + + if (g_file_test (new_file, G_FILE_TEST_IS_REGULAR)) + { + DEBUG ("... already migrated to %s", new_file); + + if (g_unlink (old_file) != 0) + { + DEBUG ("Failed to unlink %s: %s", old_file, + g_strerror (errno)); + } + + goto finally; + } + + if (!mcd_ensure_directory (new_dir, &error)) + { + DEBUG ("%s", error->message); + goto finally; + } + + if (g_rename (old_file, new_file) == 0) + { + DEBUG ("Renamed %s to %s", old_file, new_file); + } + else + { + gsize len; + + DEBUG ("Unable to rename %s to %s, will try copy+delete: %s", + old_file, new_file, g_strerror (errno)); + + if (!g_file_get_contents (old_file, &contents, &len, &error)) + { + DEBUG ("Unable to load old avatar %s: %s", old_file, + error->message); + goto finally; + } + + if (g_file_set_contents (new_file, contents, len, &error)) + { + DEBUG ("Copied old avatar from %s to %s", old_file, new_file); + } + else + { + DEBUG ("Unable to save new avatar %s: %s", new_file, + error->message); + goto finally; + } + + if (g_unlink (old_file) != 0) + { + DEBUG ("Failed to unlink %s: %s", old_file, g_strerror (errno)); + goto finally; + } + } + + /* old_dir is typically ~/.mission-control/accounts/gabble/jabber/badger0. + * We want to delete badger0, jabber, gabble, accounts if they are empty. + * If they are not, we'll just get ENOTEMPTY and stop. */ + for (i = 0; i < 4; i++) + { + gchar *tmp; + + if (g_rmdir (old_dir) != 0) + { + DEBUG ("Failed to rmdir %s: %s", old_dir, g_strerror (errno)); + goto finally; + } + + tmp = g_path_get_dirname (old_dir); + g_free (old_dir); + old_dir = tmp; + } + +finally: + g_clear_error (&error); + g_free (basename); + g_free (new_file); + g_free (new_dir); + g_free (contents); + g_free (old_file); +} + static gboolean mcd_account_setup (McdAccount *account) { @@ -3081,6 +3289,7 @@ _mcd_account_constructed (GObject *object) DEBUG ("%p (%s)", object, account->priv->unique_name); + mcd_account_migrate_avatar (account); mcd_account_setup (account); } @@ -3614,32 +3823,27 @@ _mcd_account_set_avatar (McdAccount *account, const GArray *avatar, { McdAccountPrivate *priv = MCD_ACCOUNT_PRIV (account); const gchar *account_name = mcd_account_get_unique_name (account); - gchar *data_dir, *filename; DEBUG ("called"); - data_dir = get_account_data_path (priv); - filename = g_build_filename (data_dir, MC_AVATAR_FILENAME, NULL); - if (!g_file_test (data_dir, G_FILE_TEST_EXISTS)) - g_mkdir_with_parents (data_dir, 0700); - _mcd_chmod_private (data_dir); - g_free (data_dir); if (G_LIKELY(avatar) && avatar->len > 0) { - if (!g_file_set_contents (filename, avatar->data, - (gssize)avatar->len, error)) + if (!save_avatar (account, avatar->data, avatar->len, error)) { - g_warning ("%s: writing to file %s failed", G_STRLOC, - filename); - g_free (filename); + g_warning ("%s: writing avatar failed", G_STRLOC); return FALSE; } } else { - g_remove (filename); + /* We implement "deleting" an avatar by writing out a zero-length + * file, so that it will override lower-priority directories. */ + if (!save_avatar (account, "", 0, error)) + { + g_warning ("%s: writing empty avatar failed", G_STRLOC); + return FALSE; + } } - g_free (filename); if (mime_type != NULL) mcd_storage_set_string (priv->storage, @@ -3682,7 +3886,37 @@ _mcd_account_set_avatar (McdAccount *account, const GArray *avatar, return TRUE; } -static gchar *_mcd_account_get_avatar_filename (McdAccount *account); +static GArray * +load_avatar_or_warn (const gchar *filename) +{ + GError *error = NULL; + gchar *data = NULL; + gsize length; + + if (g_file_get_contents (filename, &data, &length, &error)) + { + if (length > 0 && length < G_MAXUINT) + { + GArray *ret; + + ret = g_array_new (FALSE, FALSE, 1); + g_array_append_vals (ret, data, (guint) length); + return ret; + } + else + { + DEBUG ("avatar %s was empty or ridiculously large (%" + G_GSIZE_FORMAT " bytes)", filename, length); + return NULL; + } + } + else + { + DEBUG ("error reading %s: %s", filename, error->message); + g_error_free (error); + return NULL; + } +} void _mcd_account_get_avatar (McdAccount *account, GArray **avatar, @@ -3690,6 +3924,7 @@ _mcd_account_get_avatar (McdAccount *account, GArray **avatar, { McdAccountPrivate *priv = MCD_ACCOUNT_PRIV (account); const gchar *account_name = mcd_account_get_unique_name (account); + gchar *basename; gchar *filename; if (mime_type != NULL) @@ -3701,29 +3936,37 @@ _mcd_account_get_avatar (McdAccount *account, GArray **avatar, *avatar = NULL; - filename = _mcd_account_get_avatar_filename (account); + get_avatar_paths (account, NULL, &basename, &filename); - if (filename && g_file_test (filename, G_FILE_TEST_EXISTS)) + if (g_file_test (filename, G_FILE_TEST_EXISTS)) { - GError *error = NULL; - gchar *data = NULL; - gsize length; - if (g_file_get_contents (filename, &data, &length, &error)) - { - if (length > 0 && length < G_MAXUINT) - { - *avatar = g_array_new (FALSE, FALSE, 1); - (*avatar)->data = data; - (*avatar)->len = (guint)length; - } - } - else - { - DEBUG ("error reading %s: %s", filename, error->message); - g_error_free (error); - } + *avatar = load_avatar_or_warn (filename); } + else + { + const gchar * const *iter; + + for (iter = g_get_system_data_dirs (); + iter != NULL && *iter != NULL; + iter++) + { + gchar *candidate = g_build_filename (*iter, "telepathy", + "mission-control", + basename, NULL); + + if (g_file_test (candidate, G_FILE_TEST_EXISTS)) + { + *avatar = load_avatar_or_warn (candidate); + g_free (candidate); + break; + } + + g_free (candidate); + } + } + g_free (filename); + g_free (basename); } GPtrArray * @@ -4184,15 +4427,14 @@ _mcd_account_get_keyfile (McdAccount *account) } static gchar * -_mcd_account_get_avatar_filename (McdAccount *account) +_mcd_account_get_old_avatar_filename (McdAccount *account, + gchar **old_dir) { McdAccountPrivate *priv = account->priv; - gchar *data_dir, *filename; + gchar *filename; - data_dir = get_account_data_path (priv); - DEBUG("data dir: %s", data_dir); - filename = g_build_filename (data_dir, MC_AVATAR_FILENAME, NULL); - g_free (data_dir); + *old_dir = get_old_account_data_path (priv); + filename = g_build_filename (*old_dir, MC_OLD_AVATAR_FILENAME, NULL); return filename; } diff --git a/tests/twisted/Makefile.am b/tests/twisted/Makefile.am index ca3b28d..ac93146 100644 --- a/tests/twisted/Makefile.am +++ b/tests/twisted/Makefile.am @@ -226,7 +226,7 @@ BASIC_TESTS_ENVIRONMENT = \ XDG_CONFIG_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/config \ XDG_CONFIG_DIRS=@abs_top_srcdir@/tests/twisted \ XDG_DATA_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/localshare \ - XDG_DATA_DIRS=@abs_top_srcdir@/tests/twisted \ + XDG_DATA_DIRS=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/share:@abs_top_srcdir@/tests/twisted \ XDG_CACHE_DIR=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/cache \ XDG_CACHE_HOME=@abs_top_builddir@/tests/twisted/tmp-$(TMPSUFFIX)/cache \ MC_CLIENTS_DIR=@abs_top_srcdir@/tests/twisted/telepathy/clients \ diff --git a/tests/twisted/account-manager/avatar-persist.py b/tests/twisted/account-manager/avatar-persist.py index e907873..6dfae62 100644 --- a/tests/twisted/account-manager/avatar-persist.py +++ b/tests/twisted/account-manager/avatar-persist.py @@ -27,7 +27,7 @@ import dbus import dbus.service from servicetest import EventPattern, tp_name_prefix, tp_path_prefix, \ - call_async + call_async, assertEquals from mctest import exec_test, SimulatedConnection, create_fakecm_account, MC import constants as cs @@ -65,8 +65,12 @@ avatar_token=Deus Ex """ % account_id) accounts_cfg.close() - os.makedirs(accounts_dir + '/' + account_id) - avatar_bin = open(accounts_dir + '/' + account_id + '/avatar.bin', 'w') + datadirs = os.environ['XDG_DATA_DIRS'].split(':') + + os.makedirs(datadirs[0] + '/telepathy/mission-control') + avatar_filename = (datadirs[0] + '/telepathy/mission-control/' + + account_id.replace('/', '-') + '.avatar') + avatar_bin = open(avatar_filename, 'w') avatar_bin.write('Deus Ex') avatar_bin.close() @@ -138,6 +142,44 @@ def test(q, bus, unused): assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', byte_arrays=True) == conn.avatar + # The avatar wasn't deleted from $XDG_DATA_DIRS, but it was overridden. + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/' + + account_id + '/avatar.bin') + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm') + + avatar_filename = account_id + avatar_filename = avatar_filename.replace('/', '-') + '.avatar' + avatar_filename = (os.environ['XDG_DATA_HOME'] + + '/telepathy/mission-control/' + avatar_filename) + assertEquals('MJ12', ''.join(open(avatar_filename, 'r').readlines())) + + datadirs = os.environ['XDG_DATA_DIRS'].split(':') + low_prio_filename = account_id + low_prio_filename = low_prio_filename.replace('/', '-') + '.avatar' + low_prio_filename = (datadirs[0] + + '/telepathy/mission-control/' + low_prio_filename) + assertEquals('Deus Ex', ''.join(open(low_prio_filename, 'r').readlines())) + + # If we set the avatar to be empty, that's written out as a file, + # so it'll override the one in XDG_DATA_DIRS + call_async(q, account_props, 'Set', cs.ACCOUNT_IFACE_AVATAR, 'Avatar', + (dbus.ByteArray(''), '')) + + q.expect_many( + EventPattern('dbus-method-call', + interface=cs.CONN_IFACE_AVATARS, method='ClearAvatar', + args=[]), + EventPattern('dbus-signal', path=account.object_path, + interface=cs.ACCOUNT_IFACE_AVATAR, signal='AvatarChanged'), + EventPattern('dbus-return', method='Set') + ) + + assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', + byte_arrays=True) == ('', '') + + assertEquals('', ''.join(open(avatar_filename, 'r').readlines())) + assertEquals('Deus Ex', ''.join(open(low_prio_filename, 'r').readlines())) + if __name__ == '__main__': preseed() exec_test(test, {}, preload_mc=False) diff --git a/tests/twisted/account-manager/avatar-refresh.py b/tests/twisted/account-manager/avatar-refresh.py index 42dd5d3..f3e535a 100644 --- a/tests/twisted/account-manager/avatar-refresh.py +++ b/tests/twisted/account-manager/avatar-refresh.py @@ -27,7 +27,7 @@ import dbus import dbus.service from servicetest import EventPattern, tp_name_prefix, tp_path_prefix, \ - call_async + call_async, assertEquals from mctest import exec_test, SimulatedConnection, create_fakecm_account, MC import constants as cs @@ -119,6 +119,16 @@ def test(q, bus, unused): ), ) + # The avatar got migrated, too. + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/' + + account_id + '/avatar.bin') + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm') + avatar_filename = account_id + avatar_filename = avatar_filename.replace('/', '-') + '.avatar' + avatar_filename = (os.environ['XDG_DATA_HOME'] + + '/telepathy/mission-control/' + avatar_filename) + assertEquals('Deus Ex', ''.join(open(avatar_filename, 'r').readlines())) + if __name__ == '__main__': preseed() exec_test(test, {}, preload_mc=False) diff --git a/tests/twisted/account-manager/avatar.py b/tests/twisted/account-manager/avatar.py index c616e14..af89c6f 100644 --- a/tests/twisted/account-manager/avatar.py +++ b/tests/twisted/account-manager/avatar.py @@ -16,7 +16,8 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA # 02110-1301 USA -import dbus +import os + import dbus import dbus.service @@ -33,6 +34,13 @@ def test(q, bus, mc): account_iface = dbus.Interface(account, cs.ACCOUNT) account_props = dbus.Interface(account, cs.PROPERTIES_IFACE) + assertEquals(cs.ACCOUNT_PATH_PREFIX, + account.object_path[:len(cs.ACCOUNT_PATH_PREFIX)]) + avatar_filename = account.object_path[len(cs.ACCOUNT_PATH_PREFIX):] + avatar_filename = avatar_filename.replace('/', '-') + '.avatar' + avatar_filename = (os.environ['XDG_DATA_HOME'] + + '/telepathy/mission-control/' + avatar_filename) + call_async(q, account_props, 'Set', cs.ACCOUNT_IFACE_AVATAR, 'Avatar', dbus.Struct((dbus.ByteArray('AAAA'), 'image/jpeg'))) q.expect_many( @@ -46,6 +54,10 @@ def test(q, bus, mc): assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', byte_arrays=True) == ('AAAA', 'image/jpeg') + assertEquals('AAAA', ''.join(open(avatar_filename, 'r').readlines())) + # We aren't storing in the old location + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm') + # OK, let's go online. The avatar is set regardless of the CM conn, e = enable_fakecm_account(q, bus, mc, account, params, has_avatars=True, avatars_persist=True, @@ -72,6 +84,9 @@ def test(q, bus, mc): assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', byte_arrays=True) == ('BBBB', 'image/png') + assertEquals('BBBB', ''.join(open(avatar_filename, 'r').readlines())) + assert not os.path.exists(os.environ['MC_ACCOUNT_DIR'] + '/fakecm') + someone_else = conn.ensure_handle(cs.HT_CONTACT, 'alberto@example.com') # Another contact changes their avatar: ignored @@ -97,6 +112,8 @@ def test(q, bus, mc): assert account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', byte_arrays=True) == ('CCCC', 'image/svg') + assertEquals('CCCC', ''.join(open(avatar_filename, 'r').readlines())) + # empty avatar tests conn.forget_avatar() q.dbus_emit(conn.object_path, cs.CONN_IFACE_AVATARS, 'AvatarUpdated', @@ -108,5 +125,9 @@ def test(q, bus, mc): assertEquals(account_props.Get(cs.ACCOUNT_IFACE_AVATAR, 'Avatar', byte_arrays=False), ([], '')) + # empty avatars are represented by an empty file, not no file, + # to get the right precedence over XDG_DATA_DIRS + assertEquals('', ''.join(open(avatar_filename, 'r').readlines())) + if __name__ == '__main__': exec_test(test, {}) -- 1.7.10.4