Gnulib's freopen replacement and MinGW

[Eli Zaretskii]

Diffutils 3.2 call xfreopen with its first argument NULL, expecting
the underlying reopen to handle this. However, the MS runtime does
not implement the Posix semantics of such a call, and so, for example,
MinGW-compiled cmp fails when invoked to compare its stdin with a
file:

D:\gnu\diffutils-3.2\src>cat cmp.c | cmp cmp.c -
cmp: failed to reopen `stdin' with mode `rb': No such file or directory

The following change fixes this:

2012-05-05 Eli Zaretskii <el...@gnu.org>

* lib/freopen.c [_WIN32]: Include io.h and fcntl.h.
(rpl_freopen) [_WIN32]: If the first argument is NULL, call
_setmode to switch STREAM to either binary or text mode, as
specified by MODE.

--- lib/freopen.c~0 2011-09-02 01:35:11.000000000 +0300
+++ lib/freopen.c 2012-05-05 13:46:36.389000000 +0300
@@ -37,11 +37,23 @@ orig_freopen (const char *filename, cons

#include <string.h>

+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+#include <io.h>
+#include <fcntl.h>
+#endif
+
FILE *
rpl_freopen (const char *filename, const char *mode, FILE *stream)
{
#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
- if (filename != NULL && strcmp (filename, "/dev/null") == 0)
+ if (filename == NULL)
+ {
+ if (strchr (mode, 'b'))
+ return _setmode (_fileno (stream), _O_BINARY) == -1 ? NULL : stream;
+ else
+ return _setmode (_fileno (stream), _O_TEXT) == -1 ? NULL : stream;
+ }
+ else if (strcmp (filename, "/dev/null") == 0)
filename = "NUL";
#endif

[Bruno Haible]

Hi Eli,

> Diffutils 3.2 call xfreopen with its first argument NULL, expecting
> the underlying reopen to handle this. However, the MS runtime does
> not implement the Posix semantics of such a call, and so, for example,
> MinGW-compiled cmp fails when invoked to compare its stdin with a
> file:
>
> D:\gnu\diffutils-3.2\src>cat cmp.c | cmp cmp.c -
> cmp: failed to reopen `stdin' with mode `rb': No such file or directory
>
> The following change fixes this:
>
> 2012-05-05 Eli Zaretskii <el...@gnu.org>
>
> * lib/freopen.c [_WIN32]: Include io.h and fcntl.h.
> (rpl_freopen) [_WIN32]: If the first argument is NULL, call
> _setmode to switch STREAM to either binary or text mode, as
> specified by MODE.

Thanks for the proposed patch, but the problem has already been fixed
differently, by avoiding to use xfreopen() [1].

The question whether to extend gnulib's freopen() to support this use-case
was discussed in the thread [2][3]. The result of that discussion was "no",
because
- POSIX says that it is "implementation-defined" whether freopen()
supports this [4],
- freopen with a NULL filename is not supported on many Unix platforms,
- there is no point for the caller to use the freopen() API when the
same effect can be achieved with <binary-io.h>.

Bruno

[1] http://git.savannah.gnu.org/gitweb/?p=diffutils.git;a=commitdiff;h=7508234eabe646abcea074baea83612f8115d3b9
[2] http://lists.gnu.org/archive/html/bug-gnulib/2011-08/msg00224.html
[3] http://lists.gnu.org/archive/html/bug-gnulib/2011-09/msg00005.html
[4] http://pubs.opengroup.org/onlinepubs/9699919799/functions/freopen.html

[Eli Zaretskii]

> From: Bruno Haible <br...@clisp.org>
> Cc: bug-gn...@gnu.org
> Date: Sat, 05 May 2012 16:26:55 +0200

>
> > Diffutils 3.2 call xfreopen with its first argument NULL, expecting
> > the underlying reopen to handle this. However, the MS runtime does
> > not implement the Posix semantics of such a call, and so, for example,
> > MinGW-compiled cmp fails when invoked to compare its stdin with a
> > file:
> >
> > D:\gnu\diffutils-3.2\src>cat cmp.c | cmp cmp.c -
> > cmp: failed to reopen `stdin' with mode `rb': No such file or directory
> >
> > The following change fixes this:
> >
> > 2012-05-05 Eli Zaretskii <el...@gnu.org>
> >
> > * lib/freopen.c [_WIN32]: Include io.h and fcntl.h.
> > (rpl_freopen) [_WIN32]: If the first argument is NULL, call
> > _setmode to switch STREAM to either binary or text mode, as
> > specified by MODE.
>
> Thanks for the proposed patch, but the problem has already been fixed
> differently, by avoiding to use xfreopen() [1].

Thanks, that's good to know.

[Eric Blake]

On 05/05/2012 05:18 AM, Eli Zaretskii wrote:
> Diffutils 3.2 call xfreopen with its first argument NULL, expecting
> the underlying reopen to handle this. However, the MS runtime does
> not implement the Posix semantics of such a call,

POSIX says that such a call is implementation-defined, so technically,
mingw DOES implement the POSIX semantics (by defining that it is
unsupported). However, you are correct that it is annoying, and
something that gnulib could work around for easier coding.

> The following change fixes this:
>
> 2012-05-05 Eli Zaretskii <el...@gnu.org>
>
> * lib/freopen.c [_WIN32]: Include io.h and fcntl.h.
> (rpl_freopen) [_WIN32]: If the first argument is NULL, call
> _setmode to switch STREAM to either binary or text mode, as
> specified by MODE.
>

> --- lib/freopen.c~0 2011-09-02 01:35:11.000000000 +0300
> +++ lib/freopen.c 2012-05-05 13:46:36.389000000 +0300
> @@ -37,11 +37,23 @@ orig_freopen (const char *filename, cons
>
> #include <string.h>
>
> +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> +#include <io.h>
> +#include <fcntl.h>
> +#endif
> +
> FILE *
> rpl_freopen (const char *filename, const char *mode, FILE *stream)
> {
> #if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> - if (filename != NULL && strcmp (filename, "/dev/null") == 0)
> + if (filename == NULL)
> + {
> + if (strchr (mode, 'b'))
> + return _setmode (_fileno (stream), _O_BINARY) == -1 ? NULL : stream;
> + else
> + return _setmode (_fileno (stream), _O_TEXT) == -1 ? NULL : stream;

Shouldn't we be doing some sanity checking, as in ensuring that a
read-only stream is not being reopened with 'w', or a write-only stream
is not being reopened with 'r'? Also, shouldn't we be honoring 'w' vs.
'a' and adjusting the underlying fd's O_APPEND bit accordingly?

--
Eric Blake ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[Eli Zaretskii]

> Date: Mon, 07 May 2012 06:44:00 -0600
> From: Eric Blake <ebl...@redhat.com>
> CC: bug-g...@gnu.org, bug-gn...@gnu.org

>
> > FILE *
> > rpl_freopen (const char *filename, const char *mode, FILE *stream)
> > {
> > #if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
> > - if (filename != NULL && strcmp (filename, "/dev/null") == 0)
> > + if (filename == NULL)
> > + {
> > + if (strchr (mode, 'b'))
> > + return _setmode (_fileno (stream), _O_BINARY) == -1 ? NULL : stream;
> > + else
> > + return _setmode (_fileno (stream), _O_TEXT) == -1 ? NULL : stream;
>
> Shouldn't we be doing some sanity checking, as in ensuring that a
> read-only stream is not being reopened with 'w', or a write-only stream
> is not being reopened with 'r'?

It's desirable, but I don't know how to do that with MinGW. There's
no F_GETFL or its equivalent, AFAIK (but I'd be glad to learn that I'm
wrong).

> Also, shouldn't we be honoring 'w' vs.
> 'a' and adjusting the underlying fd's O_APPEND bit accordingly?

Again, I don't know how to do that. According to MSDN, _setmode
supports only O_BINARY and O_TEXT. If the reality is different,
please tell.

(One thing that is missing in my patch is a call to fflush, since you
cannot switch modes in the middle of a buffered operation.)

[Bruno Haible]

Eric Blake wrote:
> POSIX says that such a call is implementation-defined, so technically,
> mingw DOES implement the POSIX semantics (by defining that it is
> unsupported).

Yes. So, code in freopen.c that would try to support a NULL argument
in all cases
- would be a gnulib extension,
- would not be usable to change the read vs. write vs. append mode,
because on many systems we would need to fiddle with FILE internals,
which is hard and not reliable,
- would be only useful to change the text vs. binary mode. But for this
purpose gnulib already offers a simpler API: binary-io.h.

> However, you are correct that it is annoying, and
> something that gnulib could work around for easier coding.

I don't agree. It's not easier coding to write
xfreopen (NULL, "rb", fp);
instead of
SET_BINARY (fileno (fp));

Bruno

[Eric Blake]

[adding coreutils]

Waaaay back when, coreutils used to use SET_BINARY. We changed that to
use freopen(NULL) because of the POSIX wording that allowed NULL to
change mode, because cygwin honored the mode change, and because Paul
preferred using a POSIX interface over a complete extension interface.

Commit 8770c00ef, Jul 2005
https://lists.gnu.org/archive/html/bug-coreutils/2005-07/msg00059.html

Also, at one point, I proposed a patch to xfreopen, to which Bruno
counterproposed a new API fchangemode() that would alter a FILE* to be
binary without the baggage of freopen(NULL), although I never really
pursued it:

https://lists.gnu.org/archive/html/bug-gnulib/2010-03/msg00111.html

Now it looks like we've come full circle, and that the POSIX wording of
freopen(NULL) is so broken as to not be worth using via gnulib, and
we're back to preferring extension interfaces in the form of SET_BINARY.
It looks like several projects will have to change, coreutils included,
if we indeed decide that use of freopen(NULL) is no longer desirable.

[Bruno Haible]

Hi Eric,

Thank you for digging out the previous threads on this topic.

Eric Blake wrote:
> Waaaay back when, coreutils used to use SET_BINARY. We changed that to
> use freopen(NULL) because of the POSIX wording that allowed NULL to
> change mode, because cygwin honored the mode change, and because Paul
> preferred using a POSIX interface over a complete extension interface.
>
> Commit 8770c00ef, Jul 2005
> https://lists.gnu.org/archive/html/bug-coreutils/2005-07/msg00059.html
>
> Also, at one point, I proposed a patch to xfreopen, to which Bruno
> counterproposed a new API fchangemode() that would alter a FILE* to be
> binary without the baggage of freopen(NULL), although I never really
> pursued it:
>
> https://lists.gnu.org/archive/html/bug-gnulib/2010-03/msg00111.html
>
> Now it looks like we've come full circle, and that the POSIX wording of
> freopen(NULL) is so broken as to not be worth using via gnulib

Yes. Aside the issue with O_TEXT / O_BINARY being out of scope of POSIX,
there is also the issue with the file position. As I understand it,
freopen (NULL, ...) shall - if it succeeds - set the file position back
to 0. Citing POSIX [1]:

"even though the 'b' is ignored, a successful call to
freopen (NULL, "wb", stdout) does have an effect. In particular, for
regular files it truncates the file and sets the file-position
indicator for the stream to the start of the file."

Similarly, for stdin, a successful call to freopen (NULL, "rb", stdin)
must rewind the read-position of stdin to 0. Thus when used in a program
'prog', in the command
{ dd of=/dev/null count=1; prog; } < regular_file
'prog' will start reading from the beginning of regular_file, as if the
dd command was not present. I think this is unintended. Most coreutils
will *just* want to switch to binary mode without rewinding.

So, I continue to be in favour of APIs which don't need to fiddle with
FILE internals and with the file position. Like the proposed [2] fsetbinary()
for O_BINARY, or an fchangemode() that supports at least O_RDONLY, O_WRONLY,
O_RDWR, O_APPEND, O_TEXT, O_BINARY.

Bruno

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/freopen.html
[2] https://lists.gnu.org/archive/html/bug-gnulib/2010-03/msg00111.html