Re: [AD] Patch to fix race condition

[Peter Wang <novalazy@xxxxxxxxxx>]

On 2010-02-28, Trent Gamblin <trent@xxxxxxxxxx> wrote:
> This is a rather lazy fix for this particular problem...
> Right now loading the system driver loads allegro5.cfg
> which tries which tries to open the file which loads
> the file interface which initializes pthreads which prints
> out a string in debug mode which tries to read a value
> from the partially loaded system configuration. The easy
> fix is I just commented out that ALLEGRO_DEBUG call,
> but perhaps the whole idea is flawed... anyway here's
> a patch.

How about adding an explicit way to load configuration files without
invoking TLS, i.e. add these

    ALLEGRO_FILE *al_fopen_stdio(const char *path, const char *mode)
    ALLEGRO_CONFIG *al_load_config_file_stdio(const char *filename)

Also maybe
al_set_standard_file_interface -> al_set_stdio_file_interface

The following patch is untested.

Peter


diff --git a/docs/src/refman/config.txt b/docs/src/refman/config.txt
index c81e558..e548ebf 100644
--- a/docs/src/refman/config.txt
+++ b/docs/src/refman/config.txt
@@ -23,15 +23,26 @@ See also: [al_create_config], [al_load_config_file]
 
 ## API: al_load_config_file
 
-Read a configuration file from disk.
+Read a configuration file.
 Returns NULL on error.  The configuration structure should be destroyed
 with [al_destroy_config].
 
-See also: [al_save_config_file]
+This function calls [al_fopen] so is affected by the current file interface.
+
+See also: [al_save_config_file], [al_load_config_file_stdio],
+[al_set_new_file_interface]
+
+## API: al_load_config_file_stdio
+
+Like [al_load_config_file] but always tries to open a file on disk rather than
+going through the file interface.  That is, it calls [al_fopen_stdio] instead
+of [al_fopen].  Usually you would want to use [al_load_config_file].
+
+See also: [al_load_config_file], [al_set_new_file_interface]
 
 ## API: al_save_config_file
 
-Write out a configuration file to disk.
+Write out a configuration file.
 Returns true on success, false on error.
 
 See also: [al_load_config_file]
diff --git a/docs/src/refman/file.txt b/docs/src/refman/file.txt
index c2ecc31..3072b2b 100644
--- a/docs/src/refman/file.txt
+++ b/docs/src/refman/file.txt
@@ -55,7 +55,7 @@ Newline translations can be useful for text files but is disastrous for binary
 files.  To avoid this behaviour you need to open file streams in binary mode
 by using a mode argument containing a "b", e.g. "rb", "wb".
 
-See also: [al_set_new_file_interface].
+See also: [al_set_new_file_interface], [al_fopen_stdio].
 
 ## API: al_fclose
 
@@ -286,6 +286,15 @@ native end-of-line sequences, e.g. CR/LF instead of LF.
 
 ## Standard I/O specific routines
 
+## API: al_fopen_stdio
+
+Like [al_fopen] but always tries to open a file on disk rather than going
+through the file interface.  That is, it it unaffected by
+[al_set_new_file_interface].
+Usually you would want to use [al_fopen] instead.
+
+See also: [al_set_new_file_interface], [al_fopen]
+
 ### API: al_fopen_fd
 
 Create an [ALLEGRO_FILE] object that operates on an open file descriptor using
