DCIGETTEXT: Remove alloca support

Message ID 20170619161817.17415402AEC0E@oldenburg.str.redhat.com
State Committed
Headers

Commit Message

Florian Weimer June 19, 2017, 4:18 p.m. UTC
  2017-06-19  Florian Weimer  <fweimer@redhat.com>

	* intl/dcigettext.c: Remove alloca support.
	(DCIGETTEXT): Use heap allocations for xdomainname, single_locale.
  

Comments

Adhemerval Zanella June 21, 2017, 7:50 p.m. UTC | #1
On 19/06/2017 13:18, Florian Weimer wrote:
> 2017-06-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	* intl/dcigettext.c: Remove alloca support.
> 	(DCIGETTEXT): Use heap allocations for xdomainname, single_locale.

I think this could be another possible usage of char_array, but to safely 
use it would require to slight change '_nl_find_domain' to avoid tamper
the 'locale' argument and making a copy otherwise (current approach seems
a bad idiom imho, where it allocates a buffer in some cases and redirect
to input argument).  So I am not sure if it worth the trouble, the only
meaningful in this case is a slight better performance due less heap
usage for general cases.

So patch LGTM, but I can send a variant which uses char_array if you
prefer as well.


> 
> diff --git a/intl/dcigettext.c b/intl/dcigettext.c
> index 0e79b1f..d778d19 100644
> --- a/intl/dcigettext.c
> +++ b/intl/dcigettext.c
> @@ -27,28 +27,6 @@
>  
>  #include <sys/types.h>
>  
> -#ifdef __GNUC__
> -# define alloca __builtin_alloca
> -# define HAVE_ALLOCA 1
> -#else
> -# ifdef _MSC_VER
> -#  include <malloc.h>
> -#  define alloca _alloca
> -# else
> -#  if defined HAVE_ALLOCA_H || defined _LIBC
> -#   include <alloca.h>
> -#  else
> -#   ifdef _AIX
> - #pragma alloca
> -#   else
> -#    ifndef alloca
> -char *alloca ();
> -#    endif
> -#   endif
> -#  endif
> -# endif
> -#endif
> -
>  #include <errno.h>
>  #ifndef errno
>  extern int errno;
> @@ -374,45 +352,6 @@ static const char *get_output_charset (struct binding *domainbinding)
>  #endif
>  
>  
> -/* For those losing systems which don't have `alloca' we have to add
> -   some additional code emulating it.  */
> -#ifdef HAVE_ALLOCA
> -/* Nothing has to be done.  */
> -# define freea(p) /* nothing */
> -# define ADD_BLOCK(list, address) /* nothing */
> -# define FREE_BLOCKS(list) /* nothing */
> -#else
> -struct block_list
> -{
> -  void *address;
> -  struct block_list *next;
> -};
> -# define ADD_BLOCK(list, addr)						      \
> -  do {									      \
> -    struct block_list *newp = (struct block_list *) malloc (sizeof (*newp));  \
> -    /* If we cannot get a free block we cannot add the new element to	      \
> -       the list.  */							      \
> -    if (newp != NULL) {							      \
> -      newp->address = (addr);						      \
> -      newp->next = (list);						      \
> -      (list) = newp;							      \
> -    }									      \
> -  } while (0)
> -# define FREE_BLOCKS(list)						      \
> -  do {									      \
> -    while (list != NULL) {						      \
> -      struct block_list *old = list;					      \
> -      list = list->next;						      \
> -      free (old->address);						      \
> -      free (old);							      \
> -    }									      \
> -  } while (0)
> -# undef alloca
> -# define alloca(size) (malloc (size))
> -# define freea(p) free (p)
> -#endif	/* have alloca */
> -
> -
>  #ifdef _LIBC
>  /* List of blocks allocated for translations.  */
>  typedef struct transmem_list
> @@ -488,17 +427,14 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>  	    int plural, unsigned long int n, int category)
>  #endif
>  {
> -#ifndef HAVE_ALLOCA
> -  struct block_list *block_list = NULL;
> -#endif
>    struct loaded_l10nfile *domain;
>    struct binding *binding;
>    const char *categoryname;
>    const char *categoryvalue;
>    const char *dirname;
>    char *xdirname = NULL;
> -  char *xdomainname;
> -  char *single_locale;
> +  char *xdomainname = NULL;
> +  char *single_locale = NULL;
>    char *retval;
>    size_t retlen;
>    int saved_errno;
> @@ -507,7 +443,6 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>  #if defined HAVE_PER_THREAD_LOCALE && !defined IN_LIBGLOCALE
>    const char *localename;
>  #endif
> -  size_t domainname_len;
>  
>    /* If no real MSGID is given return NULL.  */
>    if (msgid1 == NULL)
> @@ -537,6 +472,7 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>       definition left this undefined.  */
>    if (domainname == NULL)
>      domainname = _nl_current_default_domain;
> +  size_t domainname_len = strlen (domainname);
>  
>    /* OS/2 specific: backward compatibility with older libintl versions  */
>  #ifdef LC_MESSAGES_COMPAT
> @@ -652,19 +588,16 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>    categoryvalue = guess_category_value (category, categoryname);
>  #endif
>  
> -  domainname_len = strlen (domainname);
> -  xdomainname = (char *) alloca (strlen (categoryname)
> -				 + domainname_len + 5);
> -  ADD_BLOCK (block_list, xdomainname);
> -
> -  stpcpy ((char *) mempcpy (stpcpy (stpcpy (xdomainname, categoryname), "/"),
> -			    domainname, domainname_len),
> -	  ".mo");
> -
>    /* Creating working area.  */
> -  single_locale = (char *) alloca (strlen (categoryvalue) + 1);
> -  ADD_BLOCK (block_list, single_locale);
> +  single_locale = malloc (strlen (categoryvalue) + 1);
>  
> +  if (single_locale == NULL
> +      || __asprintf (&xdomainname, "%s/%s.mo", categoryname, domainname) < 0)
> +    {
> +      free (single_locale);
> +      free (xdirname);
> +      return NULL;
> +    }
>  
>    /* Search for the given string.  This is a loop because we perhaps
>       got an ordered list of languages to consider for the translation.  */
> @@ -752,7 +685,8 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>  	      /* Found the translation of MSGID1 in domain DOMAIN:
>  		 starting at RETVAL, RETLEN bytes.  */
>  	      free (xdirname);
> -	      FREE_BLOCKS (block_list);
> +	      free (xdomainname);
> +	      free (single_locale);
>  	      if (foundp == NULL)
>  		{
>  		  /* Create a new entry and add it to the search tree.  */
> @@ -836,7 +770,8 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>   return_untranslated:
>    /* Return the untranslated MSGID.  */
>    free (xdirname);
> -  FREE_BLOCKS (block_list);
> +  free (xdomainname);
> +  free (single_locale);
>    gl_rwlock_unlock (_nl_state_lock);
>  #ifdef _LIBC
>    __libc_rwlock_unlock (__libc_setlocale_lock);
>
  

Patch

diff --git a/intl/dcigettext.c b/intl/dcigettext.c
index 0e79b1f..d778d19 100644
--- a/intl/dcigettext.c
+++ b/intl/dcigettext.c
@@ -27,28 +27,6 @@ 
 
 #include <sys/types.h>
 
-#ifdef __GNUC__
-# define alloca __builtin_alloca
-# define HAVE_ALLOCA 1
-#else
-# ifdef _MSC_VER
-#  include <malloc.h>
-#  define alloca _alloca
-# else
-#  if defined HAVE_ALLOCA_H || defined _LIBC
-#   include <alloca.h>
-#  else
-#   ifdef _AIX
- #pragma alloca
-#   else
-#    ifndef alloca
-char *alloca ();
-#    endif
-#   endif
-#  endif
-# endif
-#endif
-
 #include <errno.h>
 #ifndef errno
 extern int errno;
@@ -374,45 +352,6 @@  static const char *get_output_charset (struct binding *domainbinding)
 #endif
 
 
-/* For those losing systems which don't have `alloca' we have to add
-   some additional code emulating it.  */
-#ifdef HAVE_ALLOCA
-/* Nothing has to be done.  */
-# define freea(p) /* nothing */
-# define ADD_BLOCK(list, address) /* nothing */
-# define FREE_BLOCKS(list) /* nothing */
-#else
-struct block_list
-{
-  void *address;
-  struct block_list *next;
-};
-# define ADD_BLOCK(list, addr)						      \
-  do {									      \
-    struct block_list *newp = (struct block_list *) malloc (sizeof (*newp));  \
-    /* If we cannot get a free block we cannot add the new element to	      \
-       the list.  */							      \
-    if (newp != NULL) {							      \
-      newp->address = (addr);						      \
-      newp->next = (list);						      \
-      (list) = newp;							      \
-    }									      \
-  } while (0)
-# define FREE_BLOCKS(list)						      \
-  do {									      \
-    while (list != NULL) {						      \
-      struct block_list *old = list;					      \
-      list = list->next;						      \
-      free (old->address);						      \
-      free (old);							      \
-    }									      \
-  } while (0)
-# undef alloca
-# define alloca(size) (malloc (size))
-# define freea(p) free (p)
-#endif	/* have alloca */
-
-
 #ifdef _LIBC
 /* List of blocks allocated for translations.  */
 typedef struct transmem_list
@@ -488,17 +427,14 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
 	    int plural, unsigned long int n, int category)
 #endif
 {
-#ifndef HAVE_ALLOCA
-  struct block_list *block_list = NULL;
-#endif
   struct loaded_l10nfile *domain;
   struct binding *binding;
   const char *categoryname;
   const char *categoryvalue;
   const char *dirname;
   char *xdirname = NULL;
-  char *xdomainname;
-  char *single_locale;
+  char *xdomainname = NULL;
+  char *single_locale = NULL;
   char *retval;
   size_t retlen;
   int saved_errno;
@@ -507,7 +443,6 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
 #if defined HAVE_PER_THREAD_LOCALE && !defined IN_LIBGLOCALE
   const char *localename;
 #endif
-  size_t domainname_len;
 
   /* If no real MSGID is given return NULL.  */
   if (msgid1 == NULL)
@@ -537,6 +472,7 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
      definition left this undefined.  */
   if (domainname == NULL)
     domainname = _nl_current_default_domain;
+  size_t domainname_len = strlen (domainname);
 
   /* OS/2 specific: backward compatibility with older libintl versions  */
 #ifdef LC_MESSAGES_COMPAT
@@ -652,19 +588,16 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
   categoryvalue = guess_category_value (category, categoryname);
 #endif
 
-  domainname_len = strlen (domainname);
-  xdomainname = (char *) alloca (strlen (categoryname)
-				 + domainname_len + 5);
-  ADD_BLOCK (block_list, xdomainname);
-
-  stpcpy ((char *) mempcpy (stpcpy (stpcpy (xdomainname, categoryname), "/"),
-			    domainname, domainname_len),
-	  ".mo");
-
   /* Creating working area.  */
-  single_locale = (char *) alloca (strlen (categoryvalue) + 1);
-  ADD_BLOCK (block_list, single_locale);
+  single_locale = malloc (strlen (categoryvalue) + 1);
 
+  if (single_locale == NULL
+      || __asprintf (&xdomainname, "%s/%s.mo", categoryname, domainname) < 0)
+    {
+      free (single_locale);
+      free (xdirname);
+      return NULL;
+    }
 
   /* Search for the given string.  This is a loop because we perhaps
      got an ordered list of languages to consider for the translation.  */
@@ -752,7 +685,8 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
 	      /* Found the translation of MSGID1 in domain DOMAIN:
 		 starting at RETVAL, RETLEN bytes.  */
 	      free (xdirname);
-	      FREE_BLOCKS (block_list);
+	      free (xdomainname);
+	      free (single_locale);
 	      if (foundp == NULL)
 		{
 		  /* Create a new entry and add it to the search tree.  */
@@ -836,7 +770,8 @@  DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
  return_untranslated:
   /* Return the untranslated MSGID.  */
   free (xdirname);
-  FREE_BLOCKS (block_list);
+  free (xdomainname);
+  free (single_locale);
   gl_rwlock_unlock (_nl_state_lock);
 #ifdef _LIBC
   __libc_rwlock_unlock (__libc_setlocale_lock);