[PATCH 0/6] gconv configuration parsing cleanups

[PATCH 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

This patchset consolidates the file parsing in gconv_conf and
iconvconfig into a single file gconv_parseconfdir.  Also, add a NEWS
item to mention gconv-modules.d since it is a user visible change.

Siddhesh Poyarekar (6):
  iconv: Remove alloca use in gconv-modules configuration parsing
  gconv_conf: Remove unused variables
  gconv_conf: Split out configuration file processing
  iconvconfig: Use common gconv module parsing function
  Handle DT_UNKNOWN in gconv-modules.d
  Add NEWS item for gconv-modules.d change

 NEWS                       |   6 ++
 iconv/gconv_conf.c         | 137 ++----------------------------
 iconv/gconv_parseconfdir.h | 170 +++++++++++++++++++++++++++++++++++++
 iconv/iconvconfig.c        | 119 +++-----------------------
 4 files changed, 195 insertions(+), 237 deletions(-)
 create mode 100644 iconv/gconv_parseconfdir.h

-- 
2.31.1

[PATCH 1/6] iconv: Remove alloca use in gconv-modules configuration parsing

[Siddhesh Poyarekar]

The alloca sizes ought to be constrained to PATH_MAX, but replace them
with dynamic allocation to be safe.  A static PATH_MAX array would
have worked too but Hurd does not have PATH_MAX and the code path is
not hot enough to micro-optimise this allocation.  Revisit if any of
those realities change.
---
 iconv/gconv_conf.c  | 17 +++++++++--------
 iconv/iconvconfig.c | 17 +++++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index c8ad8099a4..3f2cef255b 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -559,15 +559,15 @@ __gconv_read_conf (void)
 
   for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
     {
-#define BUF_LEN elem_len + sizeof (gconv_conf_dirname)
-
       const char *elem = __gconv_path_elem[cnt].name;
       size_t elem_len = __gconv_path_elem[cnt].len;
-      char *buf;
 
       /* No slash needs to be inserted between elem and gconv_conf_filename;
 	 elem already ends in a slash.  */
-      buf = alloca (BUF_LEN);
+      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
+      if (buf == NULL)
+	continue;
+
       char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
 			    gconv_conf_filename, sizeof (gconv_conf_filename));
 
@@ -596,15 +596,16 @@ __gconv_read_conf (void)
 	      if (len > strlen (suffix)
 		  && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
 		{
-		  /* LEN <= PATH_MAX so this alloca is not unbounded.  */
-		  char *conf = alloca (BUF_LEN + len + 1);
-		  cp = stpcpy (conf, buf);
-		  sprintf (cp, "/%s", ent->d_name);
+		  char *conf;
+		  if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
+		    continue;
 		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
+		  free (conf);
 		}
 	    }
 	  __closedir (confdir);
 	}
+      free (buf);
     }
 #endif
 
diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
index b2a868919c..c9607fb645 100644
--- a/iconv/iconvconfig.c
+++ b/iconv/iconvconfig.c
@@ -712,7 +712,6 @@ handle_file (const char *dir, const char *infile)
 static int
 handle_dir (const char *dir)
 {
-#define BUF_LEN prefix_len + dirlen + sizeof "gconv-modules.d"
   char *cp;
   size_t dirlen = strlen (dir);
   bool found = false;
@@ -726,7 +725,10 @@ handle_dir (const char *dir)
     }
 
   /* First, look for a gconv-modules file.  */
-  char buf[BUF_LEN];
+  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);
@@ -756,16 +758,19 @@ handle_dir (const char *dir)
 	  if (len > strlen (suffix)
 	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
 	    {
-	      /* LEN <= PATH_MAX so this alloca is not unbounded.  */
-	      char *conf = alloca (BUF_LEN + len + 1);
-	      cp = stpcpy (conf, buf);
-	      sprintf (cp, "/%s", ent->d_name);
+	      char *conf;
+	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
+		continue;
 	      found |= handle_file (dir, conf);
+	      free (conf);
 	    }
 	}
       closedir (confdir);
     }
 
