localedef: improve error message [BZ #16985]

  If you pass in a path that fails to be opened, then output_path is set to
NULL, and an error is flagged.  Then at the end, we use both of those:
	cannot write output files to `(null)': No such file or directory

Tweak the message to use the user's input when output_path is NULL.

2015-08-29  Mike Frysinger  <vapier@gentoo.org>

	[BZ #16985]
	* programs/localedef.c (main): Display argv[remaining] when
	output_path is NULL.
---
 locale/programs/localedef.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

ping ...
-mike

On 29 Aug 2015 16:39, Mike Frysinger wrote:
> If you pass in a path that fails to be opened, then output_path is set to
> NULL, and an error is flagged.  Then at the end, we use both of those:
> 	cannot write output files to `(null)': No such file or directory
> 
> Tweak the message to use the user's input when output_path is NULL.
> 
> 2015-08-29  Mike Frysinger  <vapier@gentoo.org>
> 
> 	[BZ #16985]
> 	* programs/localedef.c (main): Display argv[remaining] when
> 	output_path is NULL.
> ---
>  locale/programs/localedef.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
> index 2a0f2aa..06fca12 100644
> --- a/locale/programs/localedef.c
> +++ b/locale/programs/localedef.c
> @@ -286,7 +286,7 @@ cannot open locale definition file `%s'"), runp->name));
>      {
>        if (cannot_write_why != 0)
>  	WITH_CUR_LOCALE (error (4, cannot_write_why, _("\
> -cannot write output files to `%s'"), output_path));
> +cannot write output files to `%s'"), output_path ? : argv[remaining]));
>        else
>  	write_all_categories (locales, charmap, argv[remaining], output_path);
>      }
> -- 
> 2.5.0
  

On Sat, Aug 29, 2015 at 1:39 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> If you pass in a path that fails to be opened, then output_path is set to
> NULL, and an error is flagged.  Then at the end, we use both of those:
>         cannot write output files to `(null)': No such file or directory
>
> Tweak the message to use the user's input when output_path is NULL.
>
> 2015-08-29  Mike Frysinger  <vapier@gentoo.org>
>
>         [BZ #16985]
>         * programs/localedef.c (main): Display argv[remaining] when
>         output_path is NULL.
> ---
>  locale/programs/localedef.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
> index 2a0f2aa..06fca12 100644
> --- a/locale/programs/localedef.c
> +++ b/locale/programs/localedef.c
> @@ -286,7 +286,7 @@ cannot open locale definition file `%s'"), runp->name));
>      {
>        if (cannot_write_why != 0)
>         WITH_CUR_LOCALE (error (4, cannot_write_why, _("\
> -cannot write output files to `%s'"), output_path));
> +cannot write output files to `%s'"), output_path ? : argv[remaining]));
>        else
>         write_all_categories (locales, charmap, argv[remaining], output_path);
>      }

Looks good to me.

Thanks.
  

I'm not following exactly what the case is.  It looks to me like the only
cases where construct_output_path can return NULL is when asprintf fails or
when mkdir fails.  The asprintf case really should just be an immediate
failure as xmalloc would do.  When mkdir fails, I'm not entirely clear on
why we don't just die immediately.  I guess the idea is that we can
diagnose input errors even if we will never succeed in writing the output.
I wonder how worthwhile that really is.
  

On 09/11/2015 02:27 PM, Roland McGrath wrote:
> I'm not following exactly what the case is.  It looks to me like the only
> cases where construct_output_path can return NULL is when asprintf fails or
> when mkdir fails.  The asprintf case really should just be an immediate
> failure as xmalloc would do.  When mkdir fails, I'm not entirely clear on
> why we don't just die immediately.  I guess the idea is that we can
> diagnose input errors even if we will never succeed in writing the output.
> I wonder how worthwhile that really is.

I haven't looked at the code but based on the description I can
reproduce the poor error message by passing the name of a non-
existent directory as an operand to the command. E.g., like so:

$ localedef -f localedata/charmaps/UTF-8 -i localedata/locales/en_US /foobar
cannot write output files to `(null)': Permission denied

Presumably, after applying the patch the message will reference
the name of the directory that the command cannot write to:

cannot write output files to `/foobar': Permission denied

Martin
  

> I haven't looked at the code [...]

I'm talking about the organization of the code.
The reproducer case was clear enough.
  

On 11 Sep 2015 14:17, Roland McGrath wrote:
> > I haven't looked at the code [...]
> 
> I'm talking about the organization of the code.
> The reproducer case was clear enough.

i agree the code looks dicey at best
-mike