[review v5] localedef: Add verbose messages for failure paths.

["Carlos O'Donell (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303
......................................................................


Patch Set 5:

(11 comments)

There is a bug in using 'en_US.UTF-8@tEsT' style entries which I need to fix. I'm off by the computation of the length in the xasprtinf because UTF-8 is normalized to one byte less e.g. utf8 and I need to do the math to fix that.

| --- /dev/null
| +++ include/programs/xasprintf.h
| @@ -1,0 +16,9 @@ /* asprintf with out of memory checking
| +   along with this program; if not, see <https://www.gnu.org/licenses/>.  */
| +
| +#ifndef _XASPRINTF_H
| +#define _XASPRINTF_H	1
| +
| +extern char *xasprintf (const char *format, ...)
| +    __attribute__ ((__format__ (__printf__, 1, 2), __warn_unused_result__));
| +
| +#endif /* xasprintf.h */
| --- locale/Makefile
| +++ locale/Makefile
| @@ -51,18 +51,18 @@ vpath %.h programs
|  vpath %.gperf programs
|  
|  localedef-modules	:= localedef $(categories:%=ld-%) \
|  			   charmap linereader locfile \
|  			   repertoire locarchive
|  localedef-aux		:= md5
|  locale-modules		:= locale locale-spec
|  lib-modules		:= charmap-dir simple-hash xmalloc xstrdup \
| -			   record-status
| +			   record-status xasprintf

PS4, Line 59:

Done

|  
|  
|  GPERF = gperf
|  GPERFFLAGS = -acCgopt -k1,2,5,9,$$ -L ANSI-C
|  
|  ifeq ($(run-built-tests),yes)
|  tests-special += $(objpfx)tst-locale-locpath.out
|  endif
|  
| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -443,17 +442,14 @@ System's directory for character maps : %s\n\
|  		       repertoire maps: %s\n\
|  		       locale path    : %s\n\
|  %s"),
| -		    CHARMAP_PATH, REPERTOIREMAP_PATH, LOCALE_PATH, tp) < 0)
| -	{
| -	  free (tp);
| -	  return NULL;
| -	}
| +		    CHARMAP_PATH, REPERTOIREMAP_PATH, LOCALE_PATH, tp);
| +      free (tp);

PS4, Line 446:

Done

|        return cp;
|      default:
|        break;
|      }
|    return (char *) text;
|  }
|  
|  /* Print the version information.  */
|  static void

 ...

| @@ -511,38 +512,25 @@ construct_output_path (char *path)
|  	}
| -      else
| -	/* This is to keep gcc quiet.  */
| -	endp = NULL;
| -
| -      /* We put an additional '\0' at the end of the string because at
| -	 the end of the function we need another byte for the trailing
| -	 '/'.  */
| -      ssize_t n;
| +

PS4, Line 513:

Done

|        if (normal == NULL)
| -	n = asprintf (&result, "%s%s/%s%c", output_prefix ?: "",
| -		      COMPLOCALEDIR, path, '\0');
| +	result = xasprintf ("%s%s/%s/", output_prefix ?: "",
| +			    COMPLOCALEDIR, path);
|        else
| -	n = asprintf (&result, "%s%s/%.*s%s%s%c",
| -		      output_prefix ?: "", COMPLOCALEDIR,
| -		      (int) (startp - path), path, normal, endp, '\0');
| -
| -      if (n < 0)
| -	return NULL;
| -
| -      endp = result + n - 1;
| +	result = xasprintf ("%s%s/%.*s%s%s/",
| +			    output_prefix ?: "", COMPLOCALEDIR,
| +			    (int) (startp - path), path, normal, endp ?: "");
| +      /* Free the allocated normalized codeset name.  */

PS4, Line 521:

Done

| +      free ((char *) normal);

PS4, Line 522:

Done

|      }
|    else
|      {
| -      /* This is a user path.  Please note the additional byte in the
| -	 memory allocation.  */
| -      size_t len = strlen (path) + 1;
| -      result = xmalloc (len + 1);
| -      endp = mempcpy (result, path, len) - 1;
| +      /* This is a user path.  */
| +      result = xasprintf ("%s/", path);

PS4, Line 527:

Done

|  
|        /* If the user specified an output path we cannot add the output
|  	 to the archive.  */
|        no_archive = true;
|      }
|  
|    errno = 0;
|  
|    if (no_archive && euidaccess (result, W_OK) == -1)

 ...

| @@ -564,12 +561,19 @@ construct_output_path (char *path)
| -/* Normalize codeset name.  There is no standard for the codeset
| -   names.  Normalization allows the user to use any of the common
| -   names.  */
| +/* Normalize codeset name.  There is no standard for the codeset names.
| +   Normalization allows the user to use any of the common names e.g. UTF-8,
| +   utf-8, utf8, UTF8 etc.
| +
| +   We normalize using the following rules:
| +   - Remove all non-alpha-numeric characters
| +   - Lowercase all cahracters.

PS4, Line 567:

Done

| +   - If there are only digits assume it's an ISO standard and prefix with 'iso'
| +
| +   We return the normalized string which needs to be freed by free.  */

PS4, Line 570:

Done

|  static const char *
|  normalize_codeset (const char *codeset, size_t name_len)
|  {
|    int len = 0;
|    int only_digit = 1;
|    char *retval;
|    char *wp;
|    size_t cnt;
|  

 ...

| @@ -578,17 +583,19 @@ normalize_codeset (const char *codeset, size_t name_len)
|        {
|  	++len;
|  
|  	if (isalpha (codeset[cnt]))
|  	  only_digit = 0;
|        }
|  
| +  /* If there were only digits we assume it's an ISO standard and we will
| +     prefix with 'iso' so include space for that.  */
|    retval = (char *) malloc ((only_digit ? 3 : 0) + len + 1);

PS4, Line 592:

Done

|  
|    if (retval != NULL)
|      {
|        if (only_digit)
|  	wp = stpcpy (retval, "iso");
|        else
|  	wp = retval;
|  
|        for (cnt = 0; cnt < name_len; ++cnt)
| --- locale/programs/localedef.h
| +++ locale/programs/localedef.h
| @@ -117,18 +117,19 @@ /* Global variables of the localedef program.  */
|  extern const char *repertoire_global;
|  extern int max_locarchive_open_retry;
|  extern bool no_archive;
|  extern const char *alias_file;
|  extern bool hard_links;
|  
|  
|  /* Prototypes for a few program-wide used functions.  */
|  #include <programs/xmalloc.h>
| +#include <programs/xasprintf.h>

PS4, Line 126:

Done

|  
|  
|  /* Mark given locale as to be read.  */
|  extern struct localedef_t *add_to_readlist (int locale, const char *name,
|  					    const char *repertoire_name,
|  					    int generate,
|  					    struct localedef_t *copy_locale);
|  
|  /* Find the information for the locale NAME.  */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I28b9f680711ff00252a2cb15625b774cc58ecb9d
Gerrit-Change-Number: 303
Gerrit-PatchSet: 5
Gerrit-Owner: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 17 Dec 2019 03:29:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment