[4/6] iconvconfig: Use common gconv module parsing function

Drop local copy of gconv file parsing and use the one in
gconv_parseconfdir.h instead.  Now there is a single implementation of
configuration file parsing.
---
 iconv/iconvconfig.c | 126 ++++----------------------------------------
 1 file changed, 11 insertions(+), 115 deletions(-)

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> Drop local copy of gconv file parsing and use the one in
> gconv_parseconfdir.h instead.  Now there is a single implementation of
> configuration file parsing.
> ---
>  iconv/iconvconfig.c | 126 ++++----------------------------------------
>  1 file changed, 11 insertions(+), 115 deletions(-)
>
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index c9607fb645..53e5c5c6f4 100644
> --- a/iconv/iconvconfig.c
> +++ b/iconv/iconvconfig.c
> @@ -18,7 +18,6 @@
>  
>  #include <argp.h>
>  #include <assert.h>
> -#include <dirent.h>
>  #include <error.h>
>  #include <errno.h>
>  #include <fcntl.h>
> @@ -34,10 +33,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <sys/cdefs.h>
> -#include <sys/types.h>
>  #include <sys/uio.h>
>  
>  #include "iconvconfig.h"
> +#include <gconv_parseconfdir.h>

Ok.

>  /* Add new module.  */
>  static void
> -add_module (char *rp, const char *directory)
> +add_module (char *rp, const char *directory, size_t dirlen, int modcount)

Do we need some sort of attribute_unused here?

Otherwise OK.

> @@ -646,131 +645,28 @@ add_module (char *rp, const char *directory)
>  	      cost, need_ext);
>  }
>  
> -/* Read a gconv-modules configuration file.  */
> -static bool
> -handle_file (const char *dir, const char *infile)
> -{
> -  FILE *fp;
> -  char *line = NULL;
> -  size_t linelen = 0;
> -
> -  fp = fopen (infile, "r");
> -  if (fp == NULL)
> -    return false;
> -
> -  /* No threads present.  */
> -  __fsetlocking (fp, FSETLOCKING_BYCALLER);
> -
> -  while (!feof_unlocked (fp))
> -    {
> -      char *rp, *endp, *word;
> -      ssize_t n = __getdelim (&line, &linelen, '\n', fp);
> -
> -      if (n < 0)
> -	/* An error occurred.  */
> -	break;
> -
> -      rp = line;
> -      /* Terminate the line (excluding comments or newline) with a NUL
> -	 byte to simplify the following code.  */
> -      endp = strchr (rp, '#');
> -      if (endp != NULL)
> -	*endp = '\0';
> -      else
> -	if (rp[n - 1] == '\n')
> -	  rp[n - 1] = '\0';
> -
> -      while (isspace (*rp))
> -	++rp;
> -
> -      /* If this is an empty line go on with the next one.  */
> -      if (rp == endp)
> -	continue;
> -
> -      word = rp;
> -      while (*rp != '\0' && !isspace (*rp))
> -	++rp;
> -
> -      if (rp - word == sizeof ("alias") - 1
> -	  && memcmp (word, "alias", sizeof ("alias") - 1) == 0)
> -	add_alias (rp);
> -      else if (rp - word == sizeof ("module") - 1
> -	       && memcmp (word, "module", sizeof ("module") - 1) == 0)
> -	add_module (rp, dir);
> -      /* else */
> -	/* Otherwise ignore the line.  */
> -    }
> -
> -  free (line);
> -
> -  fclose (fp);
> -
> -  return true;
> -}
> -

No longer needed; ok.

