diff mbox series

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

Message ID 20210610111853.2286873-5-siddhesh@sourceware.org
State New
Headers show
Series gconv configuration parsing cleanups | expand

Commit Message

Siddhesh Poyarekar June 10, 2021, 11:18 a.m. UTC
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(-)

Comments

DJ Delorie June 22, 2021, 4:01 a.m. UTC | #1
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.
Szabolcs Nagy July 2, 2021, 9:11 a.m. UTC | #2
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
> 

--
Szabolcs Nagy July 2, 2021, 9:27 a.m. UTC | #3
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
diff mbox series

Patch

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'",