+  free (buf);
+
+out:
   if (!found)
     {
       error (0, errno, "failed to open gconv configuration files in `%s'",
-- 
2.31.1

Re: [PATCH 1/6] iconv: Remove alloca use in gconv-modules configuration parsing

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> The alloca sizes ought to be constrained to PATH_MAX, but replace them
> with dynamic allocation to be safe.  A static PATH_MAX array would
> have worked too but Hurd does not have PATH_MAX and the code path is
> not hot enough to micro-optimise this allocation.  Revisit if any of
> those realities change.
> ---
>  iconv/gconv_conf.c  | 17 +++++++++--------
>  iconv/iconvconfig.c | 17 +++++++++++------
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index c8ad8099a4..3f2cef255b 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -559,15 +559,15 @@ __gconv_read_conf (void)
>  
>    for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
>      {
> -#define BUF_LEN elem_len + sizeof (gconv_conf_dirname)
> -
>        const char *elem = __gconv_path_elem[cnt].name;
>        size_t elem_len = __gconv_path_elem[cnt].len;
> -      char *buf;
>  
>        /* No slash needs to be inserted between elem and gconv_conf_filename;
>  	 elem already ends in a slash.  */
> -      buf = alloca (BUF_LEN);
> +      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
> +      if (buf == NULL)
> +	continue;
> +
>        char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
>  			    gconv_conf_filename, sizeof (gconv_conf_filename));
>  

replaces alloca with malloc; sizes are the same.  Ok.

> @@ -596,15 +596,16 @@ __gconv_read_conf (void)
>  	      if (len > strlen (suffix)
>  		  && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  		{
> -		  /* LEN <= PATH_MAX so this alloca is not unbounded.  */
> -		  char *conf = alloca (BUF_LEN + len + 1);
> -		  cp = stpcpy (conf, buf);
> -		  sprintf (cp, "/%s", ent->d_name);
> +		  char *conf;
> +		  if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> +		    continue;
>  		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
> +		  free (conf);

allocated by asprintf, free'd here.  Ok.

>  		}
>  	    }
>  	  __closedir (confdir);
>  	}
> +      free (buf);

Matched free, OK.

>      }
>  #endif
>  
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index b2a868919c..c9607fb645 100644
> --- a/iconv/iconvconfig.c
> +++ b/iconv/iconvconfig.c
> @@ -712,7 +712,6 @@ handle_file (const char *dir, const char *infile)
>  static int
>  handle_dir (const char *dir)
>  {
> -#define BUF_LEN prefix_len + dirlen + sizeof "gconv-modules.d"
>    char *cp;
>    size_t dirlen = strlen (dir);
>    bool found = false;
> @@ -726,7 +725,10 @@ handle_dir (const char *dir)
>      }
>  
>    /* First, look for a gconv-modules file.  */
> -  char buf[BUF_LEN];
> +  char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d");
> +  if (buf == NULL)
> +    goto out;
> +

Likewise, ok.

>    cp = buf;
>    if (dir[0] == '/')
>      cp = mempcpy (cp, prefix, prefix_len);
> @@ -756,16 +758,19 @@ handle_dir (const char *dir)
>  	  if (len > strlen (suffix)
>  	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  	    {
> -	      /* LEN <= PATH_MAX so this alloca is not unbounded.  */
> -	      char *conf = alloca (BUF_LEN + len + 1);
> -	      cp = stpcpy (conf, buf);
> -	      sprintf (cp, "/%s", ent->d_name);
> +	      char *conf;
> +	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> +		continue;
>  	      found |= handle_file (dir, conf);
> +	      free (conf);

Likewise, ok.

>  	    }
>  	}
>        closedir (confdir);
>      }
>  
> +  free (buf);
> +
> +out:

Freed, ok.

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

LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>

[PATCH 2/6] gconv_conf: Remove unused variables

[Siddhesh Poyarekar]

The modules and nmodules parameters passed to add_modules, add_alias,
etc. are not used and are hence unnecessary.  Remove them so that
their signatures match the functions in iconvconfig.
---
 iconv/gconv_conf.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index 3f2cef255b..6c6625c37a 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -132,7 +132,7 @@ detect_conflict (const char *alias)
 
 /* The actual code to add aliases.  */
 static void
