Retroarch: (Config) Not reading /etc/retroarch.cfg

[sergiobenrocha2]

RetroArch is not reading /etc/retroarch.cfg in the first run, bad commit: b878a94

See: https://forums.libretro.com/t/retroarch-corrupted-interface-in-linux-mint-19/17238/3

[orbea]

@bparker06 Can you take a look at this?

[orbea]

Reverting the above commit works for now.

diff --git a/libretro-common/file/config_file.c b/libretro-common/file/config_file.c
index 9c2b76b170..15ce4336d1 100644
--- a/libretro-common/file/config_file.c
+++ b/libretro-common/file/config_file.c
@@ -68,84 +68,6 @@ struct config_include_list
 static config_file_t *config_file_new_internal(
       const char *path, unsigned depth);

-static int config_sort_compare_func(struct config_entry_list *a,
-      struct config_entry_list *b)
-{
-   const char *a_key = a ? a->key : NULL;
-   const char *b_key = b ? b->key : NULL;
-
-   if (!a_key || !b_key)
-      return 0;
-
-   return strcasecmp(a_key, b_key);
-}
-
-/* https://stackoverflow.com/questions/7685/merge-sort-a-linked-list */
-static struct config_entry_list* merge_sort_linked_list(struct config_entry_list *list, int (*compare)(struct config_entry_list *one,struct config_entry_list *two))
-{
-   struct config_entry_list
-         *right  = list,
-         *temp   = list,
-         *last   = list,
-         *result = 0,
-         *next   = 0,
-         *tail   = 0;
-
-   /* Trivial case. */
-   if (!list || !list->next)
-      return list;
-
-   /* Find halfway through the list (by running two pointers, one at twice the speed of the other). */
-   while (temp && temp->next)
-   {
-      last = right;
-      right = right->next;
-      temp = temp->next->next;
-   }
-
-   /* Break the list in two. (prev pointers are broken here, but we fix later) */
-   last->next = 0;
-
-   /* Recurse on the two smaller lists: */
-   list = merge_sort_linked_list(list, compare);
-   right = merge_sort_linked_list(right, compare);
-
-   /* Merge: */
-   while (list || right)
-   {
-      /* Take from empty lists, or compare: */
-      if (!right)
-      {
-         next = list;
-         list = list->next;
-      }
-      else if (!list)
-      {
-         next = right;
-         right = right->next;
-      }
-      else if (compare(list, right) < 0)
-      {
-         next = list;
-         list = list->next;
-      }
-      else
-      {
-         next = right;
-         right = right->next;
-      }
-
-      if (!result)
-         result = next;
-      else
-         tail->next = next;
-
-      tail = next;
-   }
-
-   return result;
-}
-
 static char *strip_comment(char *str)
 {
    /* Remove everything after comment.
@@ -998,7 +920,7 @@ void config_file_dump(config_file_t *conf, FILE *file)
       includes = includes->next;
    }

-   list = merge_sort_linked_list((struct config_entry_list*)conf->entries, config_sort_compare_func);
+   list = (struct config_entry_list*)conf->entries;

    while (list)
    {

However this is a critical issue for system wide installs of RetroArch on linux.

[bparker06]

That commit is not related to opening config files, and it still opens /etc/retroarch.cfg for me on latest master:

$ strace -f -s 65535 ./retroarch -v 2>&1 | grep /etc/retroarch.cfg
stat("/etc/retroarch.cfg", 0x7ffdc37ba0d0) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/retroarch.cfg", O_RDONLY) = -1 ENOENT (No such file or directory)

Please provide exact steps to reproduce, along with a similar strace output showing that it is not trying to open the file (make sure ~/.config/retroarch/retroarch.cfg is deleted first).

[orbea]

@bparker06

It seems to not affect all settings and maybe only affects directories, here are some steps you can take to reproduce this.

mv ~/.config/retroarch ~/.config/retroarch.bak

sed -e 's|# audio_filter_dir =|audio_filter_dir = /usr/lib64/retroarch/filters/audio|' \
    -e 's|# video_filter_dir =|video_filter_dir = /usr/lib64/retroarch/filters/video|' \
    -e 's|# libretro_directory =|libretro_directory = /usr/lib64/libretreto|' \
    -e 's|# libretro_info_path =|libretro_info_path = /usr/lib64/libretro/info|' \
    -e 's|# menu_show_core_updater = true|menu_show_core_updater = false|' \
    -i retroarch.cfg

cp retroarch.cfg /etc/retroarch.cfg

Now start RetroArch and check settings > directories, none of the configured directories are correct and they will be the upstream defaults. However the menu_show_core_updater is correctly set to false.

Now try this again, but applying the above patch reverting the problematic commit and it will work again.

[bparker06]

That has nothing to do with reading /etc/retroarch.cfg on first run though... because if you have to modify an existing config file, it's no longer 'first run'. And I'm not sure what existing config you're even trying to modify here. Perhaps this should be a separate issue?

[orbea]

I was not clear, you are not modifying ~/.config/retroarch/retroarch.cfg. You are modifying retroarch.cfg in the RetroArch git repo.

https://github.com/libretro/RetroArch/blob/master/retroarch.cfg

This is the first run because ~/.config/retroarch/ does not exist and only /etc/retroarch.cfg does. On the first run it should source /etc/retroarch.cfg and then apply those settings (Including changed ones) to the new ~/retrorarch/retroarch.cfg which will be created. However now its only applying some settings and not others.

Given linux mint has a similar use case to slackware here and the same commit is implicated I think this is the same issue. Just it was not fully explained in the op. The problem is not that its not reading /etc/retroarch.cfg rather that its reading the file and not respecting all of settings.

[bparker06]

How does modifying that file in the repo help anything though? And where does /etc/retroarch.cfg come from (and what's in it)? I'm just trying to understand the exact setup here that triggers the issue.

[orbea]

It is installed from the RetroArch git repo with make install, but you can place it there manually too to test this. Maybe this helps explain better?

1. Move ~/.config/retroarch/ out of the way so RetroArch does not see it and the next run will be the first run.
2. Modify retroarch.cfg in the RetroArch git repo.
3. Copy retroarch.cfg from the RetroArch git repo to /etc/retroarch.cfg.
4. Now start RetroArch where it will show a little popup indicating its the first run and the issue should reproduce.

[orbea]

Here is my /etc/retroarch.cfg as an example.

https://pastebin.com/f0SQWDss

[bparker06]

Ahh ok that makes more sense. I tried your sed command but it fails:

$ sed -e 's|# audio_filter_dir =|audio_filter_dir = /usr/lib64/retroarch/filters/audio|' \
>     -e 's|# video_filter_dir =|video_filter_dir = /usr/lib64/retroarch/filters/video|' \
>     -e 's|# libretro_directory =|libretro_directory = /usr/lib64/libretreto'| \
>     -e 's|# libretro_info_path =|libretro_info_path = /usr/lib64/libretro/info|' \
>     -e 's|# menu_show_core_updater = true|menu_show_core_updater = false|' \
>     -i retroarch.cfg
bash: -e: command not found
sed: -e expression #3, char 67: unterminated `s' command

I'll just use your pasted config instead.

[bparker06]

Actually your config appears corrupt... there's no quotes around any strings.

[orbea]

I made a typo in that sed command when I posted it and I edited the comment accordingly, but apparently not quickly enough.

My pasted config did work before that change, but maybe I should try adding quotes around the paths anyways... Here is a diff with the default file in master.

--- retroarch.cfg       2018-08-23 22:45:36.733548796 -0700
+++ /etc/retroarch.cfg  2018-08-23 17:07:37.000000000 -0700
@@ -621,7 +621,7 @@
 # menu_show_online_updater = true

 # If disabled, will hide the ability to update cores (and core info files) inside the menu.
-# menu_show_core_updater = true
+menu_show_core_updater = false

 # If disabled, the libretro core will keep running in the background when we
 # are in the menu.
@@ -763,10 +763,10 @@
 # rgui_browser_directory =

 # Core directory for libretro core implementations.
-# libretro_directory =
+libretro_directory = /usr/lib64/libretro

 # Core info directory for libretro core information.
-# libretro_info_path =
+libretro_info_path = /usr/lib64/libretro/info

 # Path to content database directory.
 # content_database_path =
@@ -778,10 +778,10 @@
 # cheat_database_path =

 # Defines a directory where CPU-based video filters are kept.
-# video_filter_dir =
+video_filter_dir = /usr/lib64/retroarch/filters/video

 # Directory where DSP plugins are kept.
-# audio_filter_dir =
+audio_filter_dir = /usr/lib64/retroarch/filters/audio

 # Defines a directory where shaders (Cg, CGP, GLSL) are kept for easy access.
 # video_shader_dir =

[bparker06]

I just tried your patch, adding quotes around the strings, and on first start, the only directory that shows lib64 is video_filter_dir.

[orbea]

Yes, this is the same behavior here. Only video_filter_dir is respected.

[bparker06]

If we just print out the values of one of the bad folders before and after the sorting:

$ ./retroarch -v 2>&1 | grep 'libretro_dir'
libretro_directory before sort: /usr/lib64/libretro
libretro_directory after sort: /usr/lib64/libretro
libretro_directory before sort: ~/.config/retroarch/cores
libretro_directory after sort: ~/.config/retroarch/cores
libretro_directory before sort: ~/.config/retroarch/cores
libretro_directory after sort: ~/.config/retroarch/cores

They're there, which would lead me to believe that the issue is actually somewhere else, even though it doesn't make sense how it worked before this commit. ASAN isn't turning up anything either.

But it is clear that the sort function itself is not removing or altering the entries in any way, something is changing the entry somewhere else entirely.

[bparker06]

It's possible there is code somewhere that is storing a pointer to that particular config entry, which would become invalid if the list is sorted. That could explain this sort operation affecting code elsewhere.

[orbea]

Yea, its an interesting issue and I wasn't clear at all why it was happening.

[kivutar]

This bug is affecting Lakka. After upgrading RA, I'm getting black squares and no ttf font on a fresh install.

Note: Strings without double quotes were perfectly supported before.

[orbea]

@bparker06 Would you be opposed to reverting your commit until a better solution can be found? I think the problems with /etc/retroarch.cfg are greater than the benefit of having a sorted config.

[bparker06]

What about just turning off the sort on first run? That should keep everything working correctly without removing any functionality?

Also, when I use my own /etc/retroarch.cfg that just has libretro_directory in it... the issue does not occur.

[orbea]

What about just turning off the sort on first run? That should keep everything working correctly without removing any functionality?

That sounds like a plausible solution.

Also, when I use my own /etc/retroarch.cfg that just has libretro_directory in it... the issue does not occur.

Maybe only a specific setting is causing the problems, I'll test this more.

[orbea]

@bparker06

Seems to break in two ways.

1. It fails to parse the video_filter_dir and audio_filter_dir when both are changed, if only one of them is changed then it will work.
2. Setting menu_show_core_updater will make it not able to parse libretro_directory and libretro_info_path while it will work fine if that is not set.

Can you confirm this?

[bparker06]

with this:

$ cat /etc/retroarch.cfg
menu_show_core_updater = false
libretro_directory = "/usr/lib64/retroarch/cores"

The core folder is not set... but if I reverse the lines, it works.

[bparker06]

Getting somewhere...

This block:
https://github.com/libretro/RetroArch/blob/master/configuration.c#L2731-L2732
Is returning false, which is what is causing the value in /etc/retroarch.cfg to not be used, now to find out why that's happening.

[bparker06]

I believe this is the offending line:
https://github.com/libretro/RetroArch/blob/master/configuration.c#L2263

Basically, whenever config_file_write() is called, the config entries are sorted, making the pointer that was originally passed to the function (which is really just the first entry in a linked list) invalid. Because the list is now sorted, that pointer now points to somewhere else in the middle of the config file, so subsequent functions (like that config_get_entry() in my previous comment) sometimes cannot find an entry because it started too far down the list.

[orbea]

I think the easiest solution would be to sort after the configuration file is created.

[bparker06]

It's already sorted at the latest possible place though... when it's actually written to disk. You don't want to sort it outside of the config_file interface because then it'd become an RA-specific feature... but I think I found the solution. Can you try this?

diff --git a/libretro-common/file/config_file.c b/libretro-common/file/config_file.c
index 9c2b76b170..79dde18279 100644
--- a/libretro-common/file/config_file.c
+++ b/libretro-common/file/config_file.c
@@ -999,6 +999,7 @@ void config_file_dump(config_file_t *conf, FILE *file)
    }

    list = merge_sort_linked_list((struct config_entry_list*)conf->entries, config_sort_compare_func);
+   conf->entries = list;

    while (list)
    {

The entries pointer just needed to be reset to the new beginning of the list after the sort.

[orbea]

@bparker06 Yes, that works here, thanks! However I don't have the assets issue reported for lakka so I suggest that @kivutar should test it too.

[bparker06]

PR is just waiting for @kivutar then I guess.

[kivutar]

I test it tonight. Generating images now.

[kivutar]

It works with the PR.