diff options
-rw-r--r-- | client/meson.build | 5 | ||||
-rw-r--r-- | client/symbol.map | 14 | ||||
-rw-r--r-- | common/dconf-gvdb-utils.c (renamed from service/dconf-gvdb-utils.c) | 76 | ||||
-rw-r--r-- | common/dconf-gvdb-utils.h (renamed from service/dconf-gvdb-utils.h) | 5 | ||||
-rw-r--r-- | common/meson.build | 5 | ||||
-rw-r--r-- | engine/dconf-engine.c | 85 | ||||
-rw-r--r-- | service/dconf-writer.c | 2 | ||||
-rw-r--r-- | service/meson.build | 7 | ||||
-rw-r--r-- | tests/engine.c | 162 |
9 files changed, 318 insertions, 43 deletions
diff --git a/client/meson.build b/client/meson.build index e9672b8..0308dc2 100644 --- a/client/meson.build +++ b/client/meson.build @@ -32,6 +32,9 @@ client_deps = [ libdconf_gdbus_thread_dep, ] +symbol_map = join_paths(meson.current_source_dir(), 'symbol.map') +ldflags = cc.get_supported_link_arguments('-Wl,--version-script,@0@'.format(symbol_map)) + libdconf = shared_library( 'dconf', sources: sources, @@ -40,6 +43,8 @@ libdconf = shared_library( include_directories: top_inc, dependencies: client_deps, c_args: dconf_c_args, + link_args: ldflags, + link_depends: symbol_map, install: true, ) diff --git a/client/symbol.map b/client/symbol.map new file mode 100644 index 0000000..2fc9983 --- /dev/null +++ b/client/symbol.map @@ -0,0 +1,14 @@ +{ +global: + dconf_client_*; + dconf_changeset_*; + dconf_error_quark; + dconf_is_path; + dconf_is_key; + dconf_is_dir; + dconf_is_rel_path; + dconf_is_rel_key; + dconf_is_rel_dir; +local: + *; +}; diff --git a/service/dconf-gvdb-utils.c b/common/dconf-gvdb-utils.c index d77ae97..0aced4c 100644 --- a/service/dconf-gvdb-utils.c +++ b/common/dconf-gvdb-utils.c @@ -22,7 +22,7 @@ #include "dconf-gvdb-utils.h" -#include "../common/dconf-paths.h" +#include "./dconf-paths.h" #include "../gvdb/gvdb-builder.h" #include "../gvdb/gvdb-reader.h" @@ -32,6 +32,37 @@ #include <string.h> DConfChangeset * +dconf_gvdb_utils_changeset_from_table (GvdbTable *table) +{ + DConfChangeset *database = dconf_changeset_new_database (NULL); + gchar **names; + gsize n_names; + gsize i; + + names = gvdb_table_get_names (table, &n_names); + for (i = 0; i < n_names; i++) + { + if (dconf_is_key (names[i], NULL)) + { + GVariant *value; + + value = gvdb_table_get_value (table, names[i]); + + if (value != NULL) + { + dconf_changeset_set (database, names[i], value); + g_variant_unref (value); + } + } + + g_free (names[i]); + } + + g_free (names); + return database; +} + +DConfChangeset * dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, gboolean *file_missing, GError **error) @@ -95,38 +126,14 @@ dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, return NULL; } - /* Only allocate once we know we are in a non-error situation */ - database = dconf_changeset_new_database (NULL); - /* Fill the table up with the initial state */ if (table != NULL) { - gchar **names; - gsize n_names; - gsize i; - - names = gvdb_table_get_names (table, &n_names); - for (i = 0; i < n_names; i++) - { - if (dconf_is_key (names[i], NULL)) - { - GVariant *value; - - value = gvdb_table_get_value (table, names[i]); - - if (value != NULL) - { - dconf_changeset_set (database, names[i], value); - g_variant_unref (value); - } - } - - g_free (names[i]); - } - + database = dconf_gvdb_utils_changeset_from_table (table); gvdb_table_free (table); - g_free (names); } + else + database = dconf_changeset_new_database (NULL); if (file_missing) *file_missing = (table == NULL); @@ -186,6 +193,16 @@ dconf_gvdb_utils_add_key (const gchar *path, return TRUE; } +GHashTable * +dconf_gvdb_utils_table_from_changeset (DConfChangeset *database) +{ + GHashTable *table; + + table = gvdb_hash_table_new (NULL, NULL); + dconf_changeset_all (database, dconf_gvdb_utils_add_key, table); + return table; +} + gboolean dconf_gvdb_utils_write_file (const gchar *filename, DConfChangeset *database, @@ -194,8 +211,7 @@ dconf_gvdb_utils_write_file (const gchar *filename, GHashTable *gvdb; gboolean success; - gvdb = gvdb_hash_table_new (NULL, NULL); - dconf_changeset_all (database, dconf_gvdb_utils_add_key, gvdb); + gvdb = dconf_gvdb_utils_table_from_changeset (database); success = gvdb_table_write_contents (gvdb, filename, FALSE, error); if (!success) diff --git a/service/dconf-gvdb-utils.h b/common/dconf-gvdb-utils.h index 7076781..799c66c 100644 --- a/service/dconf-gvdb-utils.h +++ b/common/dconf-gvdb-utils.h @@ -21,11 +21,14 @@ #ifndef __dconf_gvdb_utils_h__ #define __dconf_gvdb_utils_h__ -#include "../common/dconf-changeset.h" +#include "../gvdb/gvdb-reader.h" +#include "./dconf-changeset.h" +DConfChangeset * dconf_gvdb_utils_changeset_from_table (GvdbTable *table); DConfChangeset * dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, gboolean *file_missing, GError **error); +GHashTable * dconf_gvdb_utils_table_from_changeset (DConfChangeset *database); gboolean dconf_gvdb_utils_write_file (const gchar *filename, DConfChangeset *database, GError **error); diff --git a/common/meson.build b/common/meson.build index befa9bc..e736ea8 100644 --- a/common/meson.build +++ b/common/meson.build @@ -15,18 +15,19 @@ sources = files( 'dconf-changeset.c', 'dconf-error.c', 'dconf-paths.c', + 'dconf-gvdb-utils.c', ) libdconf_common = static_library( 'dconf-common', sources: sources, include_directories: top_inc, - dependencies: glib_dep, + dependencies: [glib_dep, libgvdb_dep], c_args: dconf_c_args, pic: true, ) libdconf_common_dep = declare_dependency( - dependencies: glib_dep, + dependencies: [glib_dep, libgvdb_dep], link_with: libdconf_common, ) diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c index 18b8aa5..6a1f247 100644 --- a/engine/dconf-engine.c +++ b/engine/dconf-engine.c @@ -24,6 +24,7 @@ #include "../common/dconf-enums.h" #include "../common/dconf-paths.h" +#include "../common/dconf-gvdb-utils.h" #include "../gvdb/gvdb-reader.h" #include <string.h> #include <stdlib.h> @@ -822,6 +823,54 @@ dconf_engine_list (DConfEngine *engine, return list; } +static gboolean +dconf_engine_dir_has_writable_contents (DConfEngine *engine, + const gchar *dir) +{ + DConfChangeset *database; + GHashTable *current_state; + + /* Read the on disk state */ + if (engine->n_sources == 0 || !engine->sources[0]->writable) + // If there are no writable sources, there won't be any pending writes either + return FALSE; + + dconf_engine_acquire_sources (engine); + database = dconf_gvdb_utils_changeset_from_table (engine->sources[0]->values); + dconf_engine_release_sources (engine); + + /* Apply pending and in_flight changes to the on disk state */ + dconf_engine_lock_queue (engine); + + if (engine->in_flight != NULL) + dconf_changeset_change (database, engine->in_flight); + + if (engine->pending != NULL) + { + /** + * We don't want to seal the pending changeset because it may still be changed, + * and sealing the changeset would be a side effect of passing engine->pending + * directly into dconf_changeset_change. + */ + DConfChangeset *changes = dconf_changeset_filter_changes (database, engine->pending); + if (changes != NULL) + { + dconf_changeset_change (database, changes); + dconf_changeset_unref (changes); + } + } + + dconf_engine_unlock_queue (engine); + + /* Check if there are writable contents at the given directory in the current state */ + current_state = dconf_gvdb_utils_table_from_changeset (database); + gboolean result = g_hash_table_contains (current_state, dir); + + g_hash_table_unref (current_state); + dconf_changeset_unref (database); + return result; +} + typedef void (* DConfEngineCallHandleCallback) (DConfEngine *engine, gpointer handle, GVariant *parameter, @@ -1129,6 +1178,34 @@ dconf_engine_prepare_change (DConfEngine *engine, */ static void dconf_engine_manage_queue (DConfEngine *engine); +/** + * a #DConfChangesetPredicate which determines whether the given path and + * value is already present in the given engine. "Already present" means + * that setting that path to that value would have no effect on the + * engine, including for directory resets. + */ +static gboolean +dconf_engine_path_has_value_predicate (const gchar *path, + GVariant *new_value, + gpointer user_data) +{ + DConfEngine *engine = user_data; + + // Path reset are handled specially + if (g_str_has_suffix (path, "/")) + return !dconf_engine_dir_has_writable_contents (engine, path); + + g_autoptr(GVariant) current_value = dconf_engine_read ( + engine, + DCONF_READ_USER_VALUE, + NULL, + path + ); + return ((current_value == NULL && new_value == NULL) || + (current_value != NULL && new_value != NULL && + g_variant_equal (current_value, new_value))); +} + static void dconf_engine_emit_changes (DConfEngine *engine, DConfChangeset *changeset, @@ -1208,6 +1285,7 @@ dconf_engine_manage_queue (DConfEngine *engine) G_VARIANT_TYPE ("(s)"), sizeof (OutstandingChange)); oc->change = engine->in_flight = g_steal_pointer (&engine->pending); + dconf_changeset_seal (engine->in_flight); parameters = dconf_engine_prepare_change (engine, oc->change); @@ -1273,6 +1351,10 @@ dconf_engine_change_fast (DConfEngine *engine, if (dconf_changeset_is_empty (changeset)) return TRUE; + gboolean has_no_effect = dconf_changeset_all (changeset, + dconf_engine_path_has_value_predicate, + engine); + if (!dconf_engine_changeset_changes_only_writable_keys (engine, changeset, error)) return FALSE; @@ -1297,7 +1379,8 @@ dconf_engine_change_fast (DConfEngine *engine, dconf_engine_unlock_queue (engine); /* Emit the signal after dropping the lock to avoid deadlock on re-entry. */ - dconf_engine_emit_changes (engine, changeset, origin_tag); + if (!has_no_effect) + dconf_engine_emit_changes (engine, changeset, origin_tag); return TRUE; } diff --git a/service/dconf-writer.c b/service/dconf-writer.c index 4d054c8..438c77b 100644 --- a/service/dconf-writer.c +++ b/service/dconf-writer.c @@ -23,7 +23,7 @@ #include "dconf-writer.h" #include "../shm/dconf-shm.h" -#include "dconf-gvdb-utils.h" +#include "../common/dconf-gvdb-utils.h" #include "dconf-generated.h" #include "dconf-blame.h" diff --git a/service/meson.build b/service/meson.build index 19fe670..51e3090 100644 --- a/service/meson.build +++ b/service/meson.build @@ -12,7 +12,6 @@ configure_file( lib_sources = [ 'dconf-blame.c', - 'dconf-gvdb-utils.c', 'dconf-keyfile-writer.c', 'dconf-service.c', 'dconf-shm-writer.c', @@ -36,7 +35,7 @@ libdconf_service = static_library( sources: lib_sources, include_directories: top_inc, c_args: dconf_c_args, - dependencies: gio_unix_dep, + dependencies: [gio_unix_dep, libdconf_common_dep], link_with: [ libdconf_common, libdconf_shm, @@ -46,7 +45,7 @@ libdconf_service = static_library( libdconf_service_dep = declare_dependency( link_with: libdconf_service, - dependencies: gio_unix_dep, + dependencies: [gio_unix_dep, libdconf_common_dep], sources: dconf_generated, ) @@ -55,7 +54,7 @@ dconf_service = executable( sources, include_directories: top_inc, c_args: dconf_c_args, - dependencies: gio_unix_dep, + dependencies: [gio_unix_dep, libdconf_common_dep], link_with: libdconf_service, install: true, install_dir: dconf_libexecdir, diff --git a/tests/engine.c b/tests/engine.c index fd2a348..b0ad884 100644 --- a/tests/engine.c +++ b/tests/engine.c @@ -1466,7 +1466,7 @@ test_watch_sync (void) static void test_change_fast (void) { - DConfChangeset *empty, *good_write, *bad_write, *very_good_write, *slightly_bad_write; + DConfChangeset *empty, *good_write, *good_write2, *bad_write, *very_good_write, *slightly_bad_write; GvdbTable *table, *locks; DConfEngine *engine; gboolean success; @@ -1483,6 +1483,7 @@ test_change_fast (void) empty = dconf_changeset_new (); good_write = dconf_changeset_new_write ("/value", g_variant_new_string ("value")); + good_write2 = dconf_changeset_new_write ("/value2", g_variant_new_string ("value2")); bad_write = dconf_changeset_new_write ("/locked", g_variant_new_string ("value")); very_good_write = dconf_changeset_new_write ("/value", g_variant_new_string ("value")); dconf_changeset_set (very_good_write, "/to-reset", NULL); @@ -1517,6 +1518,15 @@ test_change_fast (void) dconf_mock_dbus_assert_no_async (); g_assert_cmpstr (change_log->str, ==, ""); + /* Verify that value is unset initially */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value"); + g_assert (value == NULL); + + /* Verify that value2 is unset initially */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2"); + g_assert (value == NULL); + + /* change /value */ success = dconf_engine_change_fast (engine, good_write, NULL, &error); g_assert_no_error (error); g_assert (success); @@ -1530,7 +1540,48 @@ test_change_fast (void) g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value"); g_variant_unref (value); - /* Fail the attempted write. This should cause a warning and a change. */ + /* Repeat the same write for /value (which is already in the in_flight queue) */ + success = dconf_engine_change_fast (engine, good_write, NULL, &error); + g_assert_no_error (error); + g_assert (success); + + /* Verify that /value is (still) set */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value"); + g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value"); + g_variant_unref (value); + + /* That should not have emitted a synthetic change event, since the (local) value did not change */ + g_assert_cmpstr (change_log->str, ==, ""); + g_string_set_size (change_log, 0); + + /* change /value2 */ + success = dconf_engine_change_fast (engine, good_write2, NULL, &error); + g_assert_no_error (error); + g_assert (success); + + /* That should have emitted a synthetic change event */ + g_assert_cmpstr (change_log->str, ==, "/value2:1::nil;"); + g_string_set_size (change_log, 0); + + /* Verify that /value2 is set */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2"); + g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2"); + g_variant_unref (value); + + /* change /value2 a second time */ + success = dconf_engine_change_fast (engine, good_write2, NULL, &error); + g_assert_no_error (error); + g_assert (success); + + /* That should not have emitted a synthetic change event because the (local) value didn't change */ + g_assert_cmpstr (change_log->str, ==, ""); + + /* Verify that /value2 is still set */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2"); + g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2"); + g_variant_unref (value); + + /* Fail the first attempted write. This should cause a warning and a signal. */ error = g_error_new_literal (G_FILE_ERROR, G_FILE_ERROR_NOENT, "something failed"); dconf_mock_dbus_async_reply (NULL, error); g_clear_error (&error); @@ -1539,8 +1590,25 @@ test_change_fast (void) assert_pop_message ("dconf", G_LOG_LEVEL_WARNING, "failed to commit changes to dconf: something failed"); - /* Verify that the value became unset due to the failure */ - value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "value"); + /* Verify that /value is still set (because the second write is in progress) */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value"); + g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value"); + g_variant_unref (value); + + /* Verify that /value2 is still set because the write is in progress */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2"); + g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2"); + g_variant_unref (value); + + /* Now allow the second set of writes to succeed */ + dconf_mock_dbus_async_reply (g_variant_new ("(s)", "tag"), NULL); + + /* Verify that /value became unset due to the in flight queue clearing */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value"); + g_assert (value == NULL); + + /* Verify that /value2 became unset due to the in flight queue clearing */ + value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2"); g_assert (value == NULL); /* Now try a successful write */ @@ -1619,6 +1687,91 @@ test_change_fast (void) change_log = NULL; } +/** + * Tests that dconf_engine_change_fast() emits local optimistic change + * notifications in the right circumstances + */ +static void +test_change_fast_redundant (void) +{ + DConfChangeset *change; + DConfEngine *engine; + change_log = g_string_new (NULL); + + // Initialise an empty engine + engine = dconf_engine_new (SRCDIR "/profile/dos", NULL, NULL); + + // Send an empty changeset, which has no effect + change = dconf_changeset_new (); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, ""); + + // Reset the root directory, which has no effect since the database is empty + change = dconf_changeset_new_write ("/", NULL); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, ""); + + // Set apple to NULL, which has no effect because it was already unset + change = dconf_changeset_new_write ("/apple", NULL); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, ""); + + // Set apple to apple + change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple")); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;"); + g_string_set_size (change_log, 0); + + // Set apple to apple, which has no effect because it is the same as the old value + change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple")); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, ""); + g_string_set_size (change_log, 0); + + // Set apple to orange, which has an effect because it is different to the old value + change = dconf_changeset_new_write ("/apple", g_variant_new_string ("orange")); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;"); + g_string_set_size (change_log, 0); + + // Set apple to NULL, which has an effect because it was previously set + change = dconf_changeset_new_write ("/apple", NULL); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;"); + g_string_set_size (change_log, 0); + + // Set apple to apple + change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple")); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;"); + g_string_set_size (change_log, 0); + + // Reset the root directory, which has an effect since the database is not empty + change = dconf_changeset_new_write ("/", NULL); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, "/:1::nil;"); + g_string_set_size (change_log, 0); + + // Reset the root directory again, which has no effect since the database is empty + change = dconf_changeset_new_write ("/", NULL); + dconf_engine_change_fast (engine, change, NULL, NULL); + dconf_changeset_unref (change); + g_assert_cmpstr (change_log->str, ==, ""); + + dconf_engine_unref (engine); + g_string_free (change_log, TRUE); + change_log = NULL; +} + static GError *change_sync_error; static GVariant *change_sync_result; @@ -2087,6 +2240,7 @@ main (int argc, char **argv) g_test_add_func ("/engine/watch/fast/short_lived", test_watch_fast_short_lived_subscriptions); g_test_add_func ("/engine/watch/sync", test_watch_sync); g_test_add_func ("/engine/change/fast", test_change_fast); + g_test_add_func ("/engine/change/fast_redundant", test_change_fast_redundant); g_test_add_func ("/engine/change/sync", test_change_sync); g_test_add_func ("/engine/signals", test_signals); g_test_add_func ("/engine/sync", test_sync); |