-add_alias2 (const char *from, const char *to, const char *wp, void *modules)
+add_alias2 (const char *from, const char *to, const char *wp)
 {
   /* Test whether this alias conflicts with any available module.  */
   if (detect_conflict (from))
@@ -161,7 +161,7 @@ add_alias2 (const char *from, const char *to, const char *wp, void *modules)
 
 /* Add new alias.  */
 static void
-add_alias (char *rp, void *modules)
+add_alias (char *rp)
 {
   /* We now expect two more string.  The strings are normalized
      (converted to UPPER case) and strored in the alias database.  */
@@ -186,7 +186,7 @@ add_alias (char *rp, void *modules)
     return;
   *wp++ = '\0';
 
-  add_alias2 (from, to, wp, modules);
+  add_alias2 (from, to, wp);
 }
 
 
@@ -250,8 +250,7 @@ insert_module (struct gconv_module *newp, int tobefreed)
 
 /* Add new module.  */
 static void
-add_module (char *rp, const char *directory, size_t dir_len, void **modules,
-	    size_t *nmodules, int modcounter)
+add_module (char *rp, const char *directory, size_t dir_len, int modcounter)
 {
   /* We expect now
      1. `from' name
@@ -364,8 +363,7 @@ add_module (char *rp, const char *directory, size_t dir_len, void **modules,
 
 /* Read the next configuration file.  */
 static void
-read_conf_file (const char *filename, const char *directory, size_t dir_len,
-		void **modules, size_t *nmodules)
+read_conf_file (const char *filename, const char *directory, size_t dir_len)
 {
   /* Note the file is opened with cancellation in the I/O functions
      disabled.  */
@@ -415,10 +413,10 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len,
 
       if (rp - word == sizeof ("alias") - 1
 	  && memcmp (word, "alias", sizeof ("alias") - 1) == 0)
-	add_alias (rp, *modules);
+	add_alias (rp);
       else if (rp - word == sizeof ("module") - 1
 	       && memcmp (word, "module", sizeof ("module") - 1) == 0)
-	add_module (rp, directory, dir_len, modules, nmodules, modcounter++);
+	add_module (rp, directory, dir_len, modcounter++);
       /* else */
 	/* Otherwise ignore the line.  */
     }
@@ -540,8 +538,6 @@ __gconv_get_path (void)
 static void
 __gconv_read_conf (void)
 {
-  void *modules = NULL;
-  size_t nmodules = 0;
   int save_errno = errno;
   size_t cnt;
 
@@ -572,7 +568,7 @@ __gconv_read_conf (void)
 			    gconv_conf_filename, sizeof (gconv_conf_filename));
 
       /* Read the gconv-modules configuration file first.  */
-      read_conf_file (buf, elem, elem_len, &modules, &nmodules);
+      read_conf_file (buf, elem, elem_len);
 
       /* Next, see if there is a gconv-modules.d directory containing
 	 configuration files and if it is non-empty.  */
@@ -599,7 +595,7 @@ __gconv_read_conf (void)
 		  char *conf;
 		  if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
 		    continue;
-		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
+		  read_conf_file (conf, elem, elem_len);
 		  free (conf);
 		}
 	    }
@@ -633,7 +629,7 @@ __gconv_read_conf (void)
       const char *to = __rawmemchr (from, '\0') + 1;
       cp = __rawmemchr (to, '\0') + 1;
 
-      add_alias2 (from, to, cp, modules);
+      add_alias2 (from, to, cp);
     }
   while (*cp != '\0');
 
-- 
2.31.1

Re: [PATCH 2/6] gconv_conf: Remove unused variables

[Andreas Schwab]

On Jun 10 2021, Siddhesh Poyarekar wrote:

> The modules and nmodules parameters passed to add_modules, add_alias,
> etc. are not used and are hence unnecessary.  Remove them so that
> their signatures match the functions in iconvconfig.

Ok.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 2/6] gconv_conf: Remove unused variables

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> The modules and nmodules parameters passed to add_modules, add_alias,
> etc. are not used and are hence unnecessary.  Remove them so that
> their signatures match the functions in iconvconfig.
> ---
>  iconv/gconv_conf.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index 3f2cef255b..6c6625c37a 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -132,7 +132,7 @@ detect_conflict (const char *alias)
>  
>  /* The actual code to add aliases.  */
>  static void
> -add_alias2 (const char *from, const char *to, const char *wp, void *modules)
> +add_alias2 (const char *from, const char *to, const char *wp)

Drop modules, ok.

> -add_alias (char *rp, void *modules)
> +add_alias (char *rp)

Likewise.  Ok.

> -  add_alias2 (from, to, wp, modules);
> +  add_alias2 (from, to, wp);

Likewise.  Ok.

> @@ -250,8 +250,7 @@ insert_module (struct gconv_module *newp, int tobefreed)
>  
>  /* Add new module.  */
>  static void
> -add_module (char *rp, const char *directory, size_t dir_len, void **modules,
> -	    size_t *nmodules, int modcounter)
> +add_module (char *rp, const char *directory, size_t dir_len, int modcounter)

Likewise, ok.

> -read_conf_file (const char *filename, const char *directory, size_t dir_len,
> -		void **modules, size_t *nmodules)
> +read_conf_file (const char *filename, const char *directory, size_t dir_len)

Likewise, ok.

> -	add_alias (rp, *modules);
> +	add_alias (rp);
>        else if (rp - word == sizeof ("module") - 1
>  	       && memcmp (word, "module", sizeof ("module") - 1) == 0)
> -	add_module (rp, directory, dir_len, modules, nmodules, modcounter++);
> +	add_module (rp, directory, dir_len, modcounter++);

Ok.

