[PATCH v2 0/6] gconv configuration parsing cleanups

[PATCH v2 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.

Changes from v1:

- Updated 4/6 to fix dirlen increment and add unused attribute.

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                       |   8 ++
 iconv/gconv_conf.c         | 142 ++-----------------------------
 iconv/gconv_parseconfdir.h | 168 +++++++++++++++++++++++++++++++++++++
 iconv/iconvconfig.c        | 123 +++------------------------
 4 files changed, 197 insertions(+), 244 deletions(-)
 create mode 100644 iconv/gconv_parseconfdir.h

-- 
2.31.1

[PATCH v2 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.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 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

[PATCH v2 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.

Reviewed-by: DJ Delorie <dj@redhat.com>
Reviewed-by: Andreas Schwab <schwab@linux-m68k.org>
---
 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

[PATCH v2 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.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 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

[PATCH v2 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 | 128 +++++---------------------------------------
 1 file changed, 13 insertions(+), 115 deletions(-)

diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
index c9607fb645..e69334d71c 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,9 @@ 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 __attribute__ ((__unused__)),
+	    int modcount __attribute__ ((__unused__)))
 {
   /* We expect now
      1. `from' name
@@ -646,131 +647,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++;
     }
 
-  /* 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 v2 4/6] iconvconfig: Use common gconv module parsing function

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@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 | 128 +++++---------------------------------------
>  1 file changed, 13 insertions(+), 115 deletions(-)
>
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index c9607fb645..e69334d71c 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.

>  /* Get libc version number.  */
>  #include "../version.h"
> @@ -568,7 +567,9 @@ 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 __attribute__ ((__unused__)),
> +	    int modcount __attribute__ ((__unused__)))

Ok.

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

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);
> +

+2 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++;
>      }

Ok.

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

Ok.

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

Ok.

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

[PATCH v2 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.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 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

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

[Siddhesh Poyarekar]

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

diff --git a/NEWS b/NEWS
index 58cf1dab68..cadc40262f 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,14 @@ Major new features:
   supported when LFS (_FILE_OFFSET_BITS=64) is also enabled.  It is only
   enabled for Linux and the full support requires a minimum version of 5.1.
 
+* The main gconv-modules file in glibc now contains only a small set of
+  essential converter modules and the rest have been moved into a supplementary
+  configuration file gconv-modules-extra.conf in the gconv-modules.d directory
+  in the same GCONV_PATH.  Similarly, external converter modules directories
+  may have supplementary configuration files in a gconv-modules.d directory
+  with names ending with .conf to logically classify the converter modules in
+  that directory.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The function pthread_mutex_consistent_np has been deprecated; programs
-- 
2.31.1

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

[DJ Delorie]

Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> writes:
> diff --git a/NEWS b/NEWS
> index 58cf1dab68..cadc40262f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -39,6 +39,14 @@ Major new features:
>    supported when LFS (_FILE_OFFSET_BITS=64) is also enabled.  It is only
>    enabled for Linux and the full support requires a minimum version of 5.1.
>  
> +* The main gconv-modules file in glibc now contains only a small set of
> +  essential converter modules and the rest have been moved into a supplementary
> +  configuration file gconv-modules-extra.conf in the gconv-modules.d directory
> +  in the same GCONV_PATH.  Similarly, external converter modules directories
> +  may have supplementary configuration files in a gconv-modules.d directory
> +  with names ending with .conf to logically classify the converter modules in
> +  that directory.
> +

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

Re: [PATCH v2 0/6] gconv configuration parsing cleanups

[Andreas Schwab]

I'm now getting an assertion failure on all big-endian platforms.

https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/p/ppc
https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/p/ppc64
https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/s/s390x

msgfmt: gconv_builtin.c:70: __gconv_get_builtin_trans: Assertion `cnt < sizeof (map) / sizeof (map[0])' failed.

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 v2 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

On 6/24/21 2:16 PM, Andreas Schwab wrote:
> I'm now getting an assertion failure on all big-endian platforms.
> 
> https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/p/ppc
> https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/p/ppc64
> https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc/glibc:testsuite/s/s390x
> 
> msgfmt: gconv_builtin.c:70: __gconv_get_builtin_trans: Assertion `cnt < sizeof (map) / sizeof (map[0])' failed.

I'll take a look.  Were you able to figure out which of the tests threw 
this assertion?

Thanks,
Siddhesh

Re: [PATCH v2 0/6] gconv configuration parsing cleanups

[Andreas Schwab]

On Jun 24 2021, Siddhesh Poyarekar wrote:

> I'll take a look.  Were you able to figure out which of the tests threw
> this assertion?

It's msgfmt.

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 v2 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

On 6/24/21 2:53 PM, Andreas Schwab wrote:
> On Jun 24 2021, Siddhesh Poyarekar wrote:
> 
>> I'll take a look.  Were you able to figure out which of the tests threw
>> this assertion?
> 
> It's msgfmt.


Ahh, so it's failing in the installed glibc, i.e. you mean the installed 
glibc has these patches already?  I'll see if I can reproduce this in 
Fedora.

Siddhesh

Re: [PATCH v2 0/6] gconv configuration parsing cleanups

[Andreas Schwab]

On Jun 24 2021, Siddhesh Poyarekar wrote:

> Ahh, so it's failing in the installed glibc, i.e. you mean the installed
> glibc has these patches already?

Yes, the testsuite build is run with the just built glibc in the build
environment.  I couldn't reproduce the error with the testrun.sh script.

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 v2 0/6] gconv configuration parsing cleanups

[Siddhesh Poyarekar]

On 6/24/21 9:16 PM, Andreas Schwab wrote:
> On Jun 24 2021, Siddhesh Poyarekar wrote:
> 
>> Ahh, so it's failing in the installed glibc, i.e. you mean the installed
>> glibc has these patches already?
> 
> Yes, the testsuite build is run with the just built glibc in the build
> environment.  I couldn't reproduce the error with the testrun.sh script.


Me neither, even with bare elf/ld.so so that it picks the system 
gconv_path.  I've got a build going now, so I'll see what I can find.

Siddhesh