Re: [PATCH] Remove unnecessary locking when reading iconv configuration [BZ #22062]

[Florian Weimer <fweimer at redhat dot com>]

On 10/26/2017 01:18 PM, Arjun Shankar wrote:

> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index f1c28ce..d5cf86e 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -423,115 +423,107 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len,
>   void
>   __gconv_get_path (void)

You should add a comment that this function must only be called under 
the libc_once guard from __gconv_load_conf, either here or in the header 
file with the declaration.

>   {
> -  struct path_elem *result;

I think it makes sense to keep the result variable because it can be 
kept in a register across calls.  In contrast, the global variable has 
to be reloaded.

> +/* This "once" variable is used to do a one-time load of the configuration.  */
> +__libc_once_define (static, once);
> +
> +
> +/* Read all configuration files found in the user-specified and the default
> +   path, but do it only "once" using __gconv_read_conf to do the actual
> +   work.  This is the function that must be called when reading iconv
> +   configuration.  */
> +void
> +attribute_hidden
> +__gconv_load_conf (void)
> +{
> +  __libc_once (once, __gconv_read_conf);
> +}
> +

attribute_hidden should only be present on the declaration, not the 
definition.

> diff --git a/iconv/tst-iconv_mt.c b/iconv/tst-iconv_mt.c
> new file mode 100644
> index 0000000..5fc1de4
> --- /dev/null
> +++ b/iconv/tst-iconv_mt.c
> @@ -0,0 +1,146 @@


> +#define WORKER_FAIL(fmt) \
> +  do { printf ("FAIL: thread %lx: " fmt ": %m\n", tidx); \
> +       pthread_exit ((void *) (long int) 1); } while (0)

If you use TEST_VERIFY or THREAD_VERIFY_EXIT, you won't need this.

> +      worker_output = xpthread_join (thread[i]);
> +      if (worker_output != NULL)
> +        retval = 1;

And this code (and the retval variable) can go away, too.

Thanks,
Florian