>  static void
>  __gconv_read_conf (void)
>  {
> -  void *modules = NULL;
> -  size_t nmodules = 0;

Don't use them, don't need them.  Ok.

> -      read_conf_file (buf, elem, elem_len, &modules, &nmodules);
> +      read_conf_file (buf, elem, elem_len);

Ok.

> -		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
> +		  read_conf_file (conf, elem, elem_len);

Ok.

> -      add_alias2 (from, to, cp, modules);
> +      add_alias2 (from, to, cp);

Ok.

LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>

[PATCH 3/6] gconv_conf: Split out configuration file processing

[Siddhesh Poyarekar]

Split configuration file processing into a separate header file and
include it.  Macroize all calls that need to go through internal
interfaces so that iconvconfig can also use them.
---
 iconv/gconv_conf.c         | 129 +----------------------------
 iconv/gconv_parseconfdir.h | 161 +++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+), 126 deletions(-)
 create mode 100644 iconv/gconv_parseconfdir.h

diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index 6c6625c37a..62bee28769 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -19,7 +19,6 @@
 
 #include <assert.h>
 #include <ctype.h>
-#include <dirent.h>
 #include <errno.h>
 #include <limits.h>
 #include <locale.h>
@@ -31,11 +30,10 @@
 #include <string.h>
 #include <unistd.h>
 #include <sys/param.h>
-#include <sys/types.h>
 
 #include <libc-lock.h>
 #include <gconv_int.h>
-
+#include <gconv_parseconfdir.h>
 
 /* This is the default path where we look for module lists.  */
 static const char default_gconv_path[] = GCONV_PATH;
@@ -56,11 +54,6 @@ size_t __gconv_max_path_elem_len;
 /* We use the following struct if we couldn't allocate memory.  */
 static const struct path_elem empty_path_elem = { NULL, 0 };
 
-/* Name of the file containing the module information in the directories
-   along the path.  */
-static const char gconv_conf_filename[] = "gconv-modules";
-static const char gconv_conf_dirname[] = "gconv-modules.d";
-
 /* Filename extension for the modules.  */
 #ifndef MODULE_EXT
 # define MODULE_EXT ".so"
@@ -99,9 +92,6 @@ static const char builtin_aliases[] =
 #undef BUILTIN_ALIAS
 };
 
-#include <libio/libioP.h>
-#define __getdelim(line, len, c, fp) _IO_getdelim (line, len, c, fp)
-
 
 /* Value of the GCONV_PATH environment variable.  */
 const char *__gconv_path_envvar;
@@ -361,72 +351,6 @@ add_module (char *rp, const char *directory, size_t dir_len, int modcounter)
 }
 
 
-/* Read the next configuration file.  */
-static void
-read_conf_file (const char *filename, const char *directory, size_t dir_len)
-{
-  /* Note the file is opened with cancellation in the I/O functions
-     disabled.  */
-  FILE *fp = fopen (filename, "rce");
-  char *line = NULL;
-  size_t line_len = 0;
-  static int modcounter;
-
-  /* Don't complain if a file is not present or readable, simply silently
-     ignore it.  */
-  if (fp == NULL)
-    return;
-
-  /* No threads reading from this stream.  */
-  __fsetlocking (fp, FSETLOCKING_BYCALLER);
-
-  /* Process the known entries of the file.  Comments start with `#' and
-     end with the end of the line.  Empty lines are ignored.  */
-  while (!__feof_unlocked (fp))
-    {
-      char *rp, *endp, *word;
-      ssize_t n = __getdelim (&line, &line_len, '\n', fp);
-      if (n < 0)
-	/* An error occurred.  */
-	break;
-
-      rp = line;
-      /* Terminate the line (excluding comments or newline) by an 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_l (*rp, _nl_C_locobj_ptr))
-	++rp;
-
-      /* If this is an empty line go on with the next one.  */
-      if (rp == endp)
-	continue;
-
-      word = rp;
-      while (*rp != '\0' && !__isspace_l (*rp, _nl_C_locobj_ptr))
-	++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, directory, dir_len, modcounter++);
-      /* else */
-	/* Otherwise ignore the line.  */
-    }
-
-  free (line);
-
-  fclose (fp);
-}
-
-
 /* Determine the directories we are looking for data in.  This function should
    only be called from __gconv_read_conf.  */
 static void
@@ -554,55 +478,8 @@ __gconv_read_conf (void)
   __gconv_get_path ();
 
   for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