>  /* Read config files and add the data for this directory to cache.  */
>  static int
>  handle_dir (const char *dir)
>  {
> -  char *cp;
>    size_t dirlen = strlen (dir);
>    bool found = false;
>  
> +  /* Add the prefix before sending it off to the parser.  */
> +  char *fulldir = xmalloc (prefix_len + dirlen + 2);
> +  char *cp = mempcpy (mempcpy (fulldir, prefix, prefix_len), dir, dirlen);
> +

+1 for / and NUL; ok.

>    if (dir[dirlen - 1] != '/')
>      {
> -      char *newp = (char *) xmalloc (dirlen + 2);
> -      dir = memcpy (newp, dir, dirlen);
> -      newp[dirlen++] = '/';
> -      newp[dirlen] = '\0';
> +      *cp++ = '/';
> +      *cp = '\0';
> +      dirlen += 2;
>      }

Why +=2 and not ++ ?  We don't want to be mempcpy'ing the NUL into the
target string in the .h.

> -  /* First, look for a gconv-modules file.  */
> -  char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d");
> -  if (buf == NULL)
> -    goto out;
> -
> -  cp = buf;
> -  if (dir[0] == '/')
> -    cp = mempcpy (cp, prefix, prefix_len);
> -  cp = mempcpy (cp, dir, dirlen);
> -  cp = stpcpy (cp, "gconv-modules");
> -
> -  found |= handle_file (dir, buf);
> -
> -  /* Next, see if there is a gconv-modules.d directory containing configuration
> -     files and if it is non-empty.  */
> -  cp[0] = '.';
> -  cp[1] = 'd';
> -  cp[2] = '\0';
> -
> -  DIR *confdir = opendir (buf);
> -  if (confdir != NULL)
> -    {
> -      struct dirent *ent;
> -      while ((ent = readdir (confdir)) != NULL)
> -	{
> -	  if (ent->d_type != DT_REG)
> -	    continue;
> -
> -	  size_t len = strlen (ent->d_name);
> -	  const char *suffix = ".conf";
> -
> -	  if (len > strlen (suffix)
> -	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
> -	    {
> -	      char *conf;
> -	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> -		continue;
> -	      found |= handle_file (dir, conf);
> -	      free (conf);
> -	    }
> -	}
> -      closedir (confdir);
> -    }
> +  found = gconv_parseconfdir (fulldir, dirlen + prefix_len);
>  
> -  free (buf);
> +  free (fulldir);

Ok.

> -out:
>    if (!found)
>      {
>        error (0, errno, "failed to open gconv configuration files in `%s'",

Ok.

The 06/10/2021 16:48, Siddhesh Poyarekar via Libc-alpha wrote:
> Drop local copy of gconv file parsing and use the one in
> gconv_parseconfdir.h instead.  Now there is a single implementation of
> configuration file parsing.

build fails for me after this with

iconvconfig.c:(.text+0x7ec): undefined reference to `__feof_unlocked'

if i configure with -Os --disable-werror

i.e. the patch relies on __USE_EXTERN_INLINES to inline __feof_unlocked
(which is not a public symbol so should not be referenced from iconv binaries)

> ---
>  iconv/iconvconfig.c | 126 ++++----------------------------------------
>  1 file changed, 11 insertions(+), 115 deletions(-)
> 
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index c9607fb645..53e5c5c6f4 100644
> --- a/iconv/iconvconfig.c
> +++ b/iconv/iconvconfig.c
> @@ -18,7 +18,6 @@
>  
>  #include <argp.h>
>  #include <assert.h>
> -#include <dirent.h>
>  #include <error.h>
>  #include <errno.h>
>  #include <fcntl.h>
> @@ -34,10 +33,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <sys/cdefs.h>
> -#include <sys/types.h>
>  #include <sys/uio.h>
>  
>  #include "iconvconfig.h"
> +#include <gconv_parseconfdir.h>
>  
>  /* Get libc version number.  */
>  #include "../version.h"
> @@ -568,7 +567,7 @@ new_module (const char *fromname, size_t fromlen, const char *toname,
>  
>  /* Add new module.  */
>  static void
> -add_module (char *rp, const char *directory)
> +add_module (char *rp, const char *directory, size_t dirlen, int modcount)
>  {
>    /* We expect now
>       1. `from' name
> @@ -646,131 +645,28 @@ add_module (char *rp, const char *directory)
>  	      cost, need_ext);
>  }
>  
> -/* Read a gconv-modules configuration file.  */
> -static bool
> -handle_file (const char *dir, const char *infile)
> -{
> -  FILE *fp;
> -  char *line = NULL;
> -  size_t linelen = 0;
> -
> -  fp = fopen (infile, "r");
> -  if (fp == NULL)
> -    return false;
> -
> -  /* No threads present.  */
> -  __fsetlocking (fp, FSETLOCKING_BYCALLER);
> -
> -  while (!feof_unlocked (fp))
> -    {
> -      char *rp, *endp, *word;
> -      ssize_t n = __getdelim (&line, &linelen, '\n', fp);
> -
> -      if (n < 0)
> -	/* An error occurred.  */
> -	break;
> -
> -      rp = line;
> -      /* Terminate the line (excluding comments or newline) with a NUL
> -	 byte to simplify the following code.  */
> -      endp = strchr (rp, '#');
> -      if (endp != NULL)
> -	*endp = '\0';
> -      else
> -	if (rp[n - 1] == '\n')
> -	  rp[n - 1] = '\0';
> -
> -      while (isspace (*rp))
> -	++rp;
> -
> -      /* If this is an empty line go on with the next one.  */
> -      if (rp == endp)
> -	continue;
> -
> -      word = rp;
> -      while (*rp != '\0' && !isspace (*rp))
> -	++rp;
> -
> -      if (rp - word == sizeof ("alias") - 1
> -	  && memcmp (word, "alias", sizeof ("alias") - 1) == 0)
> -	add_alias (rp);
> -      else if (rp - word == sizeof ("module") - 1
> -	       && memcmp (word, "module", sizeof ("module") - 1) == 0)
> -	add_module (rp, dir);
> -      /* else */
> -	/* Otherwise ignore the line.  */
> -    }
> -
> -  free (line);
> -
> -  fclose (fp);
> -
> -  return true;
> -}
> -
>  /* Read config files and add the data for this directory to cache.  */
>  static int
>  handle_dir (const char *dir)
>  {
> -  char *cp;
>    size_t dirlen = strlen (dir);
>    bool found = false;
>  
> +  /* Add the prefix before sending it off to the parser.  */
> +  char *fulldir = xmalloc (prefix_len + dirlen + 2);
> +  char *cp = mempcpy (mempcpy (fulldir, prefix, prefix_len), dir, dirlen);
> +
>    if (dir[dirlen - 1] != '/')
>      {
> -      char *newp = (char *) xmalloc (dirlen + 2);
> -      dir = memcpy (newp, dir, dirlen);
> -      newp[dirlen++] = '/';
> -      newp[dirlen] = '\0';
> +      *cp++ = '/';
> +      *cp = '\0';
> +      dirlen += 2;
>      }
>  
> -  /* First, look for a gconv-modules file.  */
> -  char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d");
> -  if (buf == NULL)
> -    goto out;
> -
> -  cp = buf;
> -  if (dir[0] == '/')
> -    cp = mempcpy (cp, prefix, prefix_len);
> -  cp = mempcpy (cp, dir, dirlen);
> -  cp = stpcpy (cp, "gconv-modules");
> -
> -  found |= handle_file (dir, buf);
> -
> -  /* Next, see if there is a gconv-modules.d directory containing configuration
> -     files and if it is non-empty.  */
> -  cp[0] = '.';
> -  cp[1] = 'd';
> -  cp[2] = '\0';
> -
> -  DIR *confdir = opendir (buf);
> -  if (confdir != NULL)
> -    {
> -      struct dirent *ent;
> -      while ((ent = readdir (confdir)) != NULL)
> -	{
> -	  if (ent->d_type != DT_REG)
> -	    continue;
> -
> -	  size_t len = strlen (ent->d_name);
> -	  const char *suffix = ".conf";
> -
> -	  if (len > strlen (suffix)
> -	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
> -	    {
> -	      char *conf;
> -	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> -		continue;
> -	      found |= handle_file (dir, conf);
> -	      free (conf);
> -	    }
> -	}
> -      closedir (confdir);
> -    }
> +  found = gconv_parseconfdir (fulldir, dirlen + prefix_len);
>  
> -  free (buf);
> +  free (fulldir);
>  
> -out:
>    if (!found)
>      {
>        error (0, errno, "failed to open gconv configuration files in `%s'",
> -- 
> 2.31.1
> 

--

The 07/02/2021 10:11, Szabolcs Nagy via Libc-alpha wrote:
> if i configure with -Os --disable-werror

i meant CLFAGS=-Os --disable-werror