Message ID | 1440880793-32082-1-git-send-email-vapier@gentoo.org |
---|---|
State | Committed |
Delegated to: | Mike Frysinger |
Headers |
Received: (qmail 80872 invoked by alias); 29 Aug 2015 20:40:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 80834 invoked by uid 89); 29 Aug 2015 20:40:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: smtp.gentoo.org From: Mike Frysinger <vapier@gentoo.org> To: libc-alpha@sourceware.org Subject: [PATCH] localedef: improve error message [BZ #16985] Date: Sat, 29 Aug 2015 16:39:53 -0400 Message-Id: <1440880793-32082-1-git-send-email-vapier@gentoo.org> |
Commit Message
Mike Frysinger
Aug. 29, 2015, 8:39 p.m. UTC
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(-)
Comments
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
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); }