-    {
-      const char *elem = __gconv_path_elem[cnt].name;
-      size_t elem_len = __gconv_path_elem[cnt].len;
-
-      /* No slash needs to be inserted between elem and gconv_conf_filename;
-	 elem already ends in a slash.  */
-      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
-      if (buf == NULL)
-	continue;
-
-      char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
-			    gconv_conf_filename, sizeof (gconv_conf_filename));
-
-      /* Read the gconv-modules configuration file first.  */
-      read_conf_file (buf, elem, elem_len);
-
-      /* Next, see if there is a gconv-modules.d directory containing
-	 configuration files and if it is non-empty.  */
-      cp--;
-      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;
-		  read_conf_file (conf, elem, elem_len);
-		  free (conf);
-		}
-	    }
-	  __closedir (confdir);
-	}
-      free (buf);
-    }
+    gconv_parseconfdir (__gconv_path_elem[cnt].name,
+			__gconv_path_elem[cnt].len);
 #endif
 
   /* Add the internal modules.  */
diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
new file mode 100644
index 0000000000..3d4d58d4be
--- /dev/null
+++ b/iconv/gconv_parseconfdir.h
@@ -0,0 +1,161 @@
+/* Handle configuration data.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <dirent.h>
+#include <libc-symbols.h>
+#include <locale.h>
+#include <sys/types.h>
+
+#if IS_IN (libc)
+# include <libio/libioP.h>
+# define __getdelim(line, len, c, fp) _IO_getdelim (line, len, c, fp)
+
+# undef isspace
+# define isspace(__c) __isspace_l ((__c), _nl_C_locobj_ptr)
+# define asprintf __asprintf
+# define opendir __opendir
+# define readdir __readdir
+# define closedir __closedir
+# define mempcpy __mempcpy
+#endif
+
+/* Name of the file containing the module information in the directories
+   along the path.  */
+static const char gconv_conf_filename[] = "gconv-modules";
+static const char gconv_conf_dirname[] = "gconv-modules.d";
+
+static void add_alias (char *);
+static void add_module (char *, const char *, size_t, int);
+
+/* Read the next configuration file.  */
+static bool
+read_conf_file (const char *filename, const char *directory, size_t dir_len)
+{
+  /* Note the file is opened with cancellation in the I/O functions
+     disabled.  */
+  FILE *fp = fopen (filename, "rce");
+  char *line = NULL;
+  size_t line_len = 0;
+  static int modcounter;
+
+  /* Don't complain if a file is not present or readable, simply silently
+     ignore it.  */
+  if (fp == NULL)
+    return false;
+
+  /* No threads reading from this stream.  */
+  __fsetlocking (fp, FSETLOCKING_BYCALLER);
+
+  /* Process the known entries of the file.  Comments start with `#' and
+     end with the end of the line.  Empty lines are ignored.  */
+  while (!__feof_unlocked (fp))
+    {
+      char *rp, *endp, *word;
+      ssize_t n = __getdelim (&line, &line_len, '\n', fp);
+      if (n < 0)
+	/* An error occurred.  */
+	break;
+
+      rp = line;
+      /* Terminate the line (excluding comments or newline) by an 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, directory, dir_len, modcounter++);
+      /* else */
+	/* Otherwise ignore the line.  */
+    }
+
+  free (line);
+
+  fclose (fp);
+  return true;
+}
+
+static __always_inline bool
+gconv_parseconfdir (const char *dir, size_t dir_len)
+{
+  /* No slash needs to be inserted between dir and gconv_conf_filename;
+     dir already ends in a slash.  */
+  char *buf = malloc (dir_len + sizeof (gconv_conf_dirname));
+  bool found = false;
+
+  if (buf == NULL)
+    return false;
+
+  char *cp = mempcpy (mempcpy (buf, dir, dir_len), gconv_conf_filename,
+		      sizeof (gconv_conf_filename));
+
+  /* Read the gconv-modules configuration file first.  */
+  found = read_conf_file (buf, dir, dir_len);
+
+  /* Next, see if there is a gconv-modules.d directory containing
+     configuration files and if it is non-empty.  */
+  cp--;
+  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 |= read_conf_file (conf, dir, dir_len);
+	      free (conf);
+	    }
+	}
+      closedir (confdir);
+    }
+  free (buf);
+  return found;
+}
-- 
2.31.1

Re: [PATCH 3/6] gconv_conf: Split out configuration file processing

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> Split configuration file processing into a separate header file and
> include it.  Macroize all calls that need to go through internal
> interfaces so that iconvconfig can also use them.

> -#include <dirent.h>
>  #include <errno.h>
>  #include <limits.h>
>  #include <locale.h>
> @@ -31,11 +30,10 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <sys/param.h>
> -#include <sys/types.h>
>  
>  #include <libc-lock.h>
>  #include <gconv_int.h>
> -
> +#include <gconv_parseconfdir.h>

Ok; gconv_parseconfdir.h includes those two deleted ones.

> -/* Name of the file containing the module information in the directories
> -   along the path.  */
> -static const char gconv_conf_filename[] = "gconv-modules";
> -static const char gconv_conf_dirname[] = "gconv-modules.d";
> -