diff --git a/include/allegro5/config.h b/include/allegro5/config.h
index 6d39511..03a2f00 100644
--- a/include/allegro5/config.h
+++ b/include/allegro5/config.h
@@ -15,6 +15,7 @@ AL_FUNC(void, al_set_config_value, (ALLEGRO_CONFIG *config, const char *section,
 AL_FUNC(void, al_add_config_comment, (ALLEGRO_CONFIG *config, const char *section, const char *comment));
 AL_FUNC(const char*, al_get_config_value, (const ALLEGRO_CONFIG *config, const char *section, const char *key));
 AL_FUNC(ALLEGRO_CONFIG*, al_load_config_file, (const char *filename));
+AL_FUNC(ALLEGRO_CONFIG*, al_load_config_file_stdio, (const char *filename));
 AL_FUNC(bool, al_save_config_file, (const ALLEGRO_CONFIG *config, const char *filename));
 AL_FUNC(void, al_merge_config_into, (ALLEGRO_CONFIG *master, const ALLEGRO_CONFIG *add));
 AL_FUNC(ALLEGRO_CONFIG *, al_merge_config, (const ALLEGRO_CONFIG *cfg1, const ALLEGRO_CONFIG *cfg2));
diff --git a/include/allegro5/file.h b/include/allegro5/file.h
index d80d6de..d54fe89 100644
--- a/include/allegro5/file.h
+++ b/include/allegro5/file.h
@@ -76,6 +76,7 @@ AL_FUNC(ALLEGRO_USTR *, al_fget_ustr, (ALLEGRO_FILE *f));
 AL_FUNC(int, al_fputs, (ALLEGRO_FILE *f, const char *p));
 
 /* Specific to stdio backend. */
+AL_FUNC(ALLEGRO_FILE*, al_fopen_stdio, (const char *path, const char *mode));
 AL_FUNC(ALLEGRO_FILE*, al_fopen_fd, (int fd, const char *mode));
 AL_FUNC(ALLEGRO_FILE*, al_make_temp_file, (const char *tmpl,
       ALLEGRO_PATH **ret_path));
diff --git a/src/config.c b/src/config.c
index 79107f9..2b531e6 100644
--- a/src/config.c
+++ b/src/config.c
@@ -329,9 +329,7 @@ static bool readline(ALLEGRO_FILE *file, ALLEGRO_USTR *line)
 }
 
 
-/* Function: al_load_config_file
- */
-ALLEGRO_CONFIG *al_load_config_file(const char *filename)
+static ALLEGRO_CONFIG *load_config_file(ALLEGRO_FILE *file)
 {
    ALLEGRO_CONFIG *config;
    ALLEGRO_CONFIG_SECTION *current_section = NULL;
@@ -339,16 +337,9 @@ ALLEGRO_CONFIG *al_load_config_file(const char *filename)
    ALLEGRO_USTR *section;
    ALLEGRO_USTR *key;
    ALLEGRO_USTR *value;
-   ALLEGRO_FILE *file;
 
-   file = al_fopen(filename, "r");
-   if (!file) {
-      return NULL;
-   }
-   
    config = al_create_config();
    if (!config) {
-      al_fclose(file);
       return NULL;
    }
 
@@ -393,8 +384,42 @@ ALLEGRO_CONFIG *al_load_config_file(const char *filename)
    al_ustr_free(key);
    al_ustr_free(value);
 
+   return config;
+}
+
+
+/* Function: al_load_config_file
+ */
+ALLEGRO_CONFIG *al_load_config_file(const char *filename)
+{
+   ALLEGRO_CONFIG *config;
+   ALLEGRO_FILE *file;
+
+   file = al_fopen(filename, "r");
+   if (!file) {
+      return NULL;
+   }
+
+   config = load_config_file(file);
    al_fclose(file);
+   return config;
+}
+
+
+/* Function: al_load_config_file_stdio
+ */
+ALLEGRO_CONFIG *al_load_config_file_stdio(const char *filename)
+{
+   ALLEGRO_CONFIG *config;
+   ALLEGRO_FILE *file;
 
+   file = al_fopen_stdio(filename, "r");
+   if (!file) {
+      return NULL;
+   }
+
+   config = load_config_file(file);
+   al_fclose(file);
    return config;
 }
 
diff --git a/src/file_stdio.c b/src/file_stdio.c
index 2dd2435..0f727fd 100644
--- a/src/file_stdio.c
+++ b/src/file_stdio.c
@@ -87,7 +87,9 @@ ALLEGRO_FILE *al_fopen_fd(int fd, const char *mode)
 }
 
 
