Re: [PATCH] locale: Fix localedef exit code [BZ #22292]

[Florian Weimer <fweimer at redhat dot com>]

On 10/13/2017 10:24 AM, Carlos O'Donell wrote:

> To fix this situation I have adopted the following high-level
> changes:
> * All errors are counted distinctly.
> * All warnings are counted distinctly.
> * All informative messages are not counted.
> * Increasing verbosity cannot generate*more*  errors, and
>    it previously did for errors conditional on verbose,
>    this is now fixed.
> * Increasing verbosity*can*  generate*more*  warnings.
> * Making the output quiet cannot generate*less*  errors,

“fewer errors“ (in case this makes it into the commit message.

The general approach of the patch looks okay to me.  I expect that one 
day, we might use the support/ infrastructure, potentially compiled for 
the host, in these programs as well, but that's not a current priority.

> @@ -1602,10 +1600,10 @@ collate_finish (struct localedef_t *locale, const struct charmap_t *charmap)
>   		  {
>   		    if (runp->weights[i].w[j]->weights == NULL)
>   		      {
> -			WITH_CUR_LOCALE (error_at_line (0, 0, runp->file,
> -							runp->line,
> -							_("symbol `%s' not defined"),
> -							runp->weights[i].w[j]->name));
> +			record_error_at_line (0, 0, runp->file,
> +					      runp->line,
> +					      _("symbol `%s' not defined"),
> +					      runp->weights[i].w[j]->name);

runp->line fits on the preceding line (similar occurrences below).

> @@ -1830,7 +1830,7 @@ symbol `%s' has the same encoding as"), (*eptr)->name);
>   	  /* This seems not to be enforced by recent standards.  Don't
>   	     emit an error, simply append UNDEFINED at the end.  */
>   	  if (0)
> -	    WITH_CUR_LOCALE (error (0, 0, _("no definition of `UNDEFINED'")));
> +	    record_error (0, 0, _("no definition of `UNDEFINED'"));

Either turn this into a pure comment, or into a real warning.

> diff --git a/locale/programs/linereader.h b/locale/programs/linereader.h
> index 3965db5..2623802 100644
> --- a/locale/programs/linereader.h
> +++ b/locale/programs/linereader.h

> -#define lr_error(lr, fmt, args...) \
> -  WITH_CUR_LOCALE (error_at_line (0, 0, lr->fname, lr->lineno, fmt, ## args))
> -
> +static inline void
> +lr_error (struct linereader *lr, const char *fmt, ...)
> +{
> +  char *str;
> +  va_list arg;
> +  struct locale_state ls;
> +  va_start (arg, fmt);
> +  ls = push_locale ();
> +  vasprintf (&str, fmt, arg);
> +  pop_locale (ls);
> +  va_end (arg);
> +  error_at_line (0, 0, lr->fname, lr->lineno, str);
> +  free (str);
> +}

Format string bug: This needs to to use error_at_line (0, 0, lr->fname, 
lr->lineno, "%s", str).  Missing error checking, and missing format 
string attribute.

> diff --git a/locale/programs/record-status.h b/locale/programs/record-status.h
> new file mode 100644
> index 0000000..8701f8c
> --- /dev/null
> +++ b/locale/programs/record-status.h
> @@ -0,0 +1,144 @@
> +/* General definitions for recording error and warning status.
> +   Copyright (C) 1998-2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published
> +   by the Free Software Foundation; version 2 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   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<http://www.gnu.org/licenses/>.  */
> +#ifndef _RECORD_STATUS_H
> +#define _RECORD_STATUS_H 1

Missing empty line.

> +/* The program using these functions must define these:  */
> +extern int recorded_warning_count;
> +extern int recorded_error_count;
> +extern int be_quiet;
> +extern int verbose;

You could turn these into tentative definitions (and update the 
comment).  Then the main program does not have to define them.

> +/* Saved state of the current locale.  */
> +struct locale_state
> +{
> +   const char *cur_locale;
> +};

Drop the const for the change below.

> +/* Alter the current locale to match the locale configured by the
> +   user, and return the previous saved state.  */
> +static inline struct locale_state
> +push_locale (void)
> +{
> +  int saved_errno = errno;
> +  const char *cl = setlocale (LC_CTYPE, NULL);
> +  setlocale (LC_CTYPE, "");
> +  errno = saved_errno;
> +  return (struct locale_state) { .cur_locale = cl };
> +}
> +
> +/* Use the saved state to restore the locale.  */
> +static inline void
> +pop_locale (struct locale_state ls)
> +{
> +  setlocale (LC_CTYPE, ls.cur_locale);
> +}

The locale string returned by setlocale is only valid until the next 
setlocale call, I think, so you need to use strdup and free for the 
locale string.  strdup and setlocale need error checking.  Maybe 
introduce xsetlocale?

> +/* Wrapper to print verbose informative messages.  */
> +static inline void
> +record_verbose (FILE *stream, const char *format, ...)
> +{
> +  char *str;
> +  va_list arg;
> +
> +  if (!verbose)
> +    return;
> +
> +  if (!be_quiet)
> +    {
> +      struct locale_state ls;
> +      va_start (arg, format);
> +      ls = push_locale ();
> +      vasprintf (&str, format, arg);
> +      pop_locale (ls);
> +      va_end (arg);
> +      fprintf (stream, str);
> +      free (str);
> +    }
> +}

These functions should not be inline and have printf/nonnull attributes.

fprintf has a format string bug, should use fputs.  vasprintf needs 
error checking.

> +
> +/* Wrapper to print warning messages.  We keep track of how
> +   many were called because this effects our exit code.
> +   Program must provide a definition of recorded_warning_count,
> +   and be_quiet.  */
> +static inline void
> +record_warning (int status, int errnum, const char *format, ...)
> +{
> +  char *str;
> +  va_list arg;
> +  recorded_warning_count++;
> +  if (!be_quiet)
> +    {
> +      struct locale_state ls;
> +      va_start (arg, format);
> +      ls = push_locale ();
> +      vasprintf (&str, format, arg);
> +      pop_locale (ls);
> +      va_end (arg);
> +      error (status, errnum, str);
> +      free (str);
> +    }
> +}

The status/errnum parameters are unused and should be removed (status in 
particularly is conceptually unusable).

error has a format string bug, should use error (status, errnum, "%s", str).

> +/* Wrapper to print error messages.  We keep track of how
> +   many were called because this effects our exit code.
> +   Program must provide a definition of recorded_error_count
> +   and be_quiet.  */
> +static inline void
> +record_error (int status, int errnum, const char *format, ...)
> +{
> +  char *str;
> +  va_list arg;
> +  recorded_error_count++;
> +  if (!be_quiet)
> +    {
> +      struct locale_state ls;
> +      va_start (arg, format);
> +      ls = push_locale ();
> +      vasprintf (&str, format, arg);
> +      pop_locale (ls);
> +      va_end (arg);
> +      error (status, errnum, str);
> +      free (str);
> +    }

The condition needs to be !be_quiet || status != 0, otherwise be_quiet 
also disables fatal errors.  This applies to record_error_at_line, too.

Thanks,
Florian