Moved to .h; ok.

> -#include <libio/libioP.h>
> -#define __getdelim(line, len, c, fp) _IO_getdelim (line, len, c, fp)
> -

Moved, ok.

> -/* Read the next configuration file.  */
> -static void
> -read_conf_file (const char *filename, const char *directory, size_t dir_len)
> -{
> -  /* Note the file is opened with cancellation in the I/O functions
> -     disabled.  */
> -  FILE *fp = fopen (filename, "rce");
> -  char *line = NULL;
> -  size_t line_len = 0;
> -  static int modcounter;
> -
> -  /* Don't complain if a file is not present or readable, simply silently
> -     ignore it.  */
> -  if (fp == NULL)
> -    return;
> -
> -  /* No threads reading from this stream.  */
> -  __fsetlocking (fp, FSETLOCKING_BYCALLER);
> -
> -  /* Process the known entries of the file.  Comments start with `#' and
> -     end with the end of the line.  Empty lines are ignored.  */
> -  while (!__feof_unlocked (fp))
> -    {
> -      char *rp, *endp, *word;
> -      ssize_t n = __getdelim (&line, &line_len, '\n', fp);
> -      if (n < 0)
> -	/* An error occurred.  */
> -	break;
> -
> -      rp = line;
> -      /* Terminate the line (excluding comments or newline) by an 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_l (*rp, _nl_C_locobj_ptr))
> -	++rp;
> -
> -      /* If this is an empty line go on with the next one.  */
> -      if (rp == endp)
> -	continue;
> -
> -      word = rp;
> -      while (*rp != '\0' && !__isspace_l (*rp, _nl_C_locobj_ptr))
> -	++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, directory, dir_len, modcounter++);
> -      /* else */
> -	/* Otherwise ignore the line.  */
> -    }
> -
> -  free (line);
> -
> -  fclose (fp);
> -}
> -
> -

Moved to .h; ok.

> @@ -554,55 +478,8 @@ __gconv_read_conf (void)
>    __gconv_get_path ();
>  
>    for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
> -    {
> -      const char *elem = __gconv_path_elem[cnt].name;
> -      size_t elem_len = __gconv_path_elem[cnt].len;
> -
> -      /* No slash needs to be inserted between elem and gconv_conf_filename;
> -	 elem already ends in a slash.  */
> -      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
> -      if (buf == NULL)
> -	continue;
> -
> -      char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
> -			    gconv_conf_filename, sizeof (gconv_conf_filename));
> -
> -      /* Read the gconv-modules configuration file first.  */
> -      read_conf_file (buf, elem, elem_len);
> -
> -      /* Next, see if there is a gconv-modules.d directory containing
> -	 configuration files and if it is non-empty.  */
> -      cp--;
> -      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;
> -		  read_conf_file (conf, elem, elem_len);
> -		  free (conf);
> -		}
> -	    }
> -	  __closedir (confdir);
> -	}
> -      free (buf);
> -    }
> +    gconv_parseconfdir (__gconv_path_elem[cnt].name,
> +			__gconv_path_elem[cnt].len);
>  #endif

Ok, found in .h.

> diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
> new file mode 100644
> index 0000000000..3d4d58d4be
> --- /dev/null
> +++ b/iconv/gconv_parseconfdir.h
> @@ -0,0 +1,161 @@
> +/* Handle configuration data.
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */

Ok.

> +#include <dirent.h>
> +#include <libc-symbols.h>
> +#include <locale.h>
> +#include <sys/types.h>

Ok.

> +#if IS_IN (libc)
> +# include <libio/libioP.h>
> +# define __getdelim(line, len, c, fp) _IO_getdelim (line, len, c, fp)
> +
> +# undef isspace
> +# define isspace(__c) __isspace_l ((__c), _nl_C_locobj_ptr)
> +# define asprintf __asprintf
> +# define opendir __opendir
> +# define readdir __readdir
> +# define closedir __closedir
> +# define mempcpy __mempcpy
> +#endif

Ok.

> +/* Name of the file containing the module information in the directories
> +   along the path.  */
> +static const char gconv_conf_filename[] = "gconv-modules";
> +static const char gconv_conf_dirname[] = "gconv-modules.d";

Ok.

> +static void add_alias (char *);
> +static void add_module (char *, const char *, size_t, int);

Ok.

