From 38f7483793145067793e740eddfaa5cdd56afb25 Mon Sep 17 00:00:00 2001 From: George Kiagiadakis Date: Fri, 4 Jun 2021 18:22:48 +0300 Subject: [PATCH] state: remove support for groups and propagate save errors There is no real use for groups in our API. Just use the name of the file as the default group and be done with it... Storing multiple groups with this API is problematic because it forces flushing the file to disk multiple times, one for each group, and it's just more performant if we use a prefix in the keys to implement some form of logical separation. This commit also makes the GKeyFile a temporary object. As we always load the file from the file system in _load() and we always replace its contents with a new dictionary in _save(), there is no point in keeping the keyfile's internal data structures stored in memory. Save errors are now also propagated to adhere to the programming practices of GObject --- lib/wp/state.c | 38 ++++++++------- lib/wp/state.h | 4 +- modules/module-default-nodes.c | 17 +++---- modules/module-default-profile.c | 11 ++--- modules/module-lua-scripting/api.c | 16 +++--- src/scripts/default-routes.lua | 10 ++-- tests/wp/state.c | 78 ++++++++---------------------- 7 files changed, 66 insertions(+), 108 deletions(-) diff --git a/lib/wp/state.c b/lib/wp/state.c index 5f39b58b..86a4fc68 100644 --- a/lib/wp/state.c +++ b/lib/wp/state.c @@ -107,7 +107,6 @@ wp_state_finalize (GObject * object) g_clear_pointer (&self->name, g_free); g_clear_pointer (&self->location, g_free); - g_clear_pointer (&self->keyfile, g_key_file_free); G_OBJECT_CLASS (wp_state_parent_class)->finalize (object); } @@ -115,7 +114,6 @@ wp_state_finalize (GObject * object) static void wp_state_init (WpState * self) { - self->keyfile = g_key_file_new (); } static void @@ -194,35 +192,35 @@ wp_state_clear (WpState *self) * \brief Saves new properties in the state, overwriting all previous data. * \ingroup wpstate * \param self the state - * \param group the group name where the properties will be save * \param props (transfer none): the properties to save + * \param error (out)(optional): return location for a GError, or NULL * \returns TRUE if the properties could be saved, FALSE otherwise */ gboolean -wp_state_save (WpState *self, const gchar *group, WpProperties *props) +wp_state_save (WpState *self, WpProperties *props, GError ** error) { + g_autoptr (GKeyFile) keyfile = g_key_file_new (); g_autoptr (WpIterator) it = NULL; g_auto (GValue) item = G_VALUE_INIT; + GError *err = NULL; g_return_val_if_fail (WP_IS_STATE (self), FALSE); - g_return_val_if_fail (group, FALSE); + g_return_val_if_fail (props, FALSE); wp_state_ensure_location (self); wp_info_object (self, "saving state into %s", self->location); - g_key_file_remove_group (self->keyfile, group, NULL); - /* Set the properties */ for (it = wp_properties_new_iterator (props); wp_iterator_next (it, &item); g_value_unset (&item)) { const gchar *key = wp_properties_iterator_item_get_key (&item); const gchar *val = wp_properties_iterator_item_get_value (&item); - g_key_file_set_string (self->keyfile, group, key, val); + g_key_file_set_string (keyfile, self->name, key, val); } - if (!g_key_file_save_to_file (self->keyfile, self->location, NULL)) { - wp_critical_object (self, "can't save %s", self->location); + if (!g_key_file_save_to_file (keyfile, self->location, &err)) { + g_propagate_prefixed_error (error, err, "could not save %s: ", self->name); return FALSE; } @@ -230,36 +228,40 @@ wp_state_save (WpState *self, const gchar *group, WpProperties *props) } /*! - * \brief Loads the state data into new properties. + * \brief Loads the state data from the file system + * + * This function will never fail. If it cannot load the state, for any reason, + * it will simply return an empty WpProperties, behaving as if there was no + * previous state stored. + * * \ingroup wpstate * \param self the state - * \param group the group which the properties will be loaded from - * \returns (transfer full): the new properties with the state data + * \returns (transfer full): a new WpProperties containing the state data */ WpProperties * -wp_state_load (WpState *self, const gchar *group) +wp_state_load (WpState *self) { + g_autoptr (GKeyFile) keyfile = g_key_file_new (); g_autoptr (WpProperties) props = wp_properties_new_empty (); gchar ** keys = NULL; g_return_val_if_fail (WP_IS_STATE (self), NULL); - g_return_val_if_fail (group, NULL); wp_state_ensure_location (self); /* Open */ - if (!g_key_file_load_from_file (self->keyfile, self->location, + if (!g_key_file_load_from_file (keyfile, self->location, G_KEY_FILE_NONE, NULL)) return g_steal_pointer (&props); /* Load all keys */ - keys = g_key_file_get_keys (self->keyfile, group, NULL, NULL); + keys = g_key_file_get_keys (keyfile, self->name, NULL, NULL); if (!keys) return g_steal_pointer (&props); for (guint i = 0; keys[i]; i++) { const gchar *key = keys[i]; g_autofree gchar *val = NULL; - val = g_key_file_get_string (self->keyfile, group, key, NULL); + val = g_key_file_get_string (keyfile, self->name, key, NULL); if (!val) continue; wp_properties_set (props, key, val); diff --git a/lib/wp/state.h b/lib/wp/state.h index edd048cb..1f8e126e 100644 --- a/lib/wp/state.h +++ b/lib/wp/state.h @@ -36,10 +36,10 @@ WP_API void wp_state_clear (WpState *self); WP_API -gboolean wp_state_save (WpState *self, const gchar *group, WpProperties *props); +gboolean wp_state_save (WpState *self, WpProperties *props, GError ** error); WP_API -WpProperties * wp_state_load (WpState *self, const gchar *group); +WpProperties * wp_state_load (WpState *self); G_END_DECLS diff --git a/modules/module-default-nodes.c b/modules/module-default-nodes.c index 37c55b56..010dd64a 100644 --- a/modules/module-default-nodes.c +++ b/modules/module-default-nodes.c @@ -57,14 +57,10 @@ wp_default_nodes_init (WpDefaultNodes * self) static void load_state (WpDefaultNodes * self) { - g_autoptr (WpProperties) props = wp_state_load (self->state, NAME); - if (!props) - wp_warning_object (self, "could not load " NAME); - else { - for (gint i = 0; i < N_DEFAULT_NODES; i++) { - const gchar *value = wp_properties_get (props, DEFAULT_CONFIG_KEY[i]); - self->defaults[i].config_value = g_strdup (value); - } + g_autoptr (WpProperties) props = wp_state_load (self->state); + for (gint i = 0; i < N_DEFAULT_NODES; i++) { + const gchar *value = wp_properties_get (props, DEFAULT_CONFIG_KEY[i]); + self->defaults[i].config_value = g_strdup (value); } } @@ -73,6 +69,7 @@ timeout_save_state_callback (gpointer data) { WpDefaultNodes *self = data; g_autoptr (WpProperties) props = wp_properties_new_empty (); + g_autoptr (GError) error = NULL; for (gint i = 0; i < N_DEFAULT_NODES; i++) { if (self->defaults[i].config_value) @@ -80,8 +77,8 @@ timeout_save_state_callback (gpointer data) self->defaults[i].config_value); } - if (!wp_state_save (self->state, NAME, props)) - wp_warning_object (self, "could not save " NAME); + if (!wp_state_save (self->state, props, &error)) + wp_warning_object (self, "%s", error->message); g_clear_pointer (&self->timeout_source, g_source_unref); return G_SOURCE_REMOVE; diff --git a/modules/module-default-profile.c b/modules/module-default-profile.c index 69b12d64..54f585a6 100644 --- a/modules/module-default-profile.c +++ b/modules/module-default-profile.c @@ -82,9 +82,10 @@ timeout_save_callback (WpDefaultProfile *self) { WpDefaultProfilePrivate *priv = wp_default_profile_get_instance_private (self); + g_autoptr (GError) error = NULL; - if (!wp_state_save (priv->state, "group", priv->profiles)) - wp_warning_object (self, "could not save profiles"); + if (!wp_state_save (priv->state, priv->profiles, &error)) + wp_warning_object (self, "%s", error->message); return G_SOURCE_REMOVE; } @@ -294,11 +295,7 @@ wp_default_profile_init (WpDefaultProfile * self) priv->state = wp_state_new (STATE_NAME); /* Load the saved profiles */ - priv->profiles = wp_state_load (priv->state, "group"); - if (!priv->profiles) { - wp_warning_object (self, "could not load profiles"); - return; - } + priv->profiles = wp_state_load (priv->state); } static void diff --git a/modules/module-lua-scripting/api.c b/modules/module-lua-scripting/api.c index e2732752..0229c52b 100644 --- a/modules/module-lua-scripting/api.c +++ b/modules/module-lua-scripting/api.c @@ -1123,20 +1123,20 @@ static int state_save (lua_State *L) { WpState *state = wplua_checkobject (L, 1, WP_TYPE_STATE); - const gchar *group = luaL_checkstring (L, 2); - luaL_checktype (L, 3, LUA_TTABLE); - g_autoptr (WpProperties) props = wplua_table_to_properties (L, 3); - gboolean ret = wp_state_save (state, group, props); - lua_pushboolean (L, ret); - return 1; + luaL_checktype (L, 2, LUA_TTABLE); + g_autoptr (WpProperties) props = wplua_table_to_properties (L, 2); + g_autoptr (GError) error = NULL; + gboolean saved = wp_state_save (state, props, &error); + lua_pushboolean (L, saved); + lua_pushstring (L, error ? error->message : ""); + return 2; } static int state_load (lua_State *L) { WpState *state = wplua_checkobject (L, 1, WP_TYPE_STATE); - const gchar *group = luaL_checkstring (L, 2); - g_autoptr (WpProperties) props = wp_state_load (state, group); + g_autoptr (WpProperties) props = wp_state_load (state); wplua_properties_to_table (L, props); return 1; } diff --git a/src/scripts/default-routes.lua b/src/scripts/default-routes.lua index 0e41589c..c1a44256 100644 --- a/src/scripts/default-routes.lua +++ b/src/scripts/default-routes.lua @@ -17,9 +17,8 @@ use_persistent_storage = config["use-persistent-storage"] or false dev_infos = {} -- the state storage -state_name = "default-routes" -state = use_persistent_storage and State(state_name) or nil -state_table = state and state:load(state_name) or {} +state = use_persistent_storage and State("default-routes") or nil +state_table = state and state:load() or {} -- simple serializer {"foo", "bar"} -> "foo;bar;" function serializeArray(a) @@ -71,7 +70,10 @@ function storeAfterTimeout() timeout_source:destroy() end timeout_source = Core.timeout_add(1000, function () - state:save(state_name, state_table) + local saved, err = state:save(state_table) + if not saved then + Log.warning(err) + end timeout_source = nil end) end diff --git a/tests/wp/state.c b/tests/wp/state.c index 57915067..eff34eaa 100644 --- a/tests/wp/state.c +++ b/tests/wp/state.c @@ -11,6 +11,7 @@ static void test_state_basic (void) { + g_autoptr (GError) error = NULL; g_autoptr (WpState) state = wp_state_new ("basic"); g_assert_nonnull (state); @@ -23,12 +24,13 @@ test_state_basic (void) wp_properties_set (props, "key1", "value1"); wp_properties_set (props, "key2", "value2"); wp_properties_set (props, "key3", "value3"); - g_assert_true (wp_state_save (state, "group", props)); + g_assert_true (wp_state_save (state, props, &error)); + g_assert_no_error (error); } /* Load */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_cmpstr (wp_properties_get (props, "key1"), ==, "value1"); g_assert_cmpstr (wp_properties_get (props, "key2"), ==, "value2"); @@ -40,12 +42,13 @@ test_state_basic (void) { g_autoptr (WpProperties) props = wp_properties_new_empty (); wp_properties_set (props, "new-key", "new-value"); - g_assert_true (wp_state_save (state, "group", props)); + g_assert_true (wp_state_save (state, props, &error)); + g_assert_no_error (error); } /* Re-Load */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_cmpstr (wp_properties_get (props, "new-key"), ==, "new-value"); g_assert_null (wp_properties_get (props, "key1")); @@ -57,7 +60,7 @@ test_state_basic (void) /* Load empty */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_null (wp_properties_get (props, "new-key")); g_assert_null (wp_properties_get (props, "key1")); @@ -71,6 +74,7 @@ test_state_basic (void) static void test_state_empty (void) { + g_autoptr (GError) error = NULL; g_autoptr (WpState) state = wp_state_new ("empty"); g_assert_nonnull (state); @@ -78,12 +82,13 @@ test_state_empty (void) { g_autoptr (WpProperties) props = wp_properties_new_empty (); wp_properties_set (props, "key", "value"); - g_assert_true (wp_state_save (state, "group", props)); + g_assert_true (wp_state_save (state, props, &error)); + g_assert_no_error (error); } /* Load */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_cmpstr (wp_properties_get (props, "key"), ==, "value"); } @@ -91,12 +96,13 @@ test_state_empty (void) /* Save empty */ { g_autoptr (WpProperties) props = wp_properties_new_empty (); - g_assert_true (wp_state_save (state, "group", props)); + g_assert_true (wp_state_save (state, props, &error)); + g_assert_no_error (error); } /* Load empty */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_null (wp_properties_get (props, "key")); } @@ -107,6 +113,7 @@ test_state_empty (void) static void test_state_spaces (void) { + g_autoptr (GError) error = NULL; g_autoptr (WpState) state = wp_state_new ("spaces"); g_assert_nonnull (state); @@ -114,12 +121,13 @@ test_state_spaces (void) { g_autoptr (WpProperties) props = wp_properties_new_empty (); wp_properties_set (props, "key", "value with spaces"); - g_assert_true (wp_state_save (state, "group", props)); + g_assert_true (wp_state_save (state, props, &error)); + g_assert_no_error (error); } /* Load */ { - g_autoptr (WpProperties) props = wp_state_load (state, "group"); + g_autoptr (WpProperties) props = wp_state_load (state); g_assert_nonnull (props); g_assert_cmpstr (wp_properties_get (props, "key"), ==, "value with spaces"); } @@ -127,53 +135,6 @@ test_state_spaces (void) wp_state_clear (state); } -static void -test_state_group (void) -{ - g_autoptr (WpState) state = wp_state_new ("group"); - g_assert_nonnull (state); - - /* Save 1 */ - { - g_autoptr (WpProperties) props = wp_properties_new_empty (); - wp_properties_set (props, "key1", "value1"); - g_assert_true (wp_state_save (state, "1", props)); - } - - /* Save 2 */ - { - g_autoptr (WpProperties) props = wp_properties_new_empty (); - wp_properties_set (props, "key2", "value2"); - g_assert_true (wp_state_save (state, "2", props)); - } - - /* Load invalid group */ - { - g_autoptr (WpProperties) props = wp_state_load (state, "invalid"); - g_assert_nonnull (props); - g_assert_null (wp_properties_get (props, "key1")); - g_assert_null (wp_properties_get (props, "key2")); - } - - /* Load 1 */ - { - g_autoptr (WpProperties) props = wp_state_load (state, "1"); - g_assert_nonnull (props); - g_assert_cmpstr (wp_properties_get (props, "key1"), ==, "value1"); - g_assert_null (wp_properties_get (props, "key2")); - } - - /* Load 2 */ - { - g_autoptr (WpProperties) props = wp_state_load (state, "2"); - g_assert_nonnull (props); - g_assert_cmpstr (wp_properties_get (props, "key2"), ==, "value2"); - g_assert_null (wp_properties_get (props, "key1")); - } - - wp_state_clear (state); -} - int main (int argc, char *argv[]) { @@ -183,7 +144,6 @@ main (int argc, char *argv[]) g_test_add_func ("/wp/state/basic", test_state_basic); g_test_add_func ("/wp/state/empty", test_state_empty); g_test_add_func ("/wp/state/spaces", test_state_spaces); - g_test_add_func ("/wp/state/group", test_state_group); return g_test_run (); }