From dfd8e7dd5838041cf0d30a825329b456e6eaa8ee Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 14:18:52 +0000 Subject: [PATCH 01/15] debian: Fix placeholder in default server URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 5fd72961012a5388ba9ea41ec10f675370ac7412 the server URL can optionally contains an ${environment} placeholder. The default URL is a build-time parameter. But in a Makefile – which debian/rules is – `$` is a special character indicating variable expansion. So previously the `${environment}` placeholder in the URL was being expanded at build time to the empty string, meaning the default server URL became `https://.metrics.endlessm.com/`. Escape the dollar sign. https://phabricator.endlessm.com/T35165 --- debian/rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/rules b/debian/rules index 1eb7881..6331803 100755 --- a/debian/rules +++ b/debian/rules @@ -14,7 +14,7 @@ # Build our documentation as well override_dh_auto_configure: - dh_auto_configure -- -Ddefault_metrics_server_url='https://${environment}.metrics.endlessm.com' + dh_auto_configure -- -Ddefault_metrics_server_url='https://$${environment}.metrics.endlessm.com' # Avoid changing the owner of configuration files and the persistent cache # directory to root:root. From 5852d0f096073d4bf9e2ecb6d9f3097e63650ad6 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 14:53:57 +0000 Subject: [PATCH 02/15] permissions-provider: Modernise GObject boilerplate https://phabricator.endlessm.com/T35165 --- daemon/emer-permissions-provider.c | 114 ++++++----------------- daemon/emer-permissions-provider.h | 42 +-------- tests/daemon/mock-permissions-provider.c | 35 +++---- 3 files changed, 47 insertions(+), 144 deletions(-) diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index 9b6019a..6e0c67e 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -31,8 +31,10 @@ #include "shared/metrics-util.h" -typedef struct _EmerPermissionsProviderPrivate +typedef struct _EmerPermissionsProvider { + GObject parent; + /* Permissions, cached from config file */ GKeyFile *permissions; @@ -44,9 +46,9 @@ typedef struct _EmerPermissionsProviderPrivate /* For reading the OSTree config file */ GFile *ostree_config_file; -} EmerPermissionsProviderPrivate; +} EmerPermissionsProvider; -G_DEFINE_TYPE_WITH_PRIVATE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT) +G_DEFINE_TYPE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT) #define DAEMON_GLOBAL_GROUP_NAME "global" #define DAEMON_ENABLED_KEY_NAME "enabled" @@ -77,10 +79,7 @@ static GParamSpec *emer_permissions_provider_props[NPROPS] = { NULL, }; static void load_fallback_data (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - if (g_key_file_load_from_data (priv->permissions, FALLBACK_CONFIG_FILE_DATA, + if (g_key_file_load_from_data (self->permissions, FALLBACK_CONFIG_FILE_DATA, -1, G_KEY_FILE_NONE, NULL)) return; @@ -93,14 +92,11 @@ load_fallback_data (EmerPermissionsProvider *self) static void write_config_file_sync (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - gchar *permissions_config_file_path = - g_file_get_path (priv->permissions_config_file); + g_file_get_path (self->permissions_config_file); GError *error = NULL; - if (!g_key_file_save_to_file (priv->permissions, permissions_config_file_path, + if (!g_key_file_save_to_file (self->permissions, permissions_config_file_path, &error)) { g_critical ("Could not write to permissions config file '%s'. Error: %s.", @@ -115,11 +111,9 @@ static gboolean write_config_file_idle_cb (gpointer data) { EmerPermissionsProvider *self = EMER_PERMISSIONS_PROVIDER (data); - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); write_config_file_sync (self); - priv->write_config_file_idle_id = 0; + self->write_config_file_idle_id = 0; return G_SOURCE_REMOVE; } @@ -128,13 +122,10 @@ write_config_file_idle_cb (gpointer data) static void schedule_config_file_update (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - if (priv->write_config_file_idle_id != 0) + if (self->write_config_file_idle_id != 0) return; - priv->write_config_file_idle_id = + self->write_config_file_idle_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE, write_config_file_idle_cb, g_object_ref (self), @@ -146,14 +137,11 @@ default. Also emits a property notification. */ static void read_config_file_sync (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - GError *error = NULL; - gchar *path = g_file_get_path (priv->permissions_config_file); + gchar *path = g_file_get_path (self->permissions_config_file); gboolean load_succeeded = - g_key_file_load_from_file (priv->permissions, path, G_KEY_FILE_NONE, &error); + g_key_file_load_from_file (self->permissions, path, G_KEY_FILE_NONE, &error); if (!load_succeeded) { load_fallback_data (self); @@ -187,10 +175,7 @@ read_config_file_sync (EmerPermissionsProvider *self) static gchar * get_ostree_url_from_file (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - gchar *path = g_file_get_path (priv->ostree_config_file); + gchar *path = g_file_get_path (self->ostree_config_file); if (path == NULL) { @@ -272,10 +257,7 @@ get_ostree_url_from_ostree_repo (void) static gchar * read_ostree_url (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - if (priv->ostree_config_file == NULL) + if (self->ostree_config_file == NULL) return get_ostree_url_from_ostree_repo (); else return get_ostree_url_from_file (self); @@ -284,11 +266,8 @@ read_ostree_url (EmerPermissionsProvider *self) static gchar * read_environment (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - GError *error = NULL; - gchar *environment = g_key_file_get_value (priv->permissions, + gchar *environment = g_key_file_get_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENVIRONMENT_KEY_NAME, &error); @@ -325,10 +304,7 @@ static void set_environment (EmerPermissionsProvider *self, const gchar *environment) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - g_key_file_set_string (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_set_string (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENVIRONMENT_KEY_NAME, environment); schedule_config_file_update (self); @@ -350,10 +326,7 @@ static void set_config_file_path (EmerPermissionsProvider *self, const gchar *path) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->permissions_config_file = g_file_new_for_path (path); + self->permissions_config_file = g_file_new_for_path (path); } /* Construct-only setter */ @@ -364,10 +337,7 @@ set_ostree_config_file_path (EmerPermissionsProvider *self, if (path == NULL) return; - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->ostree_config_file = g_file_new_for_path (path); + self->ostree_config_file = g_file_new_for_path (path); } static void @@ -442,12 +412,10 @@ static void emer_permissions_provider_finalize (GObject *object) { EmerPermissionsProvider *self = EMER_PERMISSIONS_PROVIDER (object); - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - g_key_file_unref (priv->permissions); - g_clear_object (&priv->permissions_config_file); - g_clear_object (&priv->ostree_config_file); + g_key_file_unref (self->permissions); + g_clear_object (&self->permissions_config_file); + g_clear_object (&self->ostree_config_file); G_OBJECT_CLASS (emer_permissions_provider_parent_class)->finalize (object); } @@ -495,11 +463,8 @@ emer_permissions_provider_class_init (EmerPermissionsProviderClass *klass) static void emer_permissions_provider_init (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->permissions = g_key_file_new (); - priv->ostree_config_file = NULL; + self->permissions = g_key_file_new (); + self->ostree_config_file = NULL; } /* PUBLIC API */ @@ -551,12 +516,9 @@ emer_permissions_provider_new_full (const gchar *permissions_config_file_path, gboolean emer_permissions_provider_get_daemon_enabled (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - GError *error = NULL; gboolean daemon_enabled = - g_key_file_get_boolean (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_get_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENABLED_KEY_NAME, &error); if (error != NULL) { @@ -581,10 +543,7 @@ void emer_permissions_provider_set_daemon_enabled (EmerPermissionsProvider *self, gboolean enabled) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - g_key_file_set_boolean (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_set_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENABLED_KEY_NAME, enabled); schedule_config_file_update (self); @@ -608,12 +567,9 @@ emer_permissions_provider_set_daemon_enabled (EmerPermissionsProvider *self, gboolean emer_permissions_provider_get_uploading_enabled (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - GError *error = NULL; gboolean uploading_enabled = - g_key_file_get_boolean (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_get_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_UPLOADING_ENABLED_KEY_NAME, &error); if (error != NULL) { @@ -641,10 +597,7 @@ void emer_permissions_provider_set_uploading_enabled (EmerPermissionsProvider *self, gboolean enabled) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - g_key_file_set_boolean (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_set_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_UPLOADING_ENABLED_KEY_NAME, enabled); schedule_config_file_update (self); @@ -706,12 +659,9 @@ emer_permissions_provider_get_environment (EmerPermissionsProvider *self) gchar * emer_permissions_provider_get_server_url (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - GError *error = NULL; gchar *server_url = - g_key_file_get_value (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_get_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_SERVER_URL_KEY_NAME, &error); if (error != NULL) { @@ -737,10 +687,7 @@ void emer_permissions_provider_set_server_url (EmerPermissionsProvider *self, const gchar *server_url) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - g_key_file_set_value (priv->permissions, DAEMON_GLOBAL_GROUP_NAME, + g_key_file_set_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_SERVER_URL_KEY_NAME, server_url); schedule_config_file_update (self); @@ -749,3 +696,4 @@ emer_permissions_provider_set_server_url (EmerPermissionsProvider *self, emer_permissions_provider_props[PROP_SERVER_URL]; g_object_notify_by_pspec (G_OBJECT (self), server_url_pspec); } + diff --git a/daemon/emer-permissions-provider.h b/daemon/emer-permissions-provider.h index e2bb52f..49706cf 100644 --- a/daemon/emer-permissions-provider.h +++ b/daemon/emer-permissions-provider.h @@ -28,43 +28,11 @@ G_BEGIN_DECLS #define EMER_TYPE_PERMISSIONS_PROVIDER emer_permissions_provider_get_type() - -#define EMER_PERMISSIONS_PROVIDER(obj) \ - (G_TYPE_CHECK_INSTANCE_CAST ((obj), \ - EMER_TYPE_PERMISSIONS_PROVIDER, EmerPermissionsProvider)) - -#define EMER_PERMISSIONS_PROVIDER_CLASS(klass) \ - (G_TYPE_CHECK_CLASS_CAST ((klass), \ - EMER_TYPE_PERMISSIONS_PROVIDER, EmerPermissionsProviderClass)) - -#define EMER_IS_PERMISSIONS_PROVIDER(obj) \ - (G_TYPE_CHECK_INSTANCE_TYPE ((obj), \ - EMER_TYPE_PERMISSIONS_PROVIDER)) - -#define EMER_IS_PERMISSIONS_PROVIDER_CLASS(klass) \ - (G_TYPE_CHECK_CLASS_TYPE ((klass), \ - EMER_TYPE_PERMISSIONS_PROVIDER)) - -#define EMER_PERMISSIONS_PROVIDER_GET_CLASS(obj) \ - (G_TYPE_INSTANCE_GET_CLASS ((obj), \ - EMER_TYPE_PERMISSIONS_PROVIDER, EmerPermissionsProviderClass)) - -typedef struct _EmerPermissionsProvider EmerPermissionsProvider; -typedef struct _EmerPermissionsProviderClass EmerPermissionsProviderClass; - -struct _EmerPermissionsProvider -{ - GObject parent; -}; - -struct _EmerPermissionsProviderClass -{ - GObjectClass parent_class; -}; - -G_DEFINE_AUTOPTR_CLEANUP_FUNC(EmerPermissionsProvider, g_object_unref) - -GType emer_permissions_provider_get_type (void) G_GNUC_CONST; +G_DECLARE_FINAL_TYPE (EmerPermissionsProvider, + emer_permissions_provider, + EMER, + PERMISSIONS_PROVIDER, + GObject) EmerPermissionsProvider *emer_permissions_provider_new (void); diff --git a/tests/daemon/mock-permissions-provider.c b/tests/daemon/mock-permissions-provider.c index 5d1c110..6ededc7 100644 --- a/tests/daemon/mock-permissions-provider.c +++ b/tests/daemon/mock-permissions-provider.c @@ -27,13 +27,15 @@ #include #include -typedef struct _EmerPermissionsProviderPrivate +typedef struct _EmerPermissionsProvider { + GObject parent; + gboolean daemon_enabled; gboolean uploading_enabled; -} EmerPermissionsProviderPrivate; +} EmerPermissionsProvider; -G_DEFINE_TYPE_WITH_PRIVATE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT) +G_DEFINE_TYPE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT) static void emer_permissions_provider_class_init (EmerPermissionsProviderClass *klass) @@ -43,11 +45,8 @@ emer_permissions_provider_class_init (EmerPermissionsProviderClass *klass) static void emer_permissions_provider_init (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->daemon_enabled = TRUE; - priv->uploading_enabled = TRUE; + self->daemon_enabled = TRUE; + self->uploading_enabled = TRUE; } /* MOCK PUBLIC API */ @@ -68,20 +67,14 @@ emer_permissions_provider_new_full (const gchar *config_file_path, gboolean emer_permissions_provider_get_daemon_enabled (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - return priv->daemon_enabled; + return self->daemon_enabled; } void emer_permissions_provider_set_daemon_enabled (EmerPermissionsProvider *self, gboolean enabled) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->daemon_enabled = enabled; + self->daemon_enabled = enabled; /* Emit a property notification even though there isn't a property by this * name in this mock object. @@ -92,10 +85,7 @@ emer_permissions_provider_set_daemon_enabled (EmerPermissionsProvider *self, gboolean emer_permissions_provider_get_uploading_enabled (EmerPermissionsProvider *self) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - return priv->uploading_enabled; + return self->uploading_enabled; } gchar * @@ -127,10 +117,7 @@ void emer_permissions_provider_set_uploading_enabled (EmerPermissionsProvider *self, gboolean uploading_enabled) { - EmerPermissionsProviderPrivate *priv = - emer_permissions_provider_get_instance_private (self); - - priv->uploading_enabled = uploading_enabled; + self->uploading_enabled = uploading_enabled; /* Emit a property notification even though there isn't a property by this * name in this mock object. From cb4b50b34e477660028f27a22f507d7ea8af5209 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 14:56:57 +0000 Subject: [PATCH 03/15] permissions-provider: Make :server-url property read-only Nothing has any need to change this property from outside the class. https://phabricator.endlessm.com/T35165 --- daemon/emer-permissions-provider.c | 29 +---------------------------- daemon/emer-permissions-provider.h | 3 --- 2 files changed, 1 insertion(+), 31 deletions(-) diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index 6e0c67e..4748d15 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -398,11 +398,6 @@ emer_permissions_provider_set_property (GObject *object, g_value_get_boolean (value)); break; - case PROP_SERVER_URL: - emer_permissions_provider_set_server_url (self, - g_value_get_string (value)); - break; - default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); } @@ -454,7 +449,7 @@ emer_permissions_provider_class_init (EmerPermissionsProviderClass *klass) g_param_spec_string ("server-url", "Server URL", "Metrics server URL to use", NULL, - G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS); + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS); g_object_class_install_properties (object_class, NPROPS, emer_permissions_provider_props); @@ -675,25 +670,3 @@ emer_permissions_provider_get_server_url (EmerPermissionsProvider *self) return server_url; } - -/* - * emer_permissions_provider_set_server_url: - * @self: the permissions provider - * @server_url: the metrics server URL to use - * - * Sets the metrics server URL in the configuration file. - */ -void -emer_permissions_provider_set_server_url (EmerPermissionsProvider *self, - const gchar *server_url) -{ - g_key_file_set_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, - DAEMON_SERVER_URL_KEY_NAME, server_url); - - schedule_config_file_update (self); - - GParamSpec *server_url_pspec = - emer_permissions_provider_props[PROP_SERVER_URL]; - g_object_notify_by_pspec (G_OBJECT (self), server_url_pspec); -} - diff --git a/daemon/emer-permissions-provider.h b/daemon/emer-permissions-provider.h index 49706cf..36ef331 100644 --- a/daemon/emer-permissions-provider.h +++ b/daemon/emer-permissions-provider.h @@ -53,9 +53,6 @@ gchar *emer_permissions_provider_get_environment (EmerPe gchar *emer_permissions_provider_get_server_url (EmerPermissionsProvider *self); -void emer_permissions_provider_set_server_url (EmerPermissionsProvider *self, - const gchar *server_url); - G_END_DECLS #endif /* EMER_PERMISSIONS_PROVIDER_H */ From a6cf38ce7222e21be3ee5ab2839e303148171f85 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 15:17:47 +0000 Subject: [PATCH 04/15] daemon: Add protocol version to URL later This makes no functional difference but will make some future changes a little simpler. https://phabricator.endlessm.com/T35165 --- daemon/emer-daemon.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/daemon/emer-daemon.c b/daemon/emer-daemon.c index 037499c..1385a6b 100644 --- a/daemon/emer-daemon.c +++ b/daemon/emer-daemon.c @@ -364,7 +364,7 @@ get_http_request_url (EmerDaemon *self, gchar *checksum = g_compute_checksum_for_data (G_CHECKSUM_SHA512, data, length); gchar *http_request_url_string = - g_build_filename (self->server_url, checksum, NULL); + g_build_filename (self->server_url, CLIENT_VERSION_NUMBER, checksum, NULL); g_free (checksum); g_autoptr(GError) error = NULL; @@ -1034,10 +1034,7 @@ set_server_url (EmerDaemon *self, const gchar *server_url) { g_free (self->server_url); - self->server_url = NULL; - - if (server_url != NULL) - self->server_url = g_build_filename (server_url, CLIENT_VERSION_NUMBER "/", NULL); + self->server_url = g_strdup (server_url); } static void From 4219a5e6137bb0b3962db089312e89d12cb1783f Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 15:19:00 +0000 Subject: [PATCH 05/15] tests: Use g_auto* a little This just removes a couple of lines from this test. Spotted while checking that the protocol version in the URL was actually being tested. https://phabricator.endlessm.com/T35165 --- tests/daemon/test-daemon.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/daemon/test-daemon.c b/tests/daemon/test-daemon.c index 819863c..8447a9e 100644 --- a/tests/daemon/test-daemon.c +++ b/tests/daemon/test-daemon.c @@ -584,22 +584,18 @@ get_events_from_request (GByteArray *request, emtr_util_get_current_time (CLOCK_REALTIME, &curr_absolute_time); g_assert_true (get_succeeded); - GBytes *request_bytes = g_bytes_new (request->data, request->len); + g_autoptr(GBytes) request_bytes = g_bytes_new (request->data, request->len); - gchar *checksum = + g_autofree gchar *checksum = g_compute_checksum_for_bytes (G_CHECKSUM_SHA512, request_bytes); - gchar *expected_request_path = g_build_filename ("/3/", checksum, NULL); - g_free (checksum); + g_autofree gchar *expected_request_path = g_build_filename ("/3/", checksum, NULL); g_assert_cmpstr (fixture->request_path, ==, expected_request_path); - g_free (expected_request_path); const GVariantType *REQUEST_FORMAT = G_VARIANT_TYPE ("(xxsa{ss}ya(aysxmv)a(ayssumv))"); GVariant *request_variant = g_variant_new_from_bytes (REQUEST_FORMAT, request_bytes, FALSE); - g_bytes_unref (request_bytes); - g_assert_true (g_variant_is_normal_form (request_variant)); g_variant_ref_sink (request_variant); From 28b6d9c46d8c8aa1d8b2e3cbf48306e7ab329898 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 15:47:53 +0000 Subject: [PATCH 06/15] daemon: Remove server-url property EmerPermissionsProvider now has a method to fetch the server URL. Make this method return the build-time default value if no value is specified in the config file, and adjust EmerDaemon to only use that method. Extend mock-permissions-provider to return a construct-time value. (It is a little unusual to return a freshly-allocated copy of a static from emer_permissions_provider_get_server_url() but that is how that function already works.) https://phabricator.endlessm.com/T35165 --- daemon/emer-daemon.c | 42 +----------------------- daemon/emer-daemon.h | 1 - daemon/emer-permissions-provider.c | 2 +- tests/daemon/mock-permissions-provider.c | 27 ++++++++++++++- tests/daemon/mock-permissions-provider.h | 2 ++ tests/daemon/test-daemon.c | 3 +- 6 files changed, 31 insertions(+), 46 deletions(-) diff --git a/daemon/emer-daemon.c b/daemon/emer-daemon.c index 1385a6b..cb596e0 100644 --- a/daemon/emer-daemon.c +++ b/daemon/emer-daemon.c @@ -175,7 +175,6 @@ enum { PROP_0, PROP_RANDOM_NUMBER_GENERATOR, - PROP_SERVER_URL, PROP_NETWORK_SEND_INTERVAL, PROP_PERMISSIONS_PROVIDER, PROP_PERSISTENT_CACHE_DIRECTORY, @@ -1029,14 +1028,6 @@ set_random_number_generator (EmerDaemon *self, self->rand = rand == NULL ? g_rand_new () : rand; } -static void -set_server_url (EmerDaemon *self, - const gchar *server_url) -{ - g_free (self->server_url); - self->server_url = g_strdup (server_url); -} - static void set_network_send_interval (EmerDaemon *self, guint seconds) @@ -1391,14 +1382,7 @@ emer_daemon_constructed (GObject *object) } buffer_past_aggregate_events (self); - if (self->server_url == NULL) - { - g_autofree gchar *server_url = emer_permissions_provider_get_server_url (self->permissions_provider); - if (server_url != NULL) - set_server_url (self, server_url); - else - set_server_url (self, DEFAULT_METRICS_SERVER_URL); - } + self->server_url = emer_permissions_provider_get_server_url (self->permissions_provider); gchar *environment = emer_permissions_provider_get_environment (self->permissions_provider); @@ -1445,10 +1429,6 @@ emer_daemon_set_property (GObject *object, set_random_number_generator (self, g_value_get_pointer (value)); break; - case PROP_SERVER_URL: - set_server_url (self, g_value_get_string (value)); - break; - case PROP_NETWORK_SEND_INTERVAL: set_network_send_interval (self, g_value_get_uint (value)); break; @@ -1544,20 +1524,6 @@ emer_daemon_class_init (EmerDaemonClass *klass) G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE | G_PARAM_STATIC_STRINGS); - /* - * EmerDaemon:server-url: - * - * The URL to which events are uploaded. The URL must contain the protocol and - * may contain the port number. If unspecified, the port number defaults to - * 443, which is the standard port number for SSL. - */ - emer_daemon_props[PROP_SERVER_URL] = - g_param_spec_string ("server-url", "Server URL", - "URL to which events are uploaded", - NULL, - G_PARAM_CONSTRUCT | G_PARAM_WRITABLE | - G_PARAM_STATIC_STRINGS); - /* * EmerDaemon:network-send-interval: * @@ -1700,10 +1666,6 @@ emer_daemon_new (const gchar *persistent_cache_directory, * emer_daemon_new_full: * @rand: (allow-none): random number generator to use for randomized * exponential backoff, or %NULL to use the default. - * @server_url: (allow-none): the URL (including protocol and, optionally, port - * number) to which to upload events, or %NULL to use the default. Must - * include trailing forward slash. If the port number is unspecified, it - * defaults to 443 (the standard port used by SSL). * @network_send_interval: frequency in seconds with which the client will * attempt a network send request. * @permissions_provider: The #EmerPermissionsProvider to supply information @@ -1723,7 +1685,6 @@ emer_daemon_new (const gchar *persistent_cache_directory, */ EmerDaemon * emer_daemon_new_full (GRand *rand, - const gchar *server_url, guint network_send_interval, EmerPermissionsProvider *permissions_provider, EmerPersistentCache *persistent_cache, @@ -1732,7 +1693,6 @@ emer_daemon_new_full (GRand *rand, { return g_object_new (EMER_TYPE_DAEMON, "random-number-generator", rand, - "server-url", server_url, "network-send-interval", network_send_interval, "permissions-provider", permissions_provider, "persistent-cache", persistent_cache, diff --git a/daemon/emer-daemon.h b/daemon/emer-daemon.h index 92c4d87..ba4896b 100644 --- a/daemon/emer-daemon.h +++ b/daemon/emer-daemon.h @@ -40,7 +40,6 @@ EmerDaemon * emer_daemon_new (const gchar EmerPermissionsProvider *permissions_provider); EmerDaemon * emer_daemon_new_full (GRand *rand, - const gchar *server_url, guint network_send_interval, EmerPermissionsProvider *permissions_provider, EmerPersistentCache *persistent_cache, diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index 4748d15..32b95df 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -665,7 +665,7 @@ emer_permissions_provider_get_server_url (EmerPermissionsProvider *self) DAEMON_GLOBAL_GROUP_NAME, DAEMON_SERVER_URL_KEY_NAME, error->message); g_error_free (error); - return NULL; + return g_strdup (DEFAULT_METRICS_SERVER_URL); } return server_url; diff --git a/tests/daemon/mock-permissions-provider.c b/tests/daemon/mock-permissions-provider.c index 6ededc7..f2c94d6 100644 --- a/tests/daemon/mock-permissions-provider.c +++ b/tests/daemon/mock-permissions-provider.c @@ -33,13 +33,29 @@ typedef struct _EmerPermissionsProvider gboolean daemon_enabled; gboolean uploading_enabled; + + gchar *server_url; } EmerPermissionsProvider; G_DEFINE_TYPE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT) +static void +emer_permissions_provider_finalize (GObject *object) +{ + EmerPermissionsProvider *self = EMER_PERMISSIONS_PROVIDER (object); + + g_clear_pointer (&self->server_url, g_free); + + G_OBJECT_CLASS (emer_permissions_provider_parent_class)->finalize (object); +} + static void emer_permissions_provider_class_init (EmerPermissionsProviderClass *klass) { + GObjectClass *object_class = G_OBJECT_CLASS (klass); + + object_class->finalize = emer_permissions_provider_finalize; + } static void @@ -47,6 +63,7 @@ emer_permissions_provider_init (EmerPermissionsProvider *self) { self->daemon_enabled = TRUE; self->uploading_enabled = TRUE; + self->server_url = NULL; } /* MOCK PUBLIC API */ @@ -64,6 +81,14 @@ emer_permissions_provider_new_full (const gchar *config_file_path, return emer_permissions_provider_new (); } +EmerPermissionsProvider * +mock_permissions_provider_new (const gchar *server_url) +{ + EmerPermissionsProvider *self = emer_permissions_provider_new (); + self->server_url = g_strdup (server_url); + return self; +} + gboolean emer_permissions_provider_get_daemon_enabled (EmerPermissionsProvider *self) { @@ -106,7 +131,7 @@ emer_permissions_provider_get_environment (EmerPermissionsProvider *self) gchar * emer_permissions_provider_get_server_url (EmerPermissionsProvider *self) { - return NULL; + return g_strdup (self->server_url); } /* API OF MOCK OBJECT */ diff --git a/tests/daemon/mock-permissions-provider.h b/tests/daemon/mock-permissions-provider.h index 1aec02e..8f9eb9f 100644 --- a/tests/daemon/mock-permissions-provider.h +++ b/tests/daemon/mock-permissions-provider.h @@ -29,6 +29,8 @@ G_BEGIN_DECLS +EmerPermissionsProvider *mock_permissions_provider_new (const gchar *server_url); + G_END_DECLS #endif /* MOCK_PERMISSIONS_PROVIDER_H */ diff --git a/tests/daemon/test-daemon.c b/tests/daemon/test-daemon.c index 8447a9e..f48f22c 100644 --- a/tests/daemon/test-daemon.c +++ b/tests/daemon/test-daemon.c @@ -822,7 +822,6 @@ create_test_object (Fixture *fixture) { fixture->test_object = emer_daemon_new_full (g_rand_new_with_seed (18), - fixture->server_url, 2 /* network send interval */, fixture->mock_permissions_provider, fixture->mock_persistent_cache, @@ -850,7 +849,7 @@ setup_most (Fixture *fixture, fixture->server_url = get_server_url (fixture->mock_server); - fixture->mock_permissions_provider = emer_permissions_provider_new (); + fixture->mock_permissions_provider = mock_permissions_provider_new (fixture->server_url); fixture->mock_persistent_cache = NULL; /* Not actually a mock! */ fixture->mock_aggregate_tally = emer_aggregate_tally_new (g_get_user_cache_dir ()); From 435d2fda3e49318f6c20c00d3b363152a5218154 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 16:20:59 +0000 Subject: [PATCH 07/15] Expand server URL on every upload Previously, EmerPermissionsProvider would return the raw string value from the config file (or a built-in default value), and EmerDaemon would expand any ${environment} placeholder therein. EmerDaemon.server_url would cache the result of this expansion after the first upload, so subsequent changes to the environment (such as by changing the OSTree server URL) would not be reflected. Move expansion of the URL to EmerPermissionsProvider, and have EmerDaemon call that function each time the URL is used. The test change is necessary because of some really weird emergent behaviour of this code. Normally Unix services read their configuration files once at startup, when explicitly asked to re-read them (conventionally with SIGHUP), but not at other times; and never write to them. However, EmerPermissionsProvider is not normal: 1. It re-reads its config file every time you call a function like emer_permissions_provider_get_environment(), so its properties like EmerPermissionsProvider:enabled can change in response to you calling an accessor function! 2. Reading a property of EmerPermissionsProvider can cause the file to be written: a. If an error occurs when reading the keyfile, such as the keyfile not existing, EmerPermissionsProvider writes the defaults back to that file. b. emer_permissions_provider_get_environment() inspects the system's 'eos' OSTree remote to see if it looks like a developer system; if so, and if the configured environment is 'prod', it spontaneously changes the environment to 'dev' (and overwrites the config file). 3. The daemon has a SetEnabled() D-Bus method which is wired up to emer_permissions_provider_set_enabled(), which also writes the file. In previous incarnations of this code, calling UploadMetrics() would cause the environment to be read prior to checking whether metrics are enabled. This would in turn cause the config file to be written if it didn't previously exist. And this test is relying on that behaviour to inspect the daemon's internal state. (Interestingly the test also claimed that the daemon never reads its config once it is started, but in fact it relies on it doing so.) Now, the environment is not consulted until after checking whether metrics are enabled and determining that they are. This breaks the weird behaviour that the test is relying on. https://phabricator.endlessm.com/T35165 --- daemon/emer-daemon.c | 52 +++++++++++------------------- daemon/emer-permissions-provider.c | 31 +++++++++++------- tests/test-opt-out-integration.py | 22 ++++++------- 3 files changed, 49 insertions(+), 56 deletions(-) diff --git a/daemon/emer-daemon.c b/daemon/emer-daemon.c index cb596e0..87b1312 100644 --- a/daemon/emer-daemon.c +++ b/daemon/emer-daemon.c @@ -150,8 +150,6 @@ struct _EmerDaemon GRand *rand; - gchar *server_url; - guint upload_events_timeout_source_id; guint report_invalid_cache_data_source_id; guint dispatch_aggregate_timers_daily_source_id; @@ -360,10 +358,12 @@ get_http_request_url (EmerDaemon *self, const guchar *data, gsize length) { + g_autofree gchar *server_url = + emer_permissions_provider_get_server_url (self->permissions_provider); gchar *checksum = g_compute_checksum_for_data (G_CHECKSUM_SHA512, data, length); gchar *http_request_url_string = - g_build_filename (self->server_url, CLIENT_VERSION_NUMBER, checksum, NULL); + g_build_filename (server_url, CLIENT_VERSION_NUMBER, checksum, NULL); g_free (checksum); g_autoptr(GError) error = NULL; @@ -551,12 +551,17 @@ handle_http_response (GObject *source_object, flush_to_persistent_cache (self); } + /* Log URL without checksum */ + GUri *url = soup_message_get_uri (http_message); + g_autoptr(GUri) base = g_uri_parse_relative (url, ".", G_URI_FLAGS_NONE, &error); + g_autofree gchar *base_str = g_uri_to_string (base); + g_message ("Uploaded " "%" G_GSIZE_FORMAT " events from persistent cache, " "%" G_GSIZE_FORMAT " events from buffer to %s.", callback_data->num_stored_events, callback_data->num_buffer_events, - self->server_url); + base_str); g_task_return_boolean (upload_task, TRUE); finish_network_callback (upload_task); return; @@ -834,13 +839,16 @@ handle_network_monitor_can_reach (GNetworkMonitor *network_monitor, static GSocketConnectable * get_ping_socket (EmerDaemon *self) { + g_autofree gchar *server_url = + emer_permissions_provider_get_server_url (self->permissions_provider); GError *error = NULL; GSocketConnectable *ping_socket = - g_network_address_parse_uri (self->server_url, 443 /* SSL default port */, + g_network_address_parse_uri (server_url, + 443 /* SSL default port */, &error); if (ping_socket == NULL) g_error ("Invalid server URL '%s' could not be parsed because: %s.", - self->server_url, error->message); + server_url, error->message); return ping_socket; } @@ -888,8 +896,7 @@ upload_permitted (EmerDaemon *self, } static void -dequeue_and_do_upload (EmerDaemon *self, - const gchar *environment) +dequeue_and_do_upload (EmerDaemon *self) { GError *error = NULL; if (!upload_permitted (self, &error)) @@ -901,16 +908,6 @@ dequeue_and_do_upload (EmerDaemon *self, return; } - g_assert (self->server_url != NULL); - if (strstr (self->server_url, "${environment}") != NULL) - { - GString *s = g_string_new (self->server_url); - g_string_replace (s, "${environment}", environment, 0); - - g_free (self->server_url); - self->server_url = g_string_free (s, FALSE); - } - GNetworkMonitor *network_monitor = g_network_monitor_get_default (); GSocketConnectable *ping_socket = get_ping_socket (self); g_network_monitor_can_reach_async (network_monitor, ping_socket, NULL, @@ -922,7 +919,6 @@ dequeue_and_do_upload (EmerDaemon *self, static void upload_events (EmerDaemon *self, gsize max_upload_size, - const gchar *environment, GAsyncReadyCallback callback, gpointer user_data) { @@ -934,7 +930,7 @@ upload_events (EmerDaemon *self, g_task_set_task_data (upload_task, callback_data, (GDestroyNotify) network_callback_data_free); g_queue_push_tail (self->upload_queue, upload_task); - dequeue_and_do_upload (self, environment); + dequeue_and_do_upload (self); } static void @@ -962,7 +958,7 @@ handle_upload_timer (EmerDaemon *self) gchar *environment = emer_permissions_provider_get_environment (self->permissions_provider); schedule_upload (self, environment); - upload_events (self, MAX_REQUEST_PAYLOAD, environment, + upload_events (self, MAX_REQUEST_PAYLOAD, (GAsyncReadyCallback) log_upload_error, NULL /* user_data */); g_free (environment); @@ -975,10 +971,7 @@ handle_upload_finished (EmerDaemon *self) if (g_queue_is_empty (self->upload_queue)) return; - gchar *environment = - emer_permissions_provider_get_environment (self->permissions_provider); - dequeue_and_do_upload (self, environment); - g_free (environment); + dequeue_and_do_upload (self); } static void @@ -1382,8 +1375,6 @@ emer_daemon_constructed (GObject *object) } buffer_past_aggregate_events (self); - self->server_url = emer_permissions_provider_get_server_url (self->permissions_provider); - gchar *environment = emer_permissions_provider_get_environment (self->permissions_provider); schedule_upload (self, environment); @@ -1490,7 +1481,6 @@ emer_daemon_finalize (GObject *object) g_clear_pointer (&self->variant_array, g_ptr_array_unref); g_rand_free (self->rand); - g_clear_pointer (&self->server_url, g_free); g_clear_object (&self->permissions_provider); g_clear_object (&self->aggregate_tally); g_clear_pointer (&self->persistent_cache_directory, g_free); @@ -1794,11 +1784,7 @@ emer_daemon_upload_events (EmerDaemon *self, GAsyncReadyCallback callback, gpointer user_data) { - - gchar *environment = - emer_permissions_provider_get_environment (self->permissions_provider); - upload_events (self, G_MAXSIZE, environment, callback, user_data); - g_free (environment); + upload_events (self, G_MAXSIZE, callback, user_data); } /* emer_daemon_upload_events_finish: diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index 32b95df..7912ba6 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -647,26 +647,35 @@ emer_permissions_provider_get_environment (EmerPermissionsProvider *self) * emer_permissions_provider_get_server_url: * @self: the permissions provider * - * Gets the metrics server URL from the configuration file. + * Gets the metrics server base URL from the configuration file, with any + * placeholders expanded. * * Returns: The metrics server URL. */ gchar * emer_permissions_provider_get_server_url (EmerPermissionsProvider *self) { - GError *error = NULL; - gchar *server_url = + g_autofree gchar *server_url = NULL; + g_autoptr(GError) error = NULL; + + server_url = g_key_file_get_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_SERVER_URL_KEY_NAME, &error); - if (error != NULL) + if (server_url == NULL) { - g_debug ("Couldn't find key '%s:%s' in permissions config file. " - "Returning default value. Error: %s.", - DAEMON_GLOBAL_GROUP_NAME, DAEMON_SERVER_URL_KEY_NAME, - error->message); - g_error_free (error); - return g_strdup (DEFAULT_METRICS_SERVER_URL); + g_debug ("%s: %s", G_STRFUNC, error->message); + server_url = g_strdup (DEFAULT_METRICS_SERVER_URL); + } + + if (strstr (server_url, "${environment}") != NULL) + { + GString *s = g_string_new (server_url); + g_autofree gchar *environment = + emer_permissions_provider_get_environment (self); + g_string_replace (s, "${environment}", environment, 0); + + return g_string_free (s, FALSE); } - return server_url; + return g_steal_pointer (&server_url); } diff --git a/tests/test-opt-out-integration.py b/tests/test-opt-out-integration.py index 0736ccc..45b43c8 100755 --- a/tests/test-opt-out-integration.py +++ b/tests/test-opt-out-integration.py @@ -156,17 +156,15 @@ def test_upload_doesnt_change_config(self): EmerPermissionsProvider:uploading-enabled so caused that property to be set to TRUE. """ - # Check defaults look good and erase the file before our next change - self._check_config_file(enabled="true", uploading_enabled="false") - - with self.assertRaisesRegex( - dbus.exceptions.DBusException, r"uploading is disabled" - ) as context: - self.interface.UploadEvents() - self.assertEqual( - context.exception.get_dbus_name(), - "com.endlessm.Metrics.Error.UploadingDisabled", - ) + for _ in range(2): + with self.assertRaisesRegex( + dbus.exceptions.DBusException, r"uploading is disabled" + ) as context: + self.interface.UploadEvents() + self.assertEqual( + context.exception.get_dbus_name(), + "com.endlessm.Metrics.Error.UploadingDisabled", + ) self._check_config_file(enabled="true", uploading_enabled="false") @@ -262,7 +260,7 @@ def _check_config_file(self, enabled, uploading_enabled): self.assertEqual(config.get("global", "uploading_enabled"), uploading_enabled) # erase the file after reading it to guarantee that the next time it - # exists, it's up to date. the daemon doesn't read it once started. + # exists, it's up to date. os.unlink(self.config_file) From 38591383e94c4493056f6ad30915ec2537660c0a Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 16:20:59 +0000 Subject: [PATCH 08/15] tests: Use constant strings, not macros Hiding these multi-line strings in macros made them harder to read & edit, for no benefit. --- tests/daemon/test-permissions-provider.c | 93 ++++++++++++------------ 1 file changed, 47 insertions(+), 46 deletions(-) diff --git a/tests/daemon/test-permissions-provider.c b/tests/daemon/test-permissions-provider.c index e5b44c2..aef391e 100644 --- a/tests/daemon/test-permissions-provider.c +++ b/tests/daemon/test-permissions-provider.c @@ -28,52 +28,53 @@ #include #include -#define PERMISSIONS_CONFIG_FILE_ENABLED_TEST \ - "[global]\n" \ - "enabled=true\n" \ - "uploading_enabled=true\n" \ - "environment=test" -#define PERMISSIONS_CONFIG_FILE_DISABLED_TEST \ - "[global]\n" \ - "enabled=false\n" \ - "uploading_enabled=true\n" \ - "environment=test" -#define PERMISSIONS_CONFIG_FILE_UPLOADING_DISABLED_TEST \ - "[global]\n" \ - "enabled=true\n" \ - "uploading_enabled=false\n" \ - "environment=test" -#define PERMISSIONS_CONFIG_FILE_INVALID \ - "lavubeu;f'w943ty[jdn;fbl\n" -#define PERMISSIONS_CONFIG_FILE_ENABLED_DEV \ - "[global]\n" \ - "enabled=true\n" \ - "uploading_enabled=true\n" \ - "environment=dev" -#define PERMISSIONS_CONFIG_FILE_ENABLED_PRODUCTION \ - "[global]\n" \ - "enabled=true\n" \ - "uploading_enabled=true\n" \ - "environment=production" -#define PERMISSIONS_CONFIG_FILE_ENABLED_INVALID_ENVIRONMENT \ - "[global]\n" \ - "enabled=true\n" \ - "uploading_enabled=true\n" \ - "environment=invalid" -#define OSTREE_CONFIG_FILE_STAGING_URL \ - "[core]\n" \ - "repo_version=1\n" \ - "mode=bare\n\n" \ - "[remote \"eos\"]\n" \ - "url=http://fakeurl.with/staging/in/path\n" \ - "branches=master/i386;" -#define OSTREE_CONFIG_FILE_NON_STAGING_URL \ - "[core]\n" \ - "repo_version=1\n" \ - "mode=bare\n\n" \ - "[remote \"eos\"]\n" \ - "url=http://fakeurl.without/term/in/path\n" \ - "branches=master/i386;" +const char *PERMISSIONS_CONFIG_FILE_ENABLED_TEST = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=test"; +const char *PERMISSIONS_CONFIG_FILE_DISABLED_TEST = + "[global]\n" + "enabled=false\n" + "uploading_enabled=true\n" + "environment=test"; +const char *PERMISSIONS_CONFIG_FILE_UPLOADING_DISABLED_TEST = + "[global]\n" + "enabled=true\n" + "uploading_enabled=false\n" + "environment=test"; +const char *PERMISSIONS_CONFIG_FILE_INVALID = + "lavubeu;f'w943ty[jdn;fbl\n"; +const char *PERMISSIONS_CONFIG_FILE_ENABLED_DEV = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=dev"; +const char *PERMISSIONS_CONFIG_FILE_ENABLED_PRODUCTION = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=production"; +const char *PERMISSIONS_CONFIG_FILE_ENABLED_INVALID_ENVIRONMENT = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=invalid"; + +const char *OSTREE_CONFIG_FILE_STAGING_URL = + "[core]\n" + "repo_version=1\n" + "mode=bare\n\n" + "[remote \"eos\"]\n" + "url=http://fakeurl.with/staging/in/path\n" + "branches=master/i386;"; +const char *OSTREE_CONFIG_FILE_NON_STAGING_URL = + "[core]\n" + "repo_version=1\n" + "mode=bare\n\n" + "[remote \"eos\"]\n" + "url=http://fakeurl.without/term/in/path\n" + "branches=master/i386;"; typedef struct { GFile *permissions_config_file; From abb87e8e006efe64e69a402f954748b8ed43531f Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 16:20:59 +0000 Subject: [PATCH 09/15] Test expanding server URL template --- tests/daemon/test-permissions-provider.c | 64 ++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/daemon/test-permissions-provider.c b/tests/daemon/test-permissions-provider.c index aef391e..aa00ba0 100644 --- a/tests/daemon/test-permissions-provider.c +++ b/tests/daemon/test-permissions-provider.c @@ -60,6 +60,18 @@ const char *PERMISSIONS_CONFIG_FILE_ENABLED_INVALID_ENVIRONMENT = "enabled=true\n" "uploading_enabled=true\n" "environment=invalid"; +const char *PERMISSIONS_CONFIG_FILE_WITH_TEMPLATE_URL = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=dev\n" + "server_url=https://${environment}.example.com/"; +const char *PERMISSIONS_CONFIG_FILE_WITH_PLAIN_URL = + "[global]\n" + "enabled=true\n" + "uploading_enabled=true\n" + "environment=dev\n" + "server_url=https://example.com/"; const char *OSTREE_CONFIG_FILE_STAGING_URL = "[core]\n" @@ -521,6 +533,46 @@ test_permissions_provider_set_uploading_enabled_updates_config_file (Fixture "^uploading_enabled=false$")); } +static void +test_permissions_provider_returns_valid_default_url (Fixture *fixture, + gconstpointer unused) +{ + g_autofree gchar *url = NULL; + g_autoptr(GUri) gellar = NULL; + g_autoptr(GError) error = NULL; + + url = emer_permissions_provider_get_server_url (fixture->test_object); + g_assert_nonnull (url); + + /* We can't make any specific assertion about the URL because it is a + * build-time parameter. But we can verify that that build-time parameter + * is a valid URL. + */ + gellar = g_uri_parse (url, G_URI_FLAGS_NONE, &error); + g_assert_no_error (error); + +} + +static void +test_permissions_provider_expands_url (Fixture *fixture, + gconstpointer unused) +{ + g_autofree gchar *url = NULL; + + url = emer_permissions_provider_get_server_url (fixture->test_object); + g_assert_cmpstr (url, ==, "https://dev.example.com/"); +} + +static void +test_permissions_provider_handles_plain_url (Fixture *fixture, + gconstpointer unused) +{ + g_autofree gchar *url = NULL; + + url = emer_permissions_provider_get_server_url (fixture->test_object); + g_assert_cmpstr (url, ==, "https://example.com/"); +} + gint main (gint argc, const gchar * const argv[]) @@ -609,6 +661,18 @@ main (gint argc, PERMISSIONS_CONFIG_FILE_ENABLED_TEST, setup_with_config_file, test_permissions_provider_set_uploading_enabled_updates_config_file); + ADD_PERMISSIONS_PROVIDER_TEST ("/permissions-provider/get-server-url/returns-valid-default-url", + PERMISSIONS_CONFIG_FILE_ENABLED_PRODUCTION, + setup_with_config_file, + test_permissions_provider_returns_valid_default_url); + ADD_PERMISSIONS_PROVIDER_TEST ("/permissions-provider/get-server-url/expands-template", + PERMISSIONS_CONFIG_FILE_WITH_TEMPLATE_URL, + setup_with_config_file, + test_permissions_provider_expands_url); + ADD_PERMISSIONS_PROVIDER_TEST ("/permissions-provider/get-server-url/handles-plain-url", + PERMISSIONS_CONFIG_FILE_WITH_PLAIN_URL, + setup_with_config_file, + test_permissions_provider_handles_plain_url); #undef ADD_PERMISSIONS_PROVIDER_TEST From 4664ae3e72b8d488c2164d8a3bf855eb84dc9f7f Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 15 Jan 2024 16:56:42 +0000 Subject: [PATCH 10/15] permission-provider: Use g_auto* https://phabricator.endlessm.com/T35165 --- daemon/emer-permissions-provider.c | 42 +++++++++--------------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index 7912ba6..ecee7b0 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -92,19 +92,16 @@ load_fallback_data (EmerPermissionsProvider *self) static void write_config_file_sync (EmerPermissionsProvider *self) { - gchar *permissions_config_file_path = + g_autofree gchar *permissions_config_file_path = g_file_get_path (self->permissions_config_file); - GError *error = NULL; + g_autoptr(GError) error = NULL; if (!g_key_file_save_to_file (self->permissions, permissions_config_file_path, &error)) { g_critical ("Could not write to permissions config file '%s'. Error: %s.", permissions_config_file_path, error->message); - g_clear_error (&error); } - - g_free (permissions_config_file_path); } static gboolean @@ -137,9 +134,9 @@ default. Also emits a property notification. */ static void read_config_file_sync (EmerPermissionsProvider *self) { - GError *error = NULL; + g_autoptr(GError) error = NULL; - gchar *path = g_file_get_path (self->permissions_config_file); + const gchar *path = g_file_peek_path (self->permissions_config_file); gboolean load_succeeded = g_key_file_load_from_file (self->permissions, path, G_KEY_FILE_NONE, &error); if (!load_succeeded) @@ -157,8 +154,6 @@ read_config_file_sync (EmerPermissionsProvider *self) g_clear_error (&error); } - g_free (path); - GParamSpec *daemon_enabled_pspec = emer_permissions_provider_props[PROP_DAEMON_ENABLED]; g_object_notify_by_pspec (G_OBJECT (self), daemon_enabled_pspec); @@ -175,7 +170,7 @@ read_config_file_sync (EmerPermissionsProvider *self) static gchar * get_ostree_url_from_file (EmerPermissionsProvider *self) { - gchar *path = g_file_get_path (self->ostree_config_file); + const gchar *path = g_file_peek_path (self->ostree_config_file); if (path == NULL) { @@ -183,31 +178,24 @@ get_ostree_url_from_file (EmerPermissionsProvider *self) return NULL; } - GKeyFile *ostree_configuration_key_file = g_key_file_new (); - GError *error = NULL; + g_autoptr(GKeyFile) ostree_configuration_key_file = g_key_file_new (); + g_autoptr(GError) error = NULL; if (!g_key_file_load_from_file (ostree_configuration_key_file, path, G_KEY_FILE_NONE, &error)) { g_warning ("Unable to load OSTree GKeyFile from given OSTree config " "file path %s. Error: %s.", path, error->message); - g_free (path); - g_clear_error (&error); return NULL; } - g_free (path); - gchar *ostree_url = g_key_file_get_value (ostree_configuration_key_file, "remote \"eos\"", "url", &error); - g_key_file_unref (ostree_configuration_key_file); - if (ostree_url == NULL) { g_warning ("Unable to read OSTree URL from given OSTree config file. " "Error: %s.", error->message); - g_clear_error (&error); } return ostree_url; @@ -217,15 +205,14 @@ static gchar * get_ostree_url_from_ostree_repo (void) { OstreeRepo *ostree_repo = ostree_repo_new_default (); + g_autoptr(GError) error = NULL; - GError *error = NULL; if (!ostree_repo_open (ostree_repo, NULL /* GCancellable */, &error)) { /* Don't warn if simply not on an OSTree system. */ if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) g_warning ("Unable to open OSTree repo: %s", error->message); - g_clear_error (&error); g_object_unref (ostree_repo); return NULL; } @@ -248,7 +235,6 @@ get_ostree_url_from_ostree_repo (void) if (ostree_url == NULL) { g_warning ("Unable to read OSTree URL. Error: %s.", error->message); - g_clear_error (&error); } return ostree_url; @@ -266,7 +252,7 @@ read_ostree_url (EmerPermissionsProvider *self) static gchar * read_environment (EmerPermissionsProvider *self) { - GError *error = NULL; + g_autoptr(GError) error = NULL; gchar *environment = g_key_file_get_value (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENVIRONMENT_KEY_NAME, @@ -277,7 +263,6 @@ read_environment (EmerPermissionsProvider *self) "Returning default value. Error: %s.", DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENVIRONMENT_KEY_NAME, error->message); - g_error_free (error); } if (g_strcmp0 (environment, "dev") != 0 && @@ -511,7 +496,7 @@ emer_permissions_provider_new_full (const gchar *permissions_config_file_path, gboolean emer_permissions_provider_get_daemon_enabled (EmerPermissionsProvider *self) { - GError *error = NULL; + g_autoptr(GError) error = NULL; gboolean daemon_enabled = g_key_file_get_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENABLED_KEY_NAME, &error); @@ -521,7 +506,6 @@ emer_permissions_provider_get_daemon_enabled (EmerPermissionsProvider *self) "Returning default value. Error: %s.", DAEMON_GLOBAL_GROUP_NAME, DAEMON_ENABLED_KEY_NAME, error->message); - g_error_free (error); } return daemon_enabled; @@ -562,7 +546,7 @@ emer_permissions_provider_set_daemon_enabled (EmerPermissionsProvider *self, gboolean emer_permissions_provider_get_uploading_enabled (EmerPermissionsProvider *self) { - GError *error = NULL; + g_autoptr(GError) error = NULL; gboolean uploading_enabled = g_key_file_get_boolean (self->permissions, DAEMON_GLOBAL_GROUP_NAME, DAEMON_UPLOADING_ENABLED_KEY_NAME, &error); @@ -572,7 +556,6 @@ emer_permissions_provider_get_uploading_enabled (EmerPermissionsProvider *self) "Returning default value. Error: %s.", DAEMON_GLOBAL_GROUP_NAME, DAEMON_UPLOADING_ENABLED_KEY_NAME, error->message); - g_error_free (error); return TRUE; } @@ -619,7 +602,7 @@ emer_permissions_provider_get_environment (EmerPermissionsProvider *self) read_config_file_sync (self); gchar *environment = read_environment (self); - gchar *ostree_url = read_ostree_url (self); + g_autofree gchar *ostree_url = read_ostree_url (self); if (ostree_url == NULL) { @@ -639,7 +622,6 @@ emer_permissions_provider_get_environment (EmerPermissionsProvider *self) set_environment (self, environment); } - g_clear_pointer (&ostree_url, g_free); return environment; } From 03e6d07e0c3d88e43eb45ed071b5a670a3f3a8b3 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Tue, 16 Jan 2024 13:49:24 +0000 Subject: [PATCH 11/15] permissions-provider: Include server_url in default config Under some circumstances (namely when the on-disk config is missing or unparseably malformed) the default config will be written back to disk. Personally I think this is pretty weird, but if we're going to do this, we should include all the keys from the default file. https://phabricator.endlessm.com/T35165 --- daemon/emer-permissions-provider.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/daemon/emer-permissions-provider.c b/daemon/emer-permissions-provider.c index ecee7b0..fc277b3 100644 --- a/daemon/emer-permissions-provider.c +++ b/daemon/emer-permissions-provider.c @@ -60,7 +60,8 @@ G_DEFINE_TYPE (EmerPermissionsProvider, emer_permissions_provider, G_TYPE_OBJECT "[" DAEMON_GLOBAL_GROUP_NAME "]\n" \ DAEMON_ENABLED_KEY_NAME "=true\n" \ DAEMON_UPLOADING_ENABLED_KEY_NAME "=false\n" \ - DAEMON_ENVIRONMENT_KEY_NAME "=production\n" + DAEMON_ENVIRONMENT_KEY_NAME "=production\n" \ + DAEMON_SERVER_URL_KEY_NAME "=" DEFAULT_METRICS_SERVER_URL "\n" enum { From 5228de428a50fa97d77473f7fb0260d685311577 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Tue, 16 Jan 2024 13:53:56 +0000 Subject: [PATCH 12/15] meson: Use ExternalProgram.full_path() Meson 0.55 deprecated ExternalProgram.path() in favour of a new function, ExternalProgram.full_path(). The two functions are identical in behaviour, but Meson logs a "future-deprecated" warning if you use .path(). The renaming is not explained in the deprecation message or documentation, but appears to be solely for consistency with other objects. Depend on a new-enough Meson for the new .full_path() method, and use it. https://phabricator.endlessm.com/T35165 --- debian/control | 2 +- meson.build | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/debian/control b/debian/control index c7f6e71..08e2252 100644 --- a/debian/control +++ b/debian/control @@ -8,7 +8,7 @@ Build-Depends: ca-certificates, libostree-dev (>= 2013.7), libpolkit-gobject-1-dev, libsoup-3.0-dev, - meson (>= 0.52), + meson (>= 0.55), pkg-config, python3-dbus, python3-dbusmock, diff --git a/meson.build b/meson.build index 52929d9..05b3350 100644 --- a/meson.build +++ b/meson.build @@ -3,7 +3,7 @@ project('eos-event-recorder-daemon', license: 'GPL-2.0-or-later', version: '0.0.0', default_options: ['c_std=c11', 'warning_level=2'], - meson_version: '>= 0.52.0', + meson_version: '>= 0.55', ) cc = meson.get_compiler('c') @@ -89,10 +89,7 @@ if valgrind.found() # --trace-children=yes because that test also spawns other things such as # a dbus-daemon and we don't want to valgrind those. env: { - 'EXE_WRAPPER': ' '.join( - # TODO: use .full_path() once we require meson ≥ 0.55 - [valgrind.path()] + valgrind_arguments - ), + 'EXE_WRAPPER': ' '.join([valgrind.full_path()] + valgrind_arguments), }, timeout_multiplier: 10, ) From af674f51d810af62cf28f2157044c40e03986f1a Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Tue, 16 Jan 2024 14:01:06 +0000 Subject: [PATCH 13/15] ci: Print error logs from tests https://phabricator.endlessm.com/T35165 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 313972d..f524520 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,4 +40,4 @@ jobs: - name: Build run: ninja -C _build - name: Run tests - run: meson test -C _build + run: meson test -C _build --print-errorlogs From 9daaee4343deec9ac4d88bdf9b305edfd7e1ac3e Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Tue, 16 Jan 2024 14:22:57 +0000 Subject: [PATCH 14/15] meson: factor out targeted GLib version https://phabricator.endlessm.com/T35165 --- meson.build | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/meson.build b/meson.build index 05b3350..87999dc 100644 --- a/meson.build +++ b/meson.build @@ -8,14 +8,19 @@ project('eos-event-recorder-daemon', cc = meson.get_compiler('c') -glib_dep = dependency('glib-2.0', version: '>= 2.68') +glib_major_version = '2' +glib_minor_version = '68' +glib_dep_version = '>= @0@.@1@'.format(glib_major_version, glib_minor_version) +glib_version_define = 'GLIB_VERSION_@0@_@1@'.format(glib_major_version, glib_minor_version) + +glib_dep = dependency('glib-2.0', version: glib_dep_version) eosmetrics_dep = dependency('eosmetrics-0', version: '>= 0.2') polkit_gobject_dep = dependency('polkit-gobject-1') emer_shared_required_modules = [ glib_dep, - dependency('gio-unix-2.0', version: '>= 2.46'), - dependency('gobject-2.0', version: '>= 2.58'), + dependency('gio-unix-2.0', version: glib_dep_version), + dependency('gobject-2.0', version: glib_dep_version), dependency('sqlite3'), dependency('uuid'), ] @@ -53,8 +58,8 @@ add_project_arguments( '-DPERMISSIONS_FILE="@0@"'.format(permissions_file), '-DPERSISTENT_CACHE_DIR="@0@"'.format(persistent_cache_dir), '-DSYSCONFDIR="@0@"'.format(get_option('sysconfdir')), - '-DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_68', - '-DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_68', + '-DGLIB_VERSION_MIN_REQUIRED=@0@'.format(glib_version_define), + '-DGLIB_VERSION_MAX_ALLOWED=@0@'.format(glib_version_define), '-Wno-unused-parameter', ], language: 'c', From f0e91f9cb84aeed7bcc93dfbb9b351ea396c142a Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Tue, 16 Jan 2024 14:26:43 +0000 Subject: [PATCH 15/15] daemon: Disconnect signal handler on dispose EmerDaemon listens for change notifications on EmerPermissionsProvider:daemon-enabled. Previously, it never disconnected this signal handler. In normal usage this is not a problem because the EmerPermissionsProvider is not reused for a different EmerDaemon instance. However, in the test suite it is. This manifested itself as non-deterministic segfaults. Add an assertion that makes the failure deterministic. Use g_signal_connect_object() to automatically disconnect the signal handler when the EmerDaemon object is disposed. Depend on GLib 2.74 for the G_CONNECT_DEFAULT constant, which is just a self-documenting way of writing the number 0. https://phabricator.endlessm.com/T35165 --- daemon/emer-daemon.c | 9 +++++++-- debian/control | 2 +- meson.build | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/daemon/emer-daemon.c b/daemon/emer-daemon.c index 87b1312..9cb73ea 100644 --- a/daemon/emer-daemon.c +++ b/daemon/emer-daemon.c @@ -979,6 +979,8 @@ on_permissions_changed (EmerPermissionsProvider *permissions_provider, GParamSpec *pspec, EmerDaemon *self) { + g_assert (EMER_IS_DAEMON (self)); + self->recording_enabled = emer_permissions_provider_get_daemon_enabled (permissions_provider); @@ -1037,8 +1039,11 @@ set_permissions_provider (EmerDaemon *self, else self->permissions_provider = g_object_ref (permissions_provider); - g_signal_connect (self->permissions_provider, "notify::daemon-enabled", - G_CALLBACK (on_permissions_changed), self); + g_signal_connect_object (self->permissions_provider, + "notify::daemon-enabled", + G_CALLBACK (on_permissions_changed), + G_OBJECT (self), + G_CONNECT_DEFAULT); self->recording_enabled = emer_permissions_provider_get_daemon_enabled (self->permissions_provider); } diff --git a/debian/control b/debian/control index 08e2252..92bcb4d 100644 --- a/debian/control +++ b/debian/control @@ -4,7 +4,7 @@ Maintainer: Endless OS Team Build-Depends: ca-certificates, debhelper-compat (= 13), eos-metrics-0-dev (>= 0.2), - libglib2.0-dev (>= 2.68), + libglib2.0-dev (>= 2.74), libostree-dev (>= 2013.7), libpolkit-gobject-1-dev, libsoup-3.0-dev, diff --git a/meson.build b/meson.build index 87999dc..7796de1 100644 --- a/meson.build +++ b/meson.build @@ -9,7 +9,7 @@ project('eos-event-recorder-daemon', cc = meson.get_compiler('c') glib_major_version = '2' -glib_minor_version = '68' +glib_minor_version = '74' glib_dep_version = '>= @0@.@1@'.format(glib_major_version, glib_minor_version) glib_version_define = 'GLIB_VERSION_@0@_@1@'.format(glib_major_version, glib_minor_version)