Re: [PATCH x 5] warnings, some legit

[Jim Meyering]

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!