This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[review v4] localedef: Add verbose messages for failure paths.
- From: "Adhemerval Zanella (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>
- To: Carlos O'Donell <carlos at redhat dot com>, libc-alpha at sourceware dot org
- Cc: Florian Weimer <fweimer at redhat dot com>, Simon Marchi <simon dot marchi at polymtl dot ca>
- Date: Mon, 16 Dec 2019 12:29:33 -0500
- Subject: [review v4] localedef: Add verbose messages for failure paths.
- Auto-submitted: auto-generated
- References: <gerrit.1571944987000.I28b9f680711ff00252a2cb15625b774cc58ecb9d@gnutoolchain-gerrit.osci.io>
- Reply-to: gnutoolchain-gerrit at osci dot io
Adhemerval Zanella has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303
......................................................................
Patch Set 4:
(13 comments)
| --- /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:
Ok.
|
|
| 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:
Ok, it uses the translation of 'For bug reporting...' to create
another message.
| 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:
Ok.
| 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:
Wouldn't be better to use uintptr_t for the pointer arithmetic? There
is some recent discussion on gnulib-bugs about it [1].
[1] https://lists.gnu.org/archive/html/bug-
gnulib/2019-12/msg00104.html
| + free ((char *) normal);
PS4, Line 522:
I feel a bit confusing an interface that returns a 'const char *' and
requires to explicit deallocate the memory with free, since it
requires the called to explicit remove the const qualifier with a
cast. But this is something we might fix later.
| }
| 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:
Ok.
|
| /* 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)
...
| @@ -559,17 +545,35 @@ 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:
Is it the only possible meaningful error for this case?
| + }
|
| 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:
s/cahracters/characters
| + - 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:
Ok.
| 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:
It already for failed allocation for xasprintf, wouldn't it be simpler
to do the same here?
|
| 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:
Ok.
|
|
| /* 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. */
| --- /dev/null
| +++ locale/programs/xasprintf.c
| @@ -1,0 +26,9 @@ xasprintf (const char *format, ...)
| +{
| + va_list ap;
| + va_start (ap, format);
| + char *result;
| + if (vasprintf (&result, format, ap) < 0)
| + error (EXIT_FAILURE, 0, _("memory exhausted"));
| + va_end (ap);
| + return result;
| +}
--
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: Mon, 16 Dec 2019 17:29:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment