From d23b3fddd62cf5310c7f8869663ddb1c54c192dc Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 4 May 2020 16:07:01 +1000 Subject: client: add symbol map This is similar to what is done in in libdconfsettings to prevent symbols not part of the API from being available in the compiled binaries. In practice, this makes it possible to include more code in the common directory without creating new symbols in libdconf. --- client/meson.build | 5 +++++ client/symbol.map | 14 ++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 client/symbol.map 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: + *; +}; -- cgit v1.2.1 From 658af7018a349eb925503f920a3c6b5a948b0cc1 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 6 Jan 2020 12:29:12 +1100 Subject: move dconf-gvdb-utils from service into common --- common/dconf-gvdb-utils.c | 219 +++++++++++++++++++++++++++++++++++++++++++++ common/dconf-gvdb-utils.h | 33 +++++++ common/meson.build | 5 +- service/dconf-gvdb-utils.c | 219 --------------------------------------------- service/dconf-gvdb-utils.h | 33 ------- service/dconf-writer.c | 2 +- service/meson.build | 7 +- 7 files changed, 259 insertions(+), 259 deletions(-) create mode 100644 common/dconf-gvdb-utils.c create mode 100644 common/dconf-gvdb-utils.h delete mode 100644 service/dconf-gvdb-utils.c delete mode 100644 service/dconf-gvdb-utils.h diff --git a/common/dconf-gvdb-utils.c b/common/dconf-gvdb-utils.c new file mode 100644 index 0000000..e70e2dc --- /dev/null +++ b/common/dconf-gvdb-utils.c @@ -0,0 +1,219 @@ +/* + * Copyright © 2010 Codethink Limited + * Copyright © 2012 Canonical Limited + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the licence, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see . + * + * Author: Ryan Lortie + */ + +#include "config.h" + +#include "dconf-gvdb-utils.h" + +#include "./dconf-paths.h" +#include "../gvdb/gvdb-builder.h" +#include "../gvdb/gvdb-reader.h" + +#include +#include +#include +#include + +DConfChangeset * +dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, + gboolean *file_missing, + GError **error) +{ + DConfChangeset *database; + GError *my_error = NULL; + GvdbTable *table = NULL; + gchar *contents; + gsize size; + + if (g_file_get_contents (filename, &contents, &size, &my_error)) + { + GBytes *bytes; + + bytes = g_bytes_new_take (contents, size); + table = gvdb_table_new_from_bytes (bytes, FALSE, &my_error); + g_bytes_unref (bytes); + } + + /* It is perfectly fine if the file does not exist -- then it's + * just empty. + */ + if (g_error_matches (my_error, G_FILE_ERROR, G_FILE_ERROR_NOENT)) + g_clear_error (&my_error); + + /* Otherwise, we should report errors to prevent ourselves from + * overwriting the database in other situations... + */ + if (g_error_matches (my_error, G_FILE_ERROR, G_FILE_ERROR_INVAL)) + { + /* Move the database to a backup file, warn and continue with a new + * database. The alternative is erroring out and exiting the daemon, + * which leaves the user’s session essentially unusable. + * + * The code to find an unused backup filename is racy, but this is an + * error handling path. Who cares. */ + g_autofree gchar *backup_filename = NULL; + guint i; + + for (i = 0; + i < G_MAXUINT && + (backup_filename == NULL || g_file_test (backup_filename, G_FILE_TEST_EXISTS)); + i++) + { + g_free (backup_filename); + backup_filename = g_strdup_printf ("%s~%u", filename, i); + } + + if (g_rename (filename, backup_filename) != 0) + g_warning ("Error renaming corrupt database from ‘%s’ to ‘%s’: %s", + filename, backup_filename, g_strerror (errno)); + else + g_warning ("Database ‘%s’ was corrupt: moved it to ‘%s’ and created an empty replacement", + filename, backup_filename); + + g_clear_error (&my_error); + } + else if (my_error) + { + g_propagate_prefixed_error (error, my_error, "Cannot open dconf database: "); + 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]); + } + + gvdb_table_free (table); + g_free (names); + } + + if (file_missing) + *file_missing = (table == NULL); + + return database; +} + +static GvdbItem * +dconf_gvdb_utils_get_parent (GHashTable *table, + const gchar *key) +{ + GvdbItem *grandparent, *parent; + gchar *parent_name; + gint len; + + if (g_str_equal (key, "/")) + return NULL; + + len = strlen (key); + if (key[len - 1] == '/') + len--; + + while (key[len - 1] != '/') + len--; + + parent_name = g_strndup (key, len); + parent = g_hash_table_lookup (table, parent_name); + + if (parent == NULL) + { + parent = gvdb_hash_table_insert (table, parent_name); + + grandparent = dconf_gvdb_utils_get_parent (table, parent_name); + + if (grandparent != NULL) + gvdb_item_set_parent (parent, grandparent); + } + + g_free (parent_name); + + return parent; +} + +static gboolean +dconf_gvdb_utils_add_key (const gchar *path, + GVariant *value, + gpointer user_data) +{ + GHashTable *gvdb = user_data; + GvdbItem *item; + + g_assert (g_hash_table_lookup (gvdb, path) == NULL); + item = gvdb_hash_table_insert (gvdb, path); + gvdb_item_set_parent (item, dconf_gvdb_utils_get_parent (gvdb, path)); + gvdb_item_set_value (item, value); + + return TRUE; +} + +gboolean +dconf_gvdb_utils_write_file (const gchar *filename, + DConfChangeset *database, + GError **error) +{ + GHashTable *gvdb; + gboolean success; + + gvdb = gvdb_hash_table_new (NULL, NULL); + dconf_changeset_all (database, dconf_gvdb_utils_add_key, gvdb); + success = gvdb_table_write_contents (gvdb, filename, FALSE, error); + + if (!success) + { + gchar *dirname; + + /* Maybe it failed because the directory doesn't exist. Try + * again, after mkdir(). + */ + dirname = g_path_get_dirname (filename); + g_mkdir_with_parents (dirname, 0700); + g_free (dirname); + + g_clear_error (error); + success = gvdb_table_write_contents (gvdb, filename, FALSE, error); + } + + g_hash_table_unref (gvdb); + + return success; +} diff --git a/common/dconf-gvdb-utils.h b/common/dconf-gvdb-utils.h new file mode 100644 index 0000000..8d73133 --- /dev/null +++ b/common/dconf-gvdb-utils.h @@ -0,0 +1,33 @@ +/* + * Copyright © 2010 Codethink Limited + * Copyright © 2012 Canonical Limited + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the licence, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see . + * + * Author: Ryan Lortie + */ + +#ifndef __dconf_gvdb_utils_h__ +#define __dconf_gvdb_utils_h__ + +#include "./dconf-changeset.h" + +DConfChangeset * dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, + gboolean *file_missing, + GError **error); +gboolean dconf_gvdb_utils_write_file (const gchar *filename, + DConfChangeset *database, + GError **error); + +#endif /* __dconf_gvdb_utils_h__ */ 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/service/dconf-gvdb-utils.c b/service/dconf-gvdb-utils.c deleted file mode 100644 index d77ae97..0000000 --- a/service/dconf-gvdb-utils.c +++ /dev/null @@ -1,219 +0,0 @@ -/* - * Copyright © 2010 Codethink Limited - * Copyright © 2012 Canonical Limited - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2 of the licence, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, see . - * - * Author: Ryan Lortie - */ - -#include "config.h" - -#include "dconf-gvdb-utils.h" - -#include "../common/dconf-paths.h" -#include "../gvdb/gvdb-builder.h" -#include "../gvdb/gvdb-reader.h" - -#include -#include -#include -#include - -DConfChangeset * -dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, - gboolean *file_missing, - GError **error) -{ - DConfChangeset *database; - GError *my_error = NULL; - GvdbTable *table = NULL; - gchar *contents; - gsize size; - - if (g_file_get_contents (filename, &contents, &size, &my_error)) - { - GBytes *bytes; - - bytes = g_bytes_new_take (contents, size); - table = gvdb_table_new_from_bytes (bytes, FALSE, &my_error); - g_bytes_unref (bytes); - } - - /* It is perfectly fine if the file does not exist -- then it's - * just empty. - */ - if (g_error_matches (my_error, G_FILE_ERROR, G_FILE_ERROR_NOENT)) - g_clear_error (&my_error); - - /* Otherwise, we should report errors to prevent ourselves from - * overwriting the database in other situations... - */ - if (g_error_matches (my_error, G_FILE_ERROR, G_FILE_ERROR_INVAL)) - { - /* Move the database to a backup file, warn and continue with a new - * database. The alternative is erroring out and exiting the daemon, - * which leaves the user’s session essentially unusable. - * - * The code to find an unused backup filename is racy, but this is an - * error handling path. Who cares. */ - g_autofree gchar *backup_filename = NULL; - guint i; - - for (i = 0; - i < G_MAXUINT && - (backup_filename == NULL || g_file_test (backup_filename, G_FILE_TEST_EXISTS)); - i++) - { - g_free (backup_filename); - backup_filename = g_strdup_printf ("%s~%u", filename, i); - } - - if (g_rename (filename, backup_filename) != 0) - g_warning ("Error renaming corrupt database from ‘%s’ to ‘%s’: %s", - filename, backup_filename, g_strerror (errno)); - else - g_warning ("Database ‘%s’ was corrupt: moved it to ‘%s’ and created an empty replacement", - filename, backup_filename); - - g_clear_error (&my_error); - } - else if (my_error) - { - g_propagate_prefixed_error (error, my_error, "Cannot open dconf database: "); - 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]); - } - - gvdb_table_free (table); - g_free (names); - } - - if (file_missing) - *file_missing = (table == NULL); - - return database; -} - -static GvdbItem * -dconf_gvdb_utils_get_parent (GHashTable *table, - const gchar *key) -{ - GvdbItem *grandparent, *parent; - gchar *parent_name; - gint len; - - if (g_str_equal (key, "/")) - return NULL; - - len = strlen (key); - if (key[len - 1] == '/') - len--; - - while (key[len - 1] != '/') - len--; - - parent_name = g_strndup (key, len); - parent = g_hash_table_lookup (table, parent_name); - - if (parent == NULL) - { - parent = gvdb_hash_table_insert (table, parent_name); - - grandparent = dconf_gvdb_utils_get_parent (table, parent_name); - - if (grandparent != NULL) - gvdb_item_set_parent (parent, grandparent); - } - - g_free (parent_name); - - return parent; -} - -static gboolean -dconf_gvdb_utils_add_key (const gchar *path, - GVariant *value, - gpointer user_data) -{ - GHashTable *gvdb = user_data; - GvdbItem *item; - - g_assert (g_hash_table_lookup (gvdb, path) == NULL); - item = gvdb_hash_table_insert (gvdb, path); - gvdb_item_set_parent (item, dconf_gvdb_utils_get_parent (gvdb, path)); - gvdb_item_set_value (item, value); - - return TRUE; -} - -gboolean -dconf_gvdb_utils_write_file (const gchar *filename, - DConfChangeset *database, - GError **error) -{ - GHashTable *gvdb; - gboolean success; - - gvdb = gvdb_hash_table_new (NULL, NULL); - dconf_changeset_all (database, dconf_gvdb_utils_add_key, gvdb); - success = gvdb_table_write_contents (gvdb, filename, FALSE, error); - - if (!success) - { - gchar *dirname; - - /* Maybe it failed because the directory doesn't exist. Try - * again, after mkdir(). - */ - dirname = g_path_get_dirname (filename); - g_mkdir_with_parents (dirname, 0700); - g_free (dirname); - - g_clear_error (error); - success = gvdb_table_write_contents (gvdb, filename, FALSE, error); - } - - g_hash_table_unref (gvdb); - - return success; -} diff --git a/service/dconf-gvdb-utils.h b/service/dconf-gvdb-utils.h deleted file mode 100644 index 7076781..0000000 --- a/service/dconf-gvdb-utils.h +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Copyright © 2010 Codethink Limited - * Copyright © 2012 Canonical Limited - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2 of the licence, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, see . - * - * Author: Ryan Lortie - */ - -#ifndef __dconf_gvdb_utils_h__ -#define __dconf_gvdb_utils_h__ - -#include "../common/dconf-changeset.h" - -DConfChangeset * dconf_gvdb_utils_read_and_back_up_file (const gchar *filename, - gboolean *file_missing, - GError **error); -gboolean dconf_gvdb_utils_write_file (const gchar *filename, - DConfChangeset *database, - GError **error); - -#endif /* __dconf_gvdb_utils_h__ */ 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, -- cgit v1.2.1 From 6edda52ed45feaaec203e51a23f4b50cc7d4b147 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 6 Jan 2020 12:10:58 +1100 Subject: common: factor out dconf_gvdb_utils_changeset_from_table --- common/dconf-gvdb-utils.c | 61 ++++++++++++++++++++++++++--------------------- common/dconf-gvdb-utils.h | 2 ++ 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/common/dconf-gvdb-utils.c b/common/dconf-gvdb-utils.c index e70e2dc..56609cc 100644 --- a/common/dconf-gvdb-utils.c +++ b/common/dconf-gvdb-utils.c @@ -31,6 +31,37 @@ #include #include +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, @@ -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); diff --git a/common/dconf-gvdb-utils.h b/common/dconf-gvdb-utils.h index 8d73133..4bd4cd9 100644 --- a/common/dconf-gvdb-utils.h +++ b/common/dconf-gvdb-utils.h @@ -21,8 +21,10 @@ #ifndef __dconf_gvdb_utils_h__ #define __dconf_gvdb_utils_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); -- cgit v1.2.1 From 5b24a937e12c85294d1172e41d21eb8504437f21 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 6 Jan 2020 12:41:23 +1100 Subject: common: factor out dconf_gvdb_utils_table_from_changeset --- common/dconf-gvdb-utils.c | 13 +++++++++++-- common/dconf-gvdb-utils.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/common/dconf-gvdb-utils.c b/common/dconf-gvdb-utils.c index 56609cc..0aced4c 100644 --- a/common/dconf-gvdb-utils.c +++ b/common/dconf-gvdb-utils.c @@ -193,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, @@ -201,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/common/dconf-gvdb-utils.h b/common/dconf-gvdb-utils.h index 4bd4cd9..799c66c 100644 --- a/common/dconf-gvdb-utils.h +++ b/common/dconf-gvdb-utils.h @@ -28,6 +28,7 @@ DConfChangeset * dconf_gvdb_utils_changeset_from_table (GvdbTab 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); -- cgit v1.2.1 From 4f90c7927335712edd875cbea395c01fb48f748b Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Tue, 5 May 2020 21:04:30 +1000 Subject: engine: seal the in_flight queue before sending it It would be wrong to change the in_flight queue, so seal it to ensure that it is consistently sealed. --- engine/dconf-engine.c | 1 + 1 file changed, 1 insertion(+) diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c index 18b8aa5..cbb2a00 100644 --- a/engine/dconf-engine.c +++ b/engine/dconf-engine.c @@ -1208,6 +1208,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); -- cgit v1.2.1 From 5ed0724cd763cf79dcc8f6802a6aa093d635cac7 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Tue, 12 Dec 2017 00:18:18 +1100 Subject: engine: remove spurious local change notifications When used with the "fast" (optimistic concurrency) API, the engine library emits a change notification local to a process after a change is initiated from that process. Previously, it would emit this notification even if the newly written value was the same as the previous value (according to that process's view of the state). After this change, the local change notification is not sent unless the new value is different from the current value (as seen by that process). --- engine/dconf-engine.c | 84 +++++++++++++++++++++++++- tests/engine.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 241 insertions(+), 5 deletions(-) diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c index cbb2a00..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 #include @@ -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, @@ -1274,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; @@ -1298,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/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); -- cgit v1.2.1