GNU bug report logs - #18006
Simplify via set_binary_mode

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 18006 in the body.
You can then email your comments to 18006 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Emacs bug reports and feature requests <bug-gnu-emacs <at> gnu.org>
Subject: Simplify via set_binary_mode
Date: Sat, 12 Jul 2014 13:52:28 -0700

[Message part 1 (text/plain, inline)]

Tags: patch

Attached is a proposed minor simplification to have Emacs use 
set_binary_mode more consistently.  I'm filing this as a bug report to 
give Eli a heads-up, as it affects the Microsoft ports.  This is 
relative to trunk bzr 117525.

[binary-io.patch (text/plain, attachment)]

Message #8 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Sun, 13 Jul 2014 07:20:26 +0300

> Date: Sat, 12 Jul 2014 13:52:28 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> 
> Attached is a proposed minor simplification to have Emacs use 
> set_binary_mode more consistently.  I'm filing this as a bug report to 
> give Eli a heads-up, as it affects the Microsoft ports.  This is 
> relative to trunk bzr 117525.

Thanks.

However, I don't understand what is the purpose of this patch.  If we
want to clean up uses of setmode and related issues, I'm all for it
(some of that code is old and unnecessary).  But then the Gnulib
binary-io module is not the answer, or at least not all of it.  Some
of the reasons:

  . in some places, we want all file I/O to be in binary mode; Gnulib
    doesn't support that

  . some of the code needs to switch a file descriptor to binary or
    text mode only when the descriptor is or isn't connected to a
    terminal device; Gnulib is ambivalent about that (it always does
    that for MSDOS, and never for Windows)

  . AFAIK, Gnulib's binary-io also replaces isatty, which is not
    really needed in Emacs (you don't show the patches for lib/ so I
    can only guess)

  . we don't want in Emacs the msvc-nothrow wrappers for library
    functions

Some specific comments about the patch:

