summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Playfair Cal <daniel.playfair.cal@gmail.com>2020-12-19 02:27:07 +0000
committerDaniel Playfair Cal <daniel.playfair.cal@gmail.com>2020-12-19 02:27:07 +0000
commit6e19dec8d57174f3928171e102c24102f2b61ead (patch)
treee74764e806eba70ab76d3fc69927758413e2b736
parent6f7f010b69326eac4e8ee6e0ea7275aaf3232c80 (diff)
parent5ed0724cd763cf79dcc8f6802a6aa093d635cac7 (diff)
downloaddconf-6e19dec8d57174f3928171e102c24102f2b61ead.tar.gz
Merge branch 'patch/engine-check' into 'master'
Engine: Do not emit optimistic change notifications unless the local value is different See merge request GNOME/dconf!2
-rw-r--r--client/meson.build5
-rw-r--r--client/symbol.map14
-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.build5
-rw-r--r--engine/dconf-engine.c85
-rw-r--r--service/dconf-writer.c2
-rw-r--r--service/meson.build7
-rw-r--r--tests/engine.c162
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);