Merge pull request #10524 from jdgleaver/config-write-on-change

Only write config files to disk when parameters change

configuration.c

@@ -4248,6 +4248,7 @@ bool config_save_overrides(enum override_type type, void *data)
    free(override_directory);
    free(core_path);
    free(game_path);
+   free(content_path);
 
    return ret;
 }

core_info.c

@@ -140,15 +140,17 @@ static void core_info_list_resolve_all_firmware(
          snprintf(desc_key, sizeof(desc_key), "firmware%u_desc", c);
          snprintf(opt_key,  sizeof(opt_key),  "firmware%u_opt",  c);
 
-         if (config_get_string(config, path_key, &tmp) && !string_is_empty(tmp))
+         if (config_get_string(config, path_key, &tmp))
          {
-            info->firmware[c].path = strdup(tmp);
+            if (!string_is_empty(tmp))
+               info->firmware[c].path = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
-         if (config_get_string(config, desc_key, &tmp) && !string_is_empty(tmp))
+         if (config_get_string(config, desc_key, &tmp))
          {
-            info->firmware[c].desc = strdup(tmp);
+            if (!string_is_empty(tmp))
+               info->firmware[c].desc = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
@@ -315,48 +317,48 @@ static core_info_list_t *core_info_list_new(const char *path,
       {
          char *tmp           = NULL;
 
-         if (config_get_string(conf, "display_name", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "display_name", &tmp))
          {
-            core_info[i].display_name = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].display_name = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
-         if (config_get_string(conf, "display_version", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "display_version", &tmp))
          {
-            core_info[i].display_version = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].display_version = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
-         if (config_get_string(conf, "corename", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "corename", &tmp))
          {
-            core_info[i].core_name = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].core_name = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "systemname", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "systemname", &tmp))
          {
-            core_info[i].systemname = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].systemname = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "systemid", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "systemid", &tmp))
          {
-            core_info[i].system_id = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].system_id = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "manufacturer", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "manufacturer", &tmp))
          {
-            core_info[i].system_manufacturer = strdup(tmp);
+            if (!string_is_empty(tmp))
+               core_info[i].system_manufacturer = strdup(tmp);
             free(tmp);
             tmp = NULL;
          }
@@ -367,87 +369,103 @@ static core_info_list_t *core_info_list_new(const char *path,
             core_info[i].firmware_count = count;
          }
 
-         if (config_get_string(conf, "supported_extensions", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "supported_extensions", &tmp))
          {
-            core_info[i].supported_extensions      = strdup(tmp);
-            core_info[i].supported_extensions_list =
-               string_split(core_info[i].supported_extensions, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].supported_extensions      = strdup(tmp);
+               core_info[i].supported_extensions_list =
+                  string_split(core_info[i].supported_extensions, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "authors", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "authors", &tmp))
          {
-            core_info[i].authors      = strdup(tmp);
-            core_info[i].authors_list =
-               string_split(core_info[i].authors, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].authors      = strdup(tmp);
+               core_info[i].authors_list =
+                  string_split(core_info[i].authors, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "permissions", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "permissions", &tmp))
          {
-            core_info[i].permissions      = strdup(tmp);
-            core_info[i].permissions_list =
-               string_split(core_info[i].permissions, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].permissions      = strdup(tmp);
+               core_info[i].permissions_list =
+                  string_split(core_info[i].permissions, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "license", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "license", &tmp))
          {
-            core_info[i].licenses      = strdup(tmp);
-            core_info[i].licenses_list =
-               string_split(core_info[i].licenses, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].licenses      = strdup(tmp);
+               core_info[i].licenses_list =
+                  string_split(core_info[i].licenses, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "categories", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "categories", &tmp))
          {
-            core_info[i].categories      = strdup(tmp);
-            core_info[i].categories_list =
-               string_split(core_info[i].categories, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].categories      = strdup(tmp);
+               core_info[i].categories_list =
+                  string_split(core_info[i].categories, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "database", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "database", &tmp))
          {
-            core_info[i].databases      = strdup(tmp);
-            core_info[i].databases_list =
-               string_split(core_info[i].databases, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].databases      = strdup(tmp);
+               core_info[i].databases_list =
+                  string_split(core_info[i].databases, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "notes", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "notes", &tmp))
          {
-            core_info[i].notes     = strdup(tmp);
-            core_info[i].note_list = string_split(core_info[i].notes, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].notes     = strdup(tmp);
+               core_info[i].note_list = string_split(core_info[i].notes, "|");
+            }
 
             free(tmp);
             tmp = NULL;
          }
 
