[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:

(5 comments)

Reviewed Adhemerval's comments. Patchset 5 coming up.

| --- locale/programs/localedef.c
| +++ locale/programs/localedef.c
| @@ -526,14 +518,9 @@ construct_output_path (char *path)
| -		      (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:

It must be of type (int) since that's what printf(3) says is the width
specifier type in the next argument of the variable argument list.

Note that the discussion on gnulib-bugs is specifically about using
signed types like (int), and avoiding unsigned types [1] for
subscripts and sizes.

[1] http://www.open-
std.org/jtc1/sc22/wg21/docs/papers/2019/p1428r0.pdf

| +      free ((char *) normal);

PS4, Line 522:

I actually fixed this, then undid it because I didn't want to expand
the scope of the patch, but I think I'll just clean this up because I
won't be back to look at this code for a while.

Fixed in next version.

|      }
|    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.  */

 ...

| @@ -559,14 +545,32 @@ construct_output_path (char *path)
| +			      _("cannot create output path \"%s\": %s"),
| +			      result, strerror (errno));
| +	      free (result);
| +	      return NULL;
| +	    }
| +	}
| +      else
| +	record_verbose (stderr,
| +			_("no write permission to output path \"%s\": %s"),
| +			result, strerror (errno));

PS4, Line 554:

We aren't writing to the archive, so the result has to be path, and
euidaccess returned -1 when we asked to write to it. So the error
message relates to the operation localedef was attempting to do.

e.g.
[verbose] no write permission to output path
"/home/carlos/build/glibc-gr-localedef/tmp/en_US.UTF-8/": Not a
directory

We distinguish only the ENOENT case because we might be able to do
something about that. In theory we could handle all forms of errors
returned by stat64, but instead of doing that we print errno so you'll
know if it's EACCESS, EBADF, ELOOP, ENAMETOOLONG, ENOMEM, etc. etc.

My suggestion is to leave the current message as-is, it represents
what localedef was _trying_ to do and let the user look at the printed
strerror(errno) to determine why that action (writing) was prevented.

Thoughts?

| +    }
|  
|    return result;
|  }
|  
|  
| -/* 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:

Fixed in next version.

| +   - 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.  */
|  static const char *
|  normalize_codeset (const char *codeset, size_t name_len)
|  {
|    int len = 0;
|    int only_digit = 1;
|    char *retval;

 ...

| @@ -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:

Fixed in next version.

|  
|    if (retval != NULL)
|      {
|        if (only_digit)
|  	wp = stpcpy (retval, "iso");
|        else
|  	wp = retval;
|  
|        for (cnt = 0; cnt < name_len; ++cnt)

-- 
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: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 17 Dec 2019 03:03:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-MessageType: comment