[review v4] 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 4:

(9 comments)

I rewrote part of the handling of result to avoid the in-place editing like you suggested. I also noticed another memory leak. The normalized paths need to be freed by the caller and they were not (just like the tp leak). This is ready for review again.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +10,11 @@ During testing of localedef running in a minimal container
| +there were several error cases which were hard to diagnose
| +since they appeared as strerror (errno) values printed by
| +the higher level functions.  This change adds three new
| +verbose messages for potential failure paths.  The new
| +messages give the user the opportunity to use -v and display
| +additional information about why localedef might be failing.
| +I found these messages useful myself while writing a localedef
| +container test for --no-hard-links.
| +
| +Signed-off-by: Carlos O'Donell <carlos@redhat.com>

PS1, Line 19:

Done

| +Change-Id: I28b9f680711ff00252a2cb15625b774cc58ecb9d
| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -523,14 +523,18 @@ construct_output_path (char *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;
| +	{
| +	  record_verbose (stderr,
| +			  _("failed to allocate space for compiled locale path"));

PS1, Line 531:

Done

| +	  return NULL;
| +	}
|  
|        endp = result + n - 1;
|      }
|    else
|      {
|        /* This is a user path.  Please note the additional byte in the
|  	 memory allocation.  */

 ...

| @@ -556,7 +558,19 @@ construct_output_path (char *path)
| +	  if (mkdir (result, 0777) < 0)
| +	    {
| +	      record_verbose (stderr,
| +			      _("cannot create output path \"%s\": %s"),
| +			      result, strerror (errno));
| +	      return NULL;
| +	    }
| +	}
| +      else
| +	record_verbose (stderr, _("no write permission to output path: %s"),

PS1, Line 567:

This is resolved. I added the output path.

| +			strerror (errno));
| +    }
|  
|    *endp++ = '/';
|    *endp = '\0';
|  
|    return result;
|  }
|  
| --- /dev/null
| +++ include/programs/xasprintf.h
PS2:

I like having it distinct. Just a preference.

| @@ -1,0 +12,12 @@ /* asprintf with out of memory checking
| +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
| +   GNU General Public License for more details.
| +
| +   You should have received a copy of the GNU General Public License
| +   along with this program; if not, see <https://www.gnu.org/licenses/>.  */
| +
| +#ifndef _XASPRINTF_H
| +#define _XASPRINTF_H	1
| +
| +extern int xasprintf (char **string_ptr, const char *format, ...);

PS2, Line 21:

Done

| +
| +#endif /* xasprintf.h */
| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -443,18 +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);
|        return cp;

PS2, Line 446:

Done

|      default:
|        break;
|      }
|    return (char *) text;
|  }
|  
|  /* Print the version information.  */
|  static void
|  print_version (FILE *stream, struct argp_state *state)

 ...

| @@ -556,11 +550,19 @@ construct_output_path (char *path)
| +			      result, strerror (errno));
| +	      return NULL;
| +	    }
| +	}
| +      else
| +	record_verbose (stderr, _("no write permission to output path: %s"),
| +			strerror (errno));
| +    }
|  
|    *endp++ = '/';

PS2, Line 559:

Done

|    *endp = '\0';
|  
|    return result;
|  }
|  
|  
|  /* Normalize codeset name.  There is no standard for the codeset
|     names.  Normalization allows the user to use any of the common
|     names.  */
| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -429,16 +429,15 @@ more_help (int key, const char *text, void *input)
|  {
|    char *cp;
|    char *tp;
|  
|    switch (key)
|      {
|      case ARGP_KEY_HELP_EXTRA:
|        /* We print some extra information.  */
| -      if (asprintf (&tp, gettext ("\
| +      xasprintf (&tp, gettext ("\

PS3, Line 437:

Done

|  For bug reporting instructions, please see:\n\
| -%s.\n"), REPORT_BUGS_TO) < 0)
| -	return NULL;
| -      if (asprintf (&cp, gettext ("\
| +%s.\n"), REPORT_BUGS_TO);
| +      xasprintf (&cp, gettext ("\
|  System's directory for character maps : %s\n\
|  		       repertoire maps: %s\n\
|  		       locale path    : %s\n\

 ...

| @@ -523,16 +518,13 @@ construct_output_path (char *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;
| +	n = xasprintf (&result, "%s%s/%.*s%s%s%c",
| +		       output_prefix ?: "", COMPLOCALEDIR,
| +		       (int) (startp - path), path, normal, endp, '\0');

PS3, Line 521:

Done

|  
|        endp = result + n - 1;
|      }
|    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);

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I28b9f680711ff00252a2cb15625b774cc58ecb9d
Gerrit-Change-Number: 303
Gerrit-PatchSet: 4
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: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 10 Dec 2019 21:18:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Florian Weimer <fweimer@redhat.com>
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment