[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH x 5] warnings, some legit
From: |
Jim Meyering |
Subject: |
Re: [PATCH x 5] warnings, some legit |
Date: |
Wed, 15 Oct 2008 20:48:49 +0200 |
Eric Blake <address@hidden> wrote:
> Jim Meyering <jim <at> meyering.net> writes:
>
>> At first I was tempted to simply ignore any freopen failure,
>> thinking that it was so unlikely as to be truly ignorable.
>> And in addition, most are guarded by "if (O_BINARY &&...",
>> so wouldn't even be compiled on systems I care about.
>> However, there are actually many ways in which freopen can
>> fail, so I wrote xfreopen and used that everywhere.
Thanks for the quick feedback.
Sorry I'm not as quick with yours.
> I'll have to give it a whirl on cygwin, since that is affected most by the
> freopen uses. For that matter, at one point I thought about adding a gnulib
> module to make reopening a stream in binary mode a little less painful; the
> current freopen trick has to query whether stdout is in append mode and choose
> between "w" and "a" accordingly when reopening stdout.
That sounds like the way to go.
Cygwin cares, even if most others don't.
So I'll table my xfreopen changes.
>> --- a/src/copy.h
>> +++ b/src/copy.h
>> @@ -221,6 +221,12 @@ struct cp_options
>> ? lstat (Src_name, Src_sb) \
>> : stat (Src_name, Src_sb))
>>
>> +static inline void
>> +ignore_value (int i ATTRIBUTE_UNUSED)
>
> Is this something worth putting in system.h, or even in a gnulib module,
> rather
> than buried in copy.h?
Yes, I guess it belongs in a module.
Maybe ignore-retval? If you're interested, go ahead.
Otherwise, I'll do it.
>> +#define TMP (char *) "/tmp"
>
> gcc doesn't warn about this use? Oh well, whatever works.
>
>> +void
>> +xfreopen (char const *filename, char const *mode, FILE *fp)
>> +{
>> + if (!freopen (filename, mode, fp))
>> + error (exit_failure, errno, _("failed to reopen %s"), quote (filename));
>
> Bug. filename is allowed to be NULL (most often the case with stdin/stdout).
Good catch.
That diagnostic should probably mention the mode, too.
I guess if the filename is NULL a good diagnostic
would determine if FP is stdin, stderr, or stdout
and use the corresponding name. But you probably do that
already in your cygwin-specific patches.
>> --- a/src/cat.c
>> +++ b/src/cat.c
>> @@ -39,6 +39,7 @@
>> #include "full-write.h"
>> #include "quote.h"
>> #include "safe-read.h"
>> +#include "xfreopen.h"
>>
>> /* The official name of this program (e.g., no `g' prefix). */
>> #define PROGRAM_NAME "cat"
>> @@ -664,7 +665,7 @@ main (int argc, char **argv)
>> {
>> file_open_mode |= O_BINARY;
>> if (O_BINARY && ! isatty (STDOUT_FILENO))
>> - freopen (NULL, "wb", stdout);
>> + xfreopen (NULL, "wb", stdout);
>
> Yep, this is one of those places where a blind freopen(NULL,"wb",stdout) is
> wrong if stdout is originally in append mode, and where you would expose the
> xfreopen bug. I need to revisit some of my cygwin-specific patches and push
> them upstream.
Thanks!
- [PATCH x 5] warnings, some legit, Jim Meyering, 2008/10/15
- Re: [PATCH x 5] warnings, some legit, Eric Blake, 2008/10/15
- Re: [PATCH x 5] warnings, some legit,
Jim Meyering <=
- Re: [PATCH x 5] warnings, some legit, Paul Eggert, 2008/10/16
- Re: [PATCH x 5] warnings, some legit, Jim Meyering, 2008/10/16