> @@ -155,20 +148,12 @@
>  
>        if (un_flag)
>  	{
> -	  char buf[18];
> +	  set_binary_mode (fileno (stdout), O_BINARY);
>  
> -#ifdef DOS_NT
> -#if (__DJGPP__ >= 2) || (defined WINDOWSNT)
> -          if (!isatty (fileno (stdout)))
> -	    setmode (fileno (stdout), O_BINARY);
> -#else
> -	  (stdout)->_flag &= ~_IOTEXT; /* print binary */
> -	  _setmode (fileno (stdout), O_BINARY);
> -#endif
> -#endif

We no longer support DJGPP < 2, so the #else stuff could simply go
away.

> -#ifdef DOS_NT
> -#if (__DJGPP__ >= 2) || (defined WINDOWSNT)
> -          if (!isatty (fileno (fp)))
> -	    setmode (fileno (fp), O_BINARY);
> -#else
> -	  (fp)->_flag &= ~_IOTEXT; /* read binary */
> -	  _setmode (fileno (fp), O_BINARY);
> -#endif
> -#endif

Likewise.

>    /* Don't put CRs in the DOC file.  */
> -#ifdef MSDOS
> -  _fmode = O_BINARY;
> -#if 0  /* Suspicion is that this causes hanging.
> -	  So instead we require people to use -o on MSDOS.  */
> -  (stdout)->_flag &= ~_IOTEXT;
> -  _setmode (fileno (stdout), O_BINARY);
> -#endif
> -  outfile = 0;
> -#endif /* MSDOS */
> -#ifdef WINDOWSNT
> -  _fmode = O_BINARY;
> -  _setmode (fileno (stdout), O_BINARY);
> -#endif /* WINDOWSNT */
> +  set_binary_mode (fileno (stdout), O_BINARY);

This is wrong: setting _fmode affects all I/O, input and output, not
just stdout.  Gnulib's binary-io doesn't have the equivalent
functionality.

> @@ -239,11 +237,8 @@
>    /* Manipulate tty.  */
>    if (hide_char)
>      {
> -      emacs_get_tty (fileno (stdin), &etty);
> -#ifdef WINDOWSNT
> -      if (isatty (fileno (stdin)))
> -	_setmode (fileno (stdin), O_BINARY);
> -#endif
> +      if (emacs_get_tty (fileno (stdin), &etty) == 0)
> +	set_binary_mode (fileno (stdin), O_BINARY);
>        suppress_echo_on_tty (fileno (stdin));

This logic is flawed: if emacs_get_tty failed, then emacs_set_tty
should not be called as well.

>  #endif	/* WINDOWSNT */
> +  return isatty (fd) - 1;

This will not work with Windows isatty, because it doesn't return 1
when fd is connected to a console.

To summarize, if we want to clean up that code, I'm willing to do the
job.  Bug Gnulib's binary-io is not the answer.

Thanks.

Message #11 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Sat, 12 Jul 2014 22:28:56 -0700

[Message part 1 (text/plain, inline)]

Eli Zaretskii wrote:

>    . in some places, we want all file I/O to be in binary mode; Gnulib
>      doesn't support that

The Emacs code can continue to use _fmode as before; that part won't be 
simplified, but one step at a time.

>    . some of the code needs to switch a file descriptor to binary or
>      text mode only when the descriptor is or isn't connected to a
>      terminal device; Gnulib is ambivalent about that (it always does
>      that for MSDOS, and never for Windows)

binary-io has two interfaces; one (set_binary_mode) always does it, on 
all platforms; the other (SET_BINARY) deliberately has no effect on 
__DJGPP__ when isatty returns nonzero.  Emacs can use either interface, 
as it needs.  Should SET_BINARY also should have no effect on MS-Windows 
when isatty returns nonzero?  If so, I suppose we can change SET_BINARY 
to do that.

>    . AFAIK, Gnulib's binary-io also replaces isatty

It doesn't, so this shouldn't be a problem.  (Emacs already uses 
binary-io, by the way; if this were a problem I expect we'd already have 
run into it.)

>    . we don't want in Emacs the msvc-nothrow wrappers for library
>      functions

binary-io doesn't do that either.

>>     /* Don't put CRs in the DOC file.  */
>> -#ifdef MSDOS
>> -  _fmode = O_BINARY;
>> -#if 0  /* Suspicion is that this causes hanging.
>> -	  So instead we require people to use -o on MSDOS.  */
>> -  (stdout)->_flag &= ~_IOTEXT;
>> -  _setmode (fileno (stdout), O_BINARY);
>> -#endif
>> -  outfile = 0;
>> -#endif /* MSDOS */
>> -#ifdef WINDOWSNT
>> -  _fmode = O_BINARY;
>> -  _setmode (fileno (stdout), O_BINARY);
>> -#endif /* WINDOWSNT */
>> +  set_binary_mode (fileno (stdout), O_BINARY);
>
> This is wrong: setting _fmode affects all I/O, input and output, not
> just stdout.  Gnulib's binary-io doesn't have the equivalent
> functionality.

If setting _fmode affects all I/O, why does the WINDOWSNT code bother to 
call _setmode?  Conversely, why does make-docfile.c need to set _fmode, 
if the intent is "Don't put CRs in the DOC file"; shouldn't it suffice 
to call _setmode on stdout?

> This logic is flawed: if emacs_get_tty failed, then emacs_set_tty
> should not be called as well.

I was just copying the existing logic.  But it's easy to fix while we're 
at it; revised patch attached.

> This will not work with Windows isatty, because it doesn't return 1
> when fd is connected to a console.

Thanks, I also tried to address that in the attached patch.

[binary-io.patch (text/plain, attachment)]

Message #14 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Sun, 13 Jul 2014 18:02:21 +0300

> Date: Sat, 12 Jul 2014 22:28:56 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 18006 <at> debbugs.gnu.org
> 
> >    . in some places, we want all file I/O to be in binary mode; Gnulib
> >      doesn't support that
> 
> The Emacs code can continue to use _fmode as before; that part won't be 
> simplified, but one step at a time.

See below.

> >    . some of the code needs to switch a file descriptor to binary or
> >      text mode only when the descriptor is or isn't connected to a
> >      terminal device; Gnulib is ambivalent about that (it always does
> >      that for MSDOS, and never for Windows)
> 
> binary-io has two interfaces; one (set_binary_mode) always does it, on 
> all platforms; the other (SET_BINARY) deliberately has no effect on 
> __DJGPP__ when isatty returns nonzero.  Emacs can use either interface, 
> as it needs.

OK.

> Should SET_BINARY also should have no effect on MS-Windows 
> when isatty returns nonzero?

No, the SET_BINARY macro does TRT here.

> Emacs already uses binary-io, by the way; if this were a problem I
> expect we'd already have run into it.

We don't currently use binary-io on Windows.

> >>     /* Don't put CRs in the DOC file.  */
> >> -#ifdef MSDOS
> >> -  _fmode = O_BINARY;
> >> -#if 0  /* Suspicion is that this causes hanging.
> >> -	  So instead we require people to use -o on MSDOS.  */
> >> -  (stdout)->_flag &= ~_IOTEXT;
> >> -  _setmode (fileno (stdout), O_BINARY);
> >> -#endif
> >> -  outfile = 0;
> >> -#endif /* MSDOS */
> >> -#ifdef WINDOWSNT
> >> -  _fmode = O_BINARY;
> >> -  _setmode (fileno (stdout), O_BINARY);
> >> -#endif /* WINDOWSNT */
> >> +  set_binary_mode (fileno (stdout), O_BINARY);
> >
> > This is wrong: setting _fmode affects all I/O, input and output, not
> > just stdout.  Gnulib's binary-io doesn't have the equivalent
> > functionality.
> 
> If setting _fmode affects all I/O, why does the WINDOWSNT code bother to 
> call _setmode?

Because setting _fmode only affects the open/fopen calls made _after_
the change.  It cannot affect standard streams that are opened before
'main' runs.

However, I think that we should simply use "rb", "wb", etc. in the
calls to 'fopen', and forget all this _fmode stuff.

(Btw, are there still standard C libraries we care about that don't
support "rb" and "wb"?  If not, we could use these on all platforms,
without any #ifdefs.)

> Conversely, why does make-docfile.c need to set _fmode, 
> if the intent is "Don't put CRs in the DOC file"; shouldn't it suffice 
> to call _setmode on stdout?

The comment is inaccurate: it actually doesn't want to put CRs in any
output files, not just DOC.

> > This logic is flawed: if emacs_get_tty failed, then emacs_set_tty
> > should not be called as well.
> 
> I was just copying the existing logic.  But it's easy to fix while we're 
> at it; revised patch attached.
> 
> > This will not work with Windows isatty, because it doesn't return 1
> > when fd is connected to a console.
> 
> Thanks, I also tried to address that in the attached patch.

Thanks.  I have one comment:

> +int
>  emacs_get_tty (int fd, struct emacs_tty *settings)
>  {
>    /* Retrieve the primary parameters - baud rate, character size, etcetera.  */
> @@ -786,15 +787,16 @@
>    HANDLE h = (HANDLE)_get_osfhandle (fd);
>    DWORD console_mode;
>  
> -  if (h && h != INVALID_HANDLE_VALUE)
> +  if (h && h != INVALID_HANDLE_VALUE && GetConsoleMode (h, &console_mode))
>      {
> -      if (GetConsoleMode (h, &console_mode))
> -	settings->main = console_mode;
> +      settings->main = console_mode;
> +      return 0;
>      }
>  #endif	/* WINDOWSNT */
> +  return isatty (fd) - 1;

Do we need this "-1" part now?  It could misfire if isatty does
return 1 some day.

Message #17 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Sun, 13 Jul 2014 19:11:54 -0700

[Message part 1 (text/plain, inline)]

Eli Zaretskii wrote:
>> +  return isatty (fd) - 1;
> Do we need this "-1" part now?  It could misfire if isatty does
> return 1 some day.

Sorry, I don't so how it can misfire.  isatty returns 1 on success, 0 on 
failure; whereas the caller returns 0 on success, -1 on failure.  So 
subtracting 1 is the right thing to do, no?

A revised patch, taking your other comments into account, is attached. 
It's relative to trunk bzr 117527.

[binary-io.patch (text/plain, attachment)]

Message #20 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Mon, 14 Jul 2014 18:21:49 +0300

> Date: Sun, 13 Jul 2014 19:11:54 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 18006 <at> debbugs.gnu.org
> 
> Eli Zaretskii wrote:
> 
> >  +  return isatty (fd) - 1;
> > 
>  Do we need this "-1" part now?  It could misfire if isatty does
>  return 1 some day.
> 
> Sorry, I don't so how it can misfire. isatty returns 1 on success, 0
> on failure; whereas the caller returns 0 on success, -1 on
> failure. So subtracting 1 is the right thing to do, no?

The MinGW isatty returns a non-zero value for any character device
(e.g., the null device), not just for the console.  GetConsoleMode
fails for anything that isn't a console, so the code will fall through
to this line, and return something other than -1.

In addition, even when the descriptor _is_ connected to a console,
isatty returns a value that is not 1.

You already return zero for success, so I think it is better to use an
explicit

  return -1;

here.

Or did I miss something?

> A revised patch, taking your other comments into account, is
> attached. It's relative to trunk bzr 117527.

Thanks.  This goes farther than I intended (I didn't intend to remove
_fmode from src/ files), but I guess it's good to get rid of _fmode
there as well.  It does trigger a couple more comments, though:

> === modified file 'src/process.c'
> --- src/process.c	2014-07-08 17:13:32 +0000
> +++ src/process.c	2014-07-14 02:02:15 +0000
> @@ -88,6 +88,7 @@
>  #include <pty.h>
>  #endif
> 
> +#include <binary-io.h>
>  #include <c-ctype.h>
>  #include <sig2str.h>
>  #include <verify.h>
> @@ -3142,6 +3143,8 @@
>  	  continue;
>  	}
> 
> +      set_binary_mode (s, O_BINARY);

This and other similar changes in process.c are unnecessary: sockets
don't need to be switched to binary mode.  Moreover, the file
descriptor returned by 'sys_socket' (a wrapper for 'socket') on
MS-Windows is not used for any actual I/O, it is used as a key for
looking up socket handles recorded in a table maintained by w32.c.

Of course, if you want to have this for consistency, it cannot do any
harm in this case.

>  /* Open FILE for Emacs use, using open flags OFLAG and mode MODE.
> +   Use binary I/O on systems that care about text vs binary I/O.
>     Arrange for subprograms to not inherit the file descriptor.
>     Prefer a method that is multithread-safe, if available.
>     Do not fail merely because the open was interrupted by a signal.
> @@ -2207,7 +2210,7 @@
>  emacs_open (const char *file, int oflags, int mode)
>  {
>    int fd;
> -  oflags |= O_CLOEXEC;
> +  oflags |= O_BINARY | O_CLOEXEC;
>    while ((fd = open (file, oflags, mode)) < 0 && errno == EINTR)

This is not quite right, as it effectively disallows opening a file in
text mode (lread.c, xfaces.c, and termcap.c use that feature).  It's
probably my fault: I failed to mention that _fmode controls only the
_default_ open mode, which can still be overridden by an explicit
O_BINARY or O_TEXT flag.

So the above addition of O_BINARY should be conditioned on O_TEXT not
being set in OFLAGS.

Otherwise, the patch looks good to me.

Thanks.

Message #23 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Mon, 14 Jul 2014 13:16:06 -0700

[Message part 1 (text/plain, inline)]

Eli Zaretskii wrote:
> Otherwise, the patch looks good to me.

Thanks; I installed it after fixing things up as per your comments.

Your explanation of _fmode led me to find a minor porting bug. 
Fx_load_color_file [!HAVE_X_WINDOWS] uses fopen with "rt"; unlike "rb" 
this isn't specified by POSIX and a web search suggests that it does 
fail on a few older platforms.  Emacs no longer modifies _fmode so "r" 
should suffice now anyway.  Also, there are two other places where some 
"rt"-related simplifications can be done.  Proposed further patch attached.

[binary-io.patch (text/plain, attachment)]

Message #26 received at 18006 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 18006 <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Tue, 15 Jul 2014 17:37:40 +0300

> Date: Mon, 14 Jul 2014 13:16:06 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 18006 <at> debbugs.gnu.org
> 
> Your explanation of _fmode led me to find a minor porting bug. 
> Fx_load_color_file [!HAVE_X_WINDOWS] uses fopen with "rt"; unlike "rb" 
> this isn't specified by POSIX and a web search suggests that it does 
> fail on a few older platforms.  Emacs no longer modifies _fmode so "r" 
> should suffice now anyway.  Also, there are two other places where some 
> "rt"-related simplifications can be done.  Proposed further patch attached.

Thanks.  The lib-src part looks OK to me.

> --- src/ChangeLog	2014-07-14 19:23:18 +0000
> +++ src/ChangeLog	2014-07-14 19:46:54 +0000
> @@ -1,5 +1,10 @@
>  2014-07-14  Paul Eggert  <eggert <at> cs.ucla.edu>
>  
> +	Use "b" flag more consistently; avoid "t" (Bug#18006).
> +	* lread.c (Fload) [DOS_NT]:
> +	* xfaces.c (Fx_load_color_file) [!HAVE_X_WINDOWS]:
> +	No longer need to use "rt" instead of "r".

This part doesn't look right.  If we don't use "rt", then when
emacs_fopen calls emacs_open, the latter will interpret the lack of
"t" as a sign to use the default binary mode, which is not what we
want.

I think we can fix this in one of 2 ways: either (a) let emacs_fopen
start with O_TEXT in bmode by default, at least on DOS_NT platforms;
or (b) change emacs_open back to not apply O_BINARY by default, and
instead use "rb", "wb", and O_BINARY in all the places except those
that use "rt" or "wt" now.

I like the latter alternative better, because the former makes
emacs_fopen and emacs_open use 2 different defaults (text vs binary),
which is contrary to intuition and easy to forget.

WDYT?

Message #31 received at 18006-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 18006-done <at> debbugs.gnu.org
Subject: Re: bug#18006: Simplify via set_binary_mode
Date: Tue, 15 Jul 2014 12:39:36 -0700

Eli Zaretskii wrote:
> The lib-src part looks OK to me.

Thanks, I installed that.

> This part doesn't look right.  If we don't use "rt", then when
> emacs_fopen calls emacs_open

Ah, sorry, that part was a false alarm.  I forgot that emacs_fopen 
doesn't pass the mode string to fopen, which means there's no 
portability bug there after all.  If we were going to make a change I'd 
prefer your (a) to your (b) as it keeps the mainline code simpler, but 
doing nothing at all is simpler yet, so I'll close the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.