> +/* Read the next configuration file.  */
> +static bool
> +read_conf_file (const char *filename, const char *directory, size_t dir_len)
> +{
> +  /* Note the file is opened with cancellation in the I/O functions
> +     disabled.  */
> +  FILE *fp = fopen (filename, "rce");
> +  char *line = NULL;
> +  size_t line_len = 0;
> +  static int modcounter;
> +
> +  /* Don't complain if a file is not present or readable, simply silently
> +     ignore it.  */
> +  if (fp == NULL)
> +    return false;
> +
> +  /* No threads reading from this stream.  */
> +  __fsetlocking (fp, FSETLOCKING_BYCALLER);
> +
> +  /* Process the known entries of the file.  Comments start with `#' and
> +     end with the end of the line.  Empty lines are ignored.  */
> +  while (!__feof_unlocked (fp))
> +    {
> +      char *rp, *endp, *word;
> +      ssize_t n = __getdelim (&line, &line_len, '\n', fp);
> +      if (n < 0)
> +	/* An error occurred.  */
> +	break;
> +
> +      rp = line;
> +      /* Terminate the line (excluding comments or newline) by an 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, directory, dir_len, modcounter++);
> +      /* else */
> +	/* Otherwise ignore the line.  */
> +    }
> +
> +  free (line);
> +
> +  fclose (fp);
> +  return true;
> +}
> +

Copied from above; ok.

> +static __always_inline bool
> +gconv_parseconfdir (const char *dir, size_t dir_len)
> +{
> +  /* No slash needs to be inserted between dir and gconv_conf_filename;
> +     dir already ends in a slash.  */
> +  char *buf = malloc (dir_len + sizeof (gconv_conf_dirname));
> +  bool found = false;
> +
> +  if (buf == NULL)
> +    return false;
> +
> +  char *cp = mempcpy (mempcpy (buf, dir, dir_len), gconv_conf_filename,
> +		      sizeof (gconv_conf_filename));
> +
> +  /* Read the gconv-modules configuration file first.  */
> +  found = read_conf_file (buf, dir, dir_len);
> +
> +  /* Next, see if there is a gconv-modules.d directory containing
> +     configuration files and if it is non-empty.  */
> +  cp--;
> +  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 |= read_conf_file (conf, dir, dir_len);
> +	      free (conf);
> +	    }
> +	}
> +      closedir (confdir);
> +    }
> +  free (buf);
> +  return found;
> +}

Extracted from above; ok.

LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>

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

