[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
Related show

Commit Message

Pali Rohár via Libc-alpha June 10, 2021, 11:18 a.m.
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(-)

-- 
2.31.1

Comments

Pali Rohár via Libc-alpha June 22, 2021, 4:01 a.m. | #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.
Pali Rohár via Libc-alpha July 2, 2021, 9:11 a.m. | #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

> 


--
Pali Rohár via Libc-alpha July 2, 2021, 9:27 a.m. | #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

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