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

[DJ Delorie <dj@redhat.com>]

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>