[Siddhesh Poyarekar]

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>
 
 /* 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

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

[DJ Delorie]

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.

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

[Szabolcs Nagy]

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
> 

-- 

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

[Szabolcs Nagy]

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

[PATCH 5/6] Handle DT_UNKNOWN in gconv-modules.d

[Siddhesh Poyarekar]

On filesystems that do not support dt_type, a regular file shows up as
DT_UNKNOWN.  Fall back to using lstat64 to read file properties in
such cases.
---
 iconv/gconv_parseconfdir.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
index 3d4d58d4be..e73ea0ff5c 100644
--- a/iconv/gconv_parseconfdir.h
+++ b/iconv/gconv_parseconfdir.h
@@ -32,6 +32,7 @@
 # define readdir __readdir
 # define closedir __closedir
 # define mempcpy __mempcpy
+# define lstat64 __lstat64
 #endif
 
 /* Name of the file containing the module information in the directories
@@ -138,7 +139,7 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
       struct dirent *ent;
       while ((ent = readdir (confdir)) != NULL)
 	{
-	  if (ent->d_type != DT_REG)
+	  if (ent->d_type != DT_REG && ent->d_type != DT_UNKNOWN)
 	    continue;
 
 	  size_t len = strlen (ent->d_name);
@@ -148,8 +149,14 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
 	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
 	    {
 	      char *conf;
+	      struct stat64 st;
 	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
 		continue;
+	      if (ent->d_type == DT_UNKNOWN
+		  && (lstat64 (conf, &st) == -1
+		      || !S_ISREG (st.st_mode)))
+		continue;
+
 	      found |= read_conf_file (conf, dir, dir_len);
 	      free (conf);
 	    }
-- 
2.31.1

Re: [PATCH 5/6] Handle DT_UNKNOWN in gconv-modules.d

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> On filesystems that do not support dt_type, a regular file shows up as
> DT_UNKNOWN.  Fall back to using lstat64 to read file properties in
> such cases.
> ---
>  iconv/gconv_parseconfdir.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
> index 3d4d58d4be..e73ea0ff5c 100644
> --- a/iconv/gconv_parseconfdir.h
> +++ b/iconv/gconv_parseconfdir.h
> @@ -32,6 +32,7 @@
>  # define readdir __readdir
>  # define closedir __closedir
>  # define mempcpy __mempcpy
> +# define lstat64 __lstat64
>  #endif

Ok.

>  /* Name of the file containing the module information in the directories
> @@ -138,7 +139,7 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
>        struct dirent *ent;
>        while ((ent = readdir (confdir)) != NULL)
>  	{
> -	  if (ent->d_type != DT_REG)
> +	  if (ent->d_type != DT_REG && ent->d_type != DT_UNKNOWN)
>  	    continue;

Ok.

> @@ -148,8 +149,14 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
>  	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  	    {
>  	      char *conf;
> +	      struct stat64 st;
>  	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
>  		continue;
> +	      if (ent->d_type == DT_UNKNOWN
> +		  && (lstat64 (conf, &st) == -1
> +		      || !S_ISREG (st.st_mode)))
> +		continue;

Skip non-regular files that weren't reported earlier (or other errors); ok.


LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

[PATCH 6/6] Add NEWS item for gconv-modules.d change

[Siddhesh Poyarekar]

---
 NEWS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/NEWS b/NEWS
index 1bf3daa502..605d53b256 100644
--- a/NEWS
+++ b/NEWS
@@ -31,6 +31,12 @@ Major new features:
   __STDC_WANT_IEC_60559_BFP_EXT__, as specified in TS 18661-1, is
   defined, and when _GNU_SOURCE is defined.
 
+* Configuration files for iconv converter modules have moved from the single
+  gconv-modules file to a directory gconv-modules.d containing multiple
+  configuration files with names ending with .conf.  The configuration parser
+  continues to also support parsing the gconv-modules file in $GCONV_PATH for
+  backward compatibility.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The function pthread_mutex_consistent_np has been deprecated; programs
-- 
2.31.1

Re: [PATCH 6/6] Add NEWS item for gconv-modules.d change

[Siddhesh Poyarekar]

On 6/10/21 4:48 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> ---
>   NEWS | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/NEWS b/NEWS
> index 1bf3daa502..605d53b256 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -31,6 +31,12 @@ Major new features:
>     __STDC_WANT_IEC_60559_BFP_EXT__, as specified in TS 18661-1, is
>     defined, and when _GNU_SOURCE is defined.
>   
> +* Configuration files for iconv converter modules have moved from the single
> +  gconv-modules file to a directory gconv-modules.d containing multiple
> +  configuration files with names ending with .conf.  The configuration parser
> +  continues to also support parsing the gconv-modules file in $GCONV_PATH for
> +  backward compatibility.
> +
>   Deprecated and removed features, and other changes affecting compatibility:
>   
>   * The function pthread_mutex_consistent_np has been deprecated; programs
> 

I'll update and post the NEWS separately once I reach consensus with 
Andreas on the structure of the config files.

Siddhesh

[PING][PATCH 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

Ping!

On 6/10/21 4:48 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> This patchset consolidates the file parsing in gconv_conf and
> iconvconfig into a single file gconv_parseconfdir.  Also, add a NEWS
> item to mention gconv-modules.d since it is a user visible change.
> 
> Siddhesh Poyarekar (6):
>    iconv: Remove alloca use in gconv-modules configuration parsing
>    gconv_conf: Remove unused variables
>    gconv_conf: Split out configuration file processing
>    iconvconfig: Use common gconv module parsing function
>    Handle DT_UNKNOWN in gconv-modules.d
>    Add NEWS item for gconv-modules.d change
> 
>   NEWS                       |   6 ++
>   iconv/gconv_conf.c         | 137 ++----------------------------
>   iconv/gconv_parseconfdir.h | 170 +++++++++++++++++++++++++++++++++++++
>   iconv/iconvconfig.c        | 119 +++-----------------------
>   4 files changed, 195 insertions(+), 237 deletions(-)
>   create mode 100644 iconv/gconv_parseconfdir.h
> 

[Ping 2][PATCH 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

Ping2!  Andreas has reviewed 2/6 but I still need review for 1/6 and 
{3,4,5}/6.

Thanks,
Siddhesh

On 6/10/21 4:48 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> This patchset consolidates the file parsing in gconv_conf and
> iconvconfig into a single file gconv_parseconfdir.  Also, add a NEWS
> item to mention gconv-modules.d since it is a user visible change.
> 
> Siddhesh Poyarekar (6):
>    iconv: Remove alloca use in gconv-modules configuration parsing
>    gconv_conf: Remove unused variables
>    gconv_conf: Split out configuration file processing
>    iconvconfig: Use common gconv module parsing function
>    Handle DT_UNKNOWN in gconv-modules.d
>    Add NEWS item for gconv-modules.d change
> 
>   NEWS                       |   6 ++
>   iconv/gconv_conf.c         | 137 ++----------------------------
>   iconv/gconv_parseconfdir.h | 170 +++++++++++++++++++++++++++++++++++++
>   iconv/iconvconfig.c        | 119 +++-----------------------
>   4 files changed, 195 insertions(+), 237 deletions(-)
>   create mode 100644 iconv/gconv_parseconfdir.h
>