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

@@ -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);

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
 

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;

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

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;
 }

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

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 ();
 }