Re: [AD] Patch to fix race condition

[Thomas Fjellstrom <tfjellstrom@xxxxxxxxxx>]

On February 28, 2010, Peter Wang wrote:
> 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;

I must have missed it, but what exactly is the reason tls can't be setup 
before ANYTHING else is done in al_init?

-- 
Thomas Fjellstrom
tfjellstrom@xxxxxxxxxx