-static ALLEGRO_FILE *file_stdio_fopen(const char *path, const char *mode)
+/* Function: al_fopen_stdio
+ */
+ALLEGRO_FILE *al_fopen_stdio(const char *path, const char *mode)
 {
    FILE *fp;
    ALLEGRO_FILE_STDIO *f;
@@ -256,7 +258,7 @@ Error:
 
 const struct ALLEGRO_FILE_INTERFACE _al_file_interface_stdio =
 {
-   file_stdio_fopen,
+   al_fopen_stdio,
    file_stdio_fclose,
    file_stdio_fread,
    file_stdio_fwrite,
diff --git a/src/system_new.c b/src/system_new.c
index 70e961d..1880b62 100644
--- a/src/system_new.c
+++ b/src/system_new.c
@@ -107,17 +107,20 @@ static void read_allegro_cfg(void)
    /* We cannot use any logging in this function as it would cause the
     * logging system to be initialised before all the relevant config files
     * have been read in.
+    *
+    * We use al_load_config_file_stdio instad of al_load_config_file to avoid
+    * using any TLS while TLS might not be fully set up.
     */
 
 #if defined ALLEGRO_UNIX && !defined ALLEGRO_GP2XWIZ && !defined ALLEGRO_IPHONE
    ALLEGRO_CONFIG *temp;
    ALLEGRO_PATH *path;
 
-   active_sysdrv->config = al_load_config_file("/etc/allegro5rc");
+   active_sysdrv->config = al_load_config_file_stdio("/etc/allegro5rc");
 
    path = _al_unix_get_path(ALLEGRO_USER_HOME_PATH);
    al_set_path_filename(path, "allegro5rc");
-   temp = al_load_config_file(al_path_cstr(path, '/'));
+   temp = al_load_config_file_stdio(al_path_cstr(path, '/'));
    if (temp) {
       if (active_sysdrv->config) {
          al_merge_config_into(active_sysdrv->config, temp);
@@ -129,7 +132,7 @@ static void read_allegro_cfg(void)
    }
    al_free_path(path);
 
-   temp = al_load_config_file("allegro5.cfg");
+   temp = al_load_config_file_stdio("allegro5.cfg");
    if (temp) {
       if (active_sysdrv->config) {
          al_merge_config_into(active_sysdrv->config, temp);
@@ -140,7 +143,7 @@ static void read_allegro_cfg(void)
       }
    }
 #else
-   active_sysdrv->config = al_load_config_file("allegro5.cfg");
+   active_sysdrv->config = al_load_config_file_stdio("allegro5.cfg");
 #endif
 }
 
diff --git a/src/tls.c b/src/tls.c
index 96b3971..055ddf0 100644
--- a/src/tls.c
+++ b/src/tls.c
@@ -754,21 +754,20 @@ ALLEGRO_MEMORY_BLENDER _al_get_memory_blender()
 
 
 /* Function: al_get_new_file_interface
- * FIXME: added a work-around for the situation where TLS has not yet been
- * initialised when this function is called. This may happen if Allegro
- * tries to load a configuration file before the system has been
- * initialised. Should probably rethink the logic here...
  */
 const ALLEGRO_FILE_INTERFACE *al_get_new_file_interface(void)
 {
    thread_local_state *tls;
 
-   if ((tls = tls_get()) == NULL)
+   if ((tls = tls_get()) == NULL) {
       return &_al_file_interface_stdio;
+   }
 
    /* FIXME: this situation should never arise because tls_ has the stdio
     * interface set as a default, but it arises on OS X if
     * pthread_getspecific() is called before pthreads_thread_init()...
+    *
+    * Does this still happen? --pw
     */
    if (tls->new_file_interface)
       return tls->new_file_interface;