-         if (config_get_string(conf, "required_hw_api", &tmp)
-               && !string_is_empty(tmp))
+         if (config_get_string(conf, "required_hw_api", &tmp))
          {
-            core_info[i].required_hw_api = strdup(tmp);
-            core_info[i].required_hw_api_list = string_split(core_info[i].required_hw_api, "|");
+            if (!string_is_empty(tmp))
+            {
+               core_info[i].required_hw_api = strdup(tmp);
+               core_info[i].required_hw_api_list = string_split(core_info[i].required_hw_api, "|");
+            }
 
             free(tmp);
             tmp = NULL;

gfx/video_shader_parse.c

@@ -643,6 +643,10 @@ bool video_shader_write_preset(const char *path,
       config_file_t *conf;
       bool ret;
 
+      /* Note: We always create a new/blank config
+       * file here. Loading and updating an existing
+       * file could leave us with unwanted/invalid
+       * parameters. */
       if (!(conf = config_file_new_alloc()))
          return false;
 

libretro-common/file/config_file.c

@@ -74,7 +74,17 @@ static config_file_t *config_file_new_internal(
 static int config_sort_compare_func(struct config_entry_list *a,
       struct config_entry_list *b)
 {
-   return (a && b) ? strcasecmp(a->key, b->key) : 0;
+   if (a && b)
+   {
+      if (a->key && b->key)
+         return strcasecmp(a->key, b->key);
+      else if (a->key)
+         return 1;
+      else if (b->key)
+         return -1;
+   }
+
+   return 0;
 }
 
 /* https://stackoverflow.com/questions/7685/merge-sort-a-linked-list */
@@ -206,12 +216,19 @@ static char *extract_value(char *line, bool is_value)
    while (isspace((int)*line))
       line++;
 
+   /* Note: From this point on, an empty value
+    * string is valid - and in this case, strdup("")
+    * will be returned
+    * > If we instead return NULL, the the entry
+    *   is ignored completely - which means we cannot
+    *   track *changes* in entry value */
+
    /* We have a full string. Read until next ". */
    if (*line == '"')
    {
       line++;
       if (*line == '"')
-         return NULL;
+         return strdup("");
       tok = strtok_r(line, "\"", &save);
    }
    /* We don't have that. Read until next space. */
@@ -220,7 +237,7 @@ static char *extract_value(char *line, bool is_value)
 
    if (tok && *tok)
       return strdup(tok);
-   return NULL;
+   return strdup("");
 }
 
 /* Move semantics? */
@@ -342,7 +359,7 @@ static bool parse_line(config_file_t *conf,
          char *include_line = comment + STRLEN_CONST("include ");
          char *path         = extract_value(include_line, false);
 
-         if (!path)
+         if (string_is_empty(path))
             return false;
 
          if (conf->include_depth >= MAX_INCLUDE_DEPTH)
@@ -548,7 +565,8 @@ config_file_t *config_file_new_from_string(const char *from_string,
    conf->last                     = NULL;
    conf->includes                 = NULL;
    conf->include_depth            = 0;
-   conf->guaranteed_no_duplicates = false ;
+   conf->guaranteed_no_duplicates = false;
+   conf->modified                 = false;
 
    if (!string_is_empty(path))
       conf->path                  = strdup(path);
@@ -640,7 +658,8 @@ config_file_t *config_file_new_alloc(void)
    conf->last                     = NULL;
    conf->includes                 = NULL;
    conf->include_depth            = 0;
-   conf->guaranteed_no_duplicates = false ;
+   conf->guaranteed_no_duplicates = false;
+   conf->modified                 = false;
 
    return conf;
 }
@@ -804,7 +823,7 @@ bool config_get_string(config_file_t *conf, const char *key, char **str)
 {
    const struct config_entry_list *entry = config_get_entry(conf, key, NULL);
 
-   if (!entry)
+   if (!entry || !entry->value)
       return false;
 
    *str = strdup(entry->value);
@@ -869,20 +888,44 @@ bool config_get_bool(config_file_t *conf, const char *key, bool *in)
 
 void config_set_string(config_file_t *conf, const char *key, const char *val)
 {
-   struct config_entry_list *last  = (conf->guaranteed_no_duplicates && conf->last) ? conf->last : conf->entries;
-   struct config_entry_list *entry = conf->guaranteed_no_duplicates?NULL:config_get_entry(conf, key, &last);
+   struct config_entry_list *last  = NULL;
+   struct config_entry_list *entry = NULL;
 
-   if (entry && !entry->readonly)
+   if (!conf || !key || !val)
+      return;
+
+   last  = (conf->guaranteed_no_duplicates && conf->last) ?
+         conf->last : conf->entries;
+   entry = conf->guaranteed_no_duplicates ?
+         NULL : config_get_entry(conf, key, &last);
+
+   if (entry)
    {
+      /* An entry corresponding to 'key' already exists
+       * > Check if it's read only */
+      if (entry->readonly)
+         return;
+
+      /* Check whether value is currently set */
       if (entry->value)
+      {
+         /* Do nothing if value is unchanged */
+         if (string_is_equal(entry->value, val))
+            return;
+
+         /* Value is to be updated
+          * > Free existing */
          free(entry->value);
-      entry->value = strdup(val);
+      }
+
+      /* Update value */
+      entry->value   = strdup(val);
+      conf->modified = true;
       return;
    }
 
-   if (!val)
-      return;
-
+   /* Entry corresponding to 'key' does not exist
+    * > Create new entry */
    entry = (struct config_entry_list*)malloc(sizeof(*entry));
    if (!entry)
       return;
@@ -891,6 +934,7 @@ void config_set_string(config_file_t *conf, const char *key, const char *val)
    entry->key       = strdup(key);
    entry->value     = strdup(val);
    entry->next      = NULL;
+   conf->modified   = true;
 
    if (last)
       last->next    = entry;
@@ -902,16 +946,27 @@ void config_set_string(config_file_t *conf, const char *key, const char *val)
 
 void config_unset(config_file_t *conf, const char *key)
 {
-   struct config_entry_list *last  = conf->entries;
-   struct config_entry_list *entry = config_get_entry(conf, key, &last);
+   struct config_entry_list *last  = NULL;
+   struct config_entry_list *entry = NULL;
+
+   if (!conf || !key)
+      return;
+
+   last  = conf->entries;
+   entry = config_get_entry(conf, key, &last);
 
    if (!entry)
       return;
 
-   entry->key   = NULL;
-   entry->value = NULL;
-   free(entry->key);
-   free(entry->value);
+   if (entry->key)
+      free(entry->key);
+
+   if (entry->value)
+      free(entry->value);
+
+   entry->key     = NULL;
+   entry->value   = NULL;
+   conf->modified = true;
 }
 
 void config_set_path(config_file_t *conf, const char *entry, const char *val)
@@ -1003,6 +1058,12 @@ void config_set_bool(config_file_t *conf, const char *key, bool val)
 
 bool config_file_write(config_file_t *conf, const char *path, bool sort)
 {
+   if (!conf)
+      return false;
+
+   if (!conf->modified)
+      return true;
+
    if (!string_is_empty(path))
    {
 #ifdef ORBIS
@@ -1030,6 +1091,10 @@ bool config_file_write(config_file_t *conf, const char *path, bool sort)
       if (buf)
          free(buf);
 #endif
+
+      /* Only update modified flag if config file
+       * is actually written to disk */
+      conf->modified = false;
    }
    else
       config_file_dump(conf, stdout, sort);

libretro-common/include/file/config_file.h

@@ -59,6 +59,7 @@ struct config_file
    struct config_entry_list *last;
    unsigned include_depth;
    bool guaranteed_no_duplicates;
+   bool modified;
 
    struct config_include_list *includes;
 };

menu/drivers/materialui.c

@@ -7669,8 +7669,14 @@ static int materialui_pointer_up(void *userdata,
                      menu_navigation_set_selection(ptr);
 
                   /* Perform a MENU_ACTION_SELECT on currently
-                   * active item */
-                  return materialui_menu_entry_action(mui, entry, (size_t)ptr, MENU_ACTION_SELECT);
+                   * active item
+                   * > Note that we still use 'selection'
+                   *   (i.e. old selection value) here. This
+                   *   ensures that materialui_menu_entry_action()
+                   *   registers any change due to the above automatic
+                   *   'pointer item' activation, and thus operates
+                   *   on the correct target entry */
+                  return materialui_menu_entry_action(mui, entry, selection, MENU_ACTION_SELECT);
                }
                else
                {

retroarch.c

@@ -10592,8 +10592,7 @@ error:
  **/
 static void core_option_manager_flush(
       config_file_t *conf,
-      core_option_manager_t *opt,
-      const char *path)
+      core_option_manager_t *opt)
 {
    size_t i;
 
@@ -28649,21 +28648,30 @@ static void retroarch_deinit_core_options(void)
    if (!runloop_core_options)
       return;
 
-   /* check if game options file was just created and flush
-      to that file instead */
+   /* Check whether game-specific options file is being used */
    if (!path_is_empty(RARCH_PATH_CORE_OPTIONS))
    {
+      const char *path        = path_get(RARCH_PATH_CORE_OPTIONS);
+      config_file_t *conf_tmp = NULL;
+
       /* We only need to save configuration settings for
-       * the current core, so create a temporary config_file
-       * object and populate the required values. */
-      config_file_t *conf_tmp = config_file_new_alloc();
+       * the current core
+       * > If game-specific options file exists, have
+       *   to read it (to ensure file only gets written
+       *   if config values change)
+       * > Otherwise, create a new, empty config_file_t
+       *   object */
+      if (path_is_valid(path))
+         conf_tmp = config_file_new_from_path_to_string(path);
+
+      if (!conf_tmp)
+         conf_tmp = config_file_new_alloc();
 
       if (conf_tmp)
       {
-         const char *path = path_get(RARCH_PATH_CORE_OPTIONS);
          core_option_manager_flush(
                conf_tmp,
-               runloop_core_options, path);
+               runloop_core_options);
          RARCH_LOG("[Core Options]: Saved game-specific core options to \"%s\"\n", path);
          config_file_write(conf_tmp, path, true);
          config_file_free(conf_tmp);
@@ -28676,7 +28684,7 @@ static void retroarch_deinit_core_options(void)
       const char *path = runloop_core_options->conf_path;
       core_option_manager_flush(
             runloop_core_options->conf,
-            runloop_core_options, path);
+            runloop_core_options);
       RARCH_LOG("[Core Options]: Saved core options file to \"%s\"\n", path);
       config_file_write(runloop_core_options->conf, path, true);
    }