bug#18006: Simplify via set_binary_mode

bug#18006: Simplify via set_binary_mode

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

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.

[-- Attachment #2: binary-io.patch --]
[-- Type: text/plain, Size: 6935 bytes --]

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2014-06-30 00:01:51 +0000
+++ admin/ChangeLog	2014-07-12 20:47:50 +0000
@@ -1,3 +1,9 @@
+2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module.
+	* merge-gnulib (GNULIB_MODULES): Add binary-io.  It was already
+	present implicitly; this just makes the dependence explicit.
+
 2014-06-30  Glenn Morris  <rgm@gnu.org>
 
 	* update_autogen: Find loaddefs targets rather than

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2014-05-17 08:11:31 +0000
+++ admin/merge-gnulib	2014-07-12 20:47:50 +0000
@@ -26,7 +26,7 @@
 GNULIB_URL=git://git.savannah.gnu.org/gnulib.git
 
 GNULIB_MODULES='
-  alloca-opt byteswap c-ctype c-strcase
+  alloca-opt binary-io byteswap c-ctype c-strcase
   careadlinkat close-stream count-one-bits count-trailing-zeros
   crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
   dtoastr dtotimespec dup2 environ execinfo faccessat

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-07-12 17:53:29 +0000
+++ lib-src/ChangeLog	2014-07-12 20:47:50 +0000
@@ -1,5 +1,10 @@
 2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Use binary-io module.
+	* hexl.c, make-docfile.c:
+	Include binary-io.h instead of fcntl.h and/or io.h.
+	(main): Use set_binary_mode in place of handcrafted code.
+
 	* etags.c (Lisp_functions): Also record cl-defun etc. (Bug#17965)
 
 2014-06-26  Glenn Morris  <rgm@gnu.org>

=== modified file 'lib-src/hexl.c'
--- lib-src/hexl.c	2014-01-01 07:43:34 +0000
+++ lib-src/hexl.c	2014-07-12 20:47:50 +0000
@@ -24,15 +24,8 @@
 
 #include <stdio.h>
 #include <ctype.h>
-#ifdef DOS_NT
-#include <fcntl.h>
-#if __DJGPP__ >= 2
-#include <io.h>
-#endif
-#endif
-#ifdef WINDOWSNT
-#include <io.h>
-#endif
+
+#include <binary-io.h>
 
 #define DEFAULT_GROUPING	0x01
 #define DEFAULT_BASE		16
@@ -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
 	  for (;;)
 	    {
-	      register int i, c = 0, d;
+	      int i, c = 0, d;
+	      char buf[18];
 
 #define hexchar(x) (isdigit (x) ? x - '0' : x - 'a' + 10)
 
@@ -210,15 +195,7 @@
 	}
       else
 	{
-#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
+	  set_binary_mode (fileno (fp), O_BINARY);
 	  address = 0;
 	  string[0] = ' ';
 	  string[17] = '\0';

=== modified file 'lib-src/make-docfile.c'
--- lib-src/make-docfile.c	2014-06-17 16:09:19 +0000
+++ lib-src/make-docfile.c	2014-07-12 20:47:50 +0000
@@ -38,17 +38,16 @@
 
 #include <stdio.h>
 #include <stdlib.h>   /* config.h unconditionally includes this anyway */
-#ifdef MSDOS
-#include <fcntl.h>
-#endif /* MSDOS */
+
 #ifdef WINDOWSNT
 /* Defined to be sys_fopen in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.  */
 #undef fopen
-#include <fcntl.h>
 #include <direct.h>
 #endif /* WINDOWSNT */
 
+#include <binary-io.h>
+
 #ifdef DOS_NT
 /* Defined to be sys_chdir in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.
@@ -138,19 +137,7 @@
   outfile = stdout;
 
   /* 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);
 
   /* If first two args are -o FILE, output to FILE.  */
   i = 1;

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2014-07-12 17:53:29 +0000
+++ src/ChangeLog	2014-07-12 20:47:50 +0000
@@ -1,3 +1,11 @@
+2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module.
+	* minibuf.c: Include binary-io.h instead of fcntl.h.
+	(read_minibuf_noninteractive):
+	Use set_binary_mode instead of handcrafted code.
+	* sysdep.c, systty.h (emacs_get_tty): Return int, not void.
+
 2014-07-12  Eli Zaretskii  <eliz@gnu.org>
 
 	* xdisp.c (display_line): Don't call FETCH_BYTE with argument less

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2014-07-12 07:47:40 +0000
+++ src/minibuf.c	2014-07-12 20:47:50 +0000
@@ -22,9 +22,7 @@
 #include <errno.h>
 #include <stdio.h>
 
-#ifdef WINDOWSNT
-#include <fcntl.h>	/* For O_BINARY, O_TEXT. */
-#endif
+#include <binary-io.h>
 
 #include "lisp.h"
 #include "commands.h"
@@ -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));
     }
 
@@ -281,11 +276,8 @@
   if (hide_char)
     {
       fprintf (stdout, "\n");
-      emacs_set_tty (fileno (stdin), &etty, 0);
-#ifdef WINDOWSNT
-      if (isatty (fileno (stdin)))
-	_setmode (fileno (stdin), O_TEXT);
-#endif
+      if (emacs_set_tty (fileno (stdin), &etty, 0) == 0)
+	set_binary_mode (fileno (stdin), O_TEXT);
     }
 
   if (len || c == '\n' || c == '\r')

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2014-07-11 17:55:24 +0000
+++ src/sysdep.c	2014-07-12 20:47:50 +0000
@@ -775,8 +775,9 @@
 /* Getting and setting emacs_tty structures.  */
 
 /* Set *TC to the parameters associated with the terminal FD,
-   or clear it if the parameters are not available.  */
-void
+   or clear it if the parameters are not available.
+   Return 0 on success, -1 (setting errno) on failure.  */
+int
 emacs_get_tty (int fd, struct emacs_tty *settings)
 {
   /* Retrieve the primary parameters - baud rate, character size, etcetera.  */
@@ -792,9 +793,10 @@
 	settings->main = console_mode;
     }
 #endif	/* WINDOWSNT */
+  return isatty (fd) - 1;
 #else	/* !DOS_NT */
   /* We have those nifty POSIX tcmumbleattr functions.  */
-  tcgetattr (fd, &settings->main);
+  return tcgetattr (fd, &settings->main);
 #endif
 }
 

=== modified file 'src/systty.h'
--- src/systty.h	2014-07-11 17:55:24 +0000
+++ src/systty.h	2014-07-12 20:47:50 +0000
@@ -80,7 +80,7 @@
 };
 \f
 /* From sysdep.c or w32.c  */
-extern void emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
+extern int emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
 extern int emacs_set_tty (int, struct emacs_tty *, bool) EXTERNALLY_VISIBLE;
 extern void suppress_echo_on_tty (int);
 extern int serial_open (Lisp_Object);

bug#18006: Simplify via set_binary_mode

[Eli Zaretskii]

> Date: Sat, 12 Jul 2014 13:52:28 -0700
> From: Paul Eggert <eggert@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.

bug#18006: Simplify via set_binary_mode

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

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.

[-- Attachment #2: binary-io.patch --]
[-- Type: text/plain, Size: 7556 bytes --]

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2014-06-30 00:01:51 +0000
+++ admin/ChangeLog	2014-07-12 20:56:51 +0000
@@ -1,3 +1,9 @@
+2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module (Bug#18006).
+	* merge-gnulib (GNULIB_MODULES): Add binary-io.  It was already
+	present implicitly; this just makes the dependence explicit.
+
 2014-06-30  Glenn Morris  <rgm@gnu.org>
 
 	* update_autogen: Find loaddefs targets rather than

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2014-05-17 08:11:31 +0000
+++ admin/merge-gnulib	2014-07-12 20:47:50 +0000
@@ -26,7 +26,7 @@
 GNULIB_URL=git://git.savannah.gnu.org/gnulib.git
 
 GNULIB_MODULES='
-  alloca-opt byteswap c-ctype c-strcase
+  alloca-opt binary-io byteswap c-ctype c-strcase
   careadlinkat close-stream count-one-bits count-trailing-zeros
   crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
   dtoastr dtotimespec dup2 environ execinfo faccessat

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-07-12 17:53:29 +0000
+++ lib-src/ChangeLog	2014-07-13 05:24:43 +0000
@@ -1,5 +1,11 @@
 2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Use binary-io module (Bug#18006).
+	* hexl.c, make-docfile.c:
+	Include binary-io.h instead of fcntl.h and/or io.h.
+	(main): Use set_binary_mode or SET_BINARY
+	in place of handcrafted code.
+
 	* etags.c (Lisp_functions): Also record cl-defun etc. (Bug#17965)
 
 2014-06-26  Glenn Morris  <rgm@gnu.org>

=== modified file 'lib-src/hexl.c'
--- lib-src/hexl.c	2014-01-01 07:43:34 +0000
+++ lib-src/hexl.c	2014-07-13 05:24:43 +0000
@@ -24,15 +24,8 @@
 
 #include <stdio.h>
 #include <ctype.h>
-#ifdef DOS_NT
-#include <fcntl.h>
-#if __DJGPP__ >= 2
-#include <io.h>
-#endif
-#endif
-#ifdef WINDOWSNT
-#include <io.h>
-#endif
+
+#include <binary-io.h>
 
 #define DEFAULT_GROUPING	0x01
 #define DEFAULT_BASE		16
@@ -155,20 +148,12 @@
 
       if (un_flag)
 	{
-	  char buf[18];
+	  SET_BINARY (fileno (stdout));
 
-#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
 	  for (;;)
 	    {
-	      register int i, c = 0, d;
+	      int i, c = 0, d;
+	      char buf[18];
 
 #define hexchar(x) (isdigit (x) ? x - '0' : x - 'a' + 10)
 
@@ -210,15 +195,7 @@
 	}
       else
 	{
-#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
+	  SET_BINARY (fileno (fp));
 	  address = 0;
 	  string[0] = ' ';
 	  string[17] = '\0';

=== modified file 'lib-src/make-docfile.c'
--- lib-src/make-docfile.c	2014-06-17 16:09:19 +0000
+++ lib-src/make-docfile.c	2014-07-12 20:47:50 +0000
@@ -38,17 +38,16 @@
 
 #include <stdio.h>
 #include <stdlib.h>   /* config.h unconditionally includes this anyway */
-#ifdef MSDOS
-#include <fcntl.h>
-#endif /* MSDOS */
+
 #ifdef WINDOWSNT
 /* Defined to be sys_fopen in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.  */
 #undef fopen
-#include <fcntl.h>
 #include <direct.h>
 #endif /* WINDOWSNT */
 
+#include <binary-io.h>
+
 #ifdef DOS_NT
 /* Defined to be sys_chdir in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.
@@ -138,19 +137,7 @@
   outfile = stdout;
 
   /* 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);
 
   /* If first two args are -o FILE, output to FILE.  */
   i = 1;

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2014-07-12 17:53:29 +0000
+++ src/ChangeLog	2014-07-13 05:24:43 +0000
@@ -1,3 +1,12 @@
+2014-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module (Bug#18006).
+	* minibuf.c: Include binary-io.h instead of fcntl.h.
+	(read_minibuf_noninteractive):
+	Use set_binary_mode instead of handcrafted code.
+	Don't call emacs_set_tty if emacs_get_tty failed.
+	* sysdep.c, systty.h (emacs_get_tty): Return int, not void.
+
 2014-07-12  Eli Zaretskii  <eliz@gnu.org>
 
 	* xdisp.c (display_line): Don't call FETCH_BYTE with argument less

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2014-07-12 07:47:40 +0000
+++ src/minibuf.c	2014-07-13 05:24:43 +0000
@@ -22,9 +22,7 @@
 #include <errno.h>
 #include <stdio.h>
 
-#ifdef WINDOWSNT
-#include <fcntl.h>	/* For O_BINARY, O_TEXT. */
-#endif
+#include <binary-io.h>
 
 #include "lisp.h"
 #include "commands.h"
@@ -231,6 +229,7 @@
   int c;
   unsigned char hide_char = 0;
   struct emacs_tty etty;
+  bool etty_valid;
 
   /* Check, whether we need to suppress echoing.  */
   if (CHARACTERP (Vread_hide_char))
@@ -239,11 +238,9 @@
   /* Manipulate tty.  */
   if (hide_char)
     {
-      emacs_get_tty (fileno (stdin), &etty);
-#ifdef WINDOWSNT
-      if (isatty (fileno (stdin)))
-	_setmode (fileno (stdin), O_BINARY);
-#endif
+      etty_valid = emacs_get_tty (fileno (stdin), &etty) == 0;
+      if (etty_valid)
+	set_binary_mode (fileno (stdin), O_BINARY);
       suppress_echo_on_tty (fileno (stdin));
     }
 
@@ -281,11 +278,11 @@
   if (hide_char)
     {
       fprintf (stdout, "\n");
-      emacs_set_tty (fileno (stdin), &etty, 0);
-#ifdef WINDOWSNT
-      if (isatty (fileno (stdin)))
-	_setmode (fileno (stdin), O_TEXT);
-#endif
+      if (etty_valid)
+	{
+	  emacs_set_tty (fileno (stdin), &etty, 0);
+	  set_binary_mode (fileno (stdin), O_TEXT);
+	}
     }
 
   if (len || c == '\n' || c == '\r')

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2014-07-11 17:55:24 +0000
+++ src/sysdep.c	2014-07-13 05:24:43 +0000
@@ -775,8 +775,9 @@
 /* Getting and setting emacs_tty structures.  */
 
 /* Set *TC to the parameters associated with the terminal FD,
-   or clear it if the parameters are not available.  */
-void
+   or clear it if the parameters are not available.
+   Return 0 on success, -1 (setting errno) on failure.  */
+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;
 #else	/* !DOS_NT */
   /* We have those nifty POSIX tcmumbleattr functions.  */
-  tcgetattr (fd, &settings->main);
+  return tcgetattr (fd, &settings->main);
 #endif
 }
 

=== modified file 'src/systty.h'
--- src/systty.h	2014-07-11 17:55:24 +0000
+++ src/systty.h	2014-07-12 20:47:50 +0000
@@ -80,7 +80,7 @@
 };
 \f
 /* From sysdep.c or w32.c  */
-extern void emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
+extern int emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
 extern int emacs_set_tty (int, struct emacs_tty *, bool) EXTERNALLY_VISIBLE;
 extern void suppress_echo_on_tty (int);
 extern int serial_open (Lisp_Object);

bug#18006: Simplify via set_binary_mode

[Eli Zaretskii]

> Date: Sat, 12 Jul 2014 22:28:56 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 18006@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.

bug#18006: Simplify via set_binary_mode

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

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.

[-- Attachment #2: binary-io.patch --]
[-- Type: text/plain, Size: 16497 bytes --]

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2014-06-30 00:01:51 +0000
+++ admin/ChangeLog	2014-07-14 02:02:15 +0000
@@ -1,3 +1,9 @@
+2014-07-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module, O_BINARY, and "b" flag (Bug#18006).
+	* merge-gnulib (GNULIB_MODULES): Add binary-io.  It was already
+	present implicitly; this just makes the dependence explicit.
+
 2014-06-30  Glenn Morris  <rgm@gnu.org>
 
 	* update_autogen: Find loaddefs targets rather than

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2014-05-17 08:11:31 +0000
+++ admin/merge-gnulib	2014-07-12 20:47:50 +0000
@@ -26,7 +26,7 @@
 GNULIB_URL=git://git.savannah.gnu.org/gnulib.git
 
 GNULIB_MODULES='
-  alloca-opt byteswap c-ctype c-strcase
+  alloca-opt binary-io byteswap c-ctype c-strcase
   careadlinkat close-stream count-one-bits count-trailing-zeros
   crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
   dtoastr dtotimespec dup2 environ execinfo faccessat

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-07-14 00:45:19 +0000
+++ lib-src/ChangeLog	2014-07-14 02:02:15 +0000
@@ -1,3 +1,18 @@
+2014-07-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module, O_BINARY, and "b" flag (Bug#18006).
+	* etags.c, hexl.c, make-docfile.c:
+	Include binary-io.h instead of fcntl.h and/or io.h.
+	(main): Use set_binary_mode or SET_BINARY
+	in place of handcrafted code.
+	* etags.c (main) [DOS_NT]:
+	* movemail.c (main) [WINDOWSNT]:
+	Don't mess with _fmode.
+	* etags.c (main, process_file_name, analyse_regex):
+	Use fopen/popen's "b" flag instead.
+	* movemail.c (main, popmail): Use open/lk_open/mkostemp's O_BINARY
+	instead.
+
 2014-07-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* make-docfile.c: Simplify a bit, to simplify further refactoring.

=== modified file 'lib-src/etags.c'
--- lib-src/etags.c	2014-07-12 16:26:54 +0000
+++ lib-src/etags.c	2014-07-14 02:02:15 +0000
@@ -105,17 +105,13 @@
 #ifdef MSDOS
 # undef MSDOS
 # define MSDOS true
-# include <fcntl.h>
 # include <sys/param.h>
-# include <io.h>
 #else
 # define MSDOS false
 #endif /* MSDOS */
 
 #ifdef WINDOWSNT
-# include <fcntl.h>
 # include <direct.h>
-# include <io.h>
 # define MAXPATHLEN _MAX_PATH
 # undef HAVE_NTGUI
 # undef  DOS_NT
@@ -131,6 +127,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <binary-io.h>
 #include <c-strcase.h>
 
 #include <assert.h>
@@ -1002,13 +999,8 @@
   linebuffer filename_lb;
   bool help_asked = false;
   ptrdiff_t len;
- char *optstring;
- int opt;
-
-
-#ifdef DOS_NT
-  _fmode = O_BINARY;   /* all of files are treated as binary files */
-#endif /* DOS_NT */
+  char *optstring;
+  int opt;
 
   progname = argv[0];
   nincluded_files = 0;
@@ -1195,15 +1187,10 @@
       if (streq (tagfile, "-"))
 	{
 	  tagf = stdout;
-#ifdef DOS_NT
-	  /* Switch redirected `stdout' to binary mode (setting `_fmode'
-	     doesn't take effect until after `stdout' is already open). */
-	  if (!isatty (fileno (stdout)))
-	    setmode (fileno (stdout), O_BINARY);
-#endif /* DOS_NT */
+	  SET_BINARY (fileno (stdout));
 	}
       else
-	tagf = fopen (tagfile, append_to_tagfile ? "a" : "w");
+	tagf = fopen (tagfile, append_to_tagfile ? "ab" : "wb");
       if (tagf == NULL)
 	pfatal (tagfile);
     }
@@ -1306,7 +1293,7 @@
       append_to_tagfile = true;
     }
 
-  tagf = fopen (tagfile, append_to_tagfile ? "a" : "w");
+  tagf = fopen (tagfile, append_to_tagfile ? "ab" : "wb");
   if (tagf == NULL)
     pfatal (tagfile);
   put_entries (nodehead);	/* write all the tags (CTAGS) */
@@ -1547,11 +1534,11 @@
   if (real_name == compressed_name)
     {
       char *cmd = concat (compr->command, " ", real_name);
-      inf = (FILE *) popen (cmd, "r");
+      inf = popen (cmd, "rb");
       free (cmd);
     }
   else
-    inf = fopen (real_name, "r");
+    inf = fopen (real_name, "rb");
   if (inf == NULL)
     {
       perror (real_name);
@@ -5614,7 +5601,7 @@
 	char *regexfile = regex_arg + 1;
 
 	/* regexfile is a file containing regexps, one per line. */
-	regexfp = fopen (regexfile, "r");
+	regexfp = fopen (regexfile, "rb");
 	if (regexfp == NULL)
 	  pfatal (regexfile);
 	linebuffer_init (&regexbuf);

=== modified file 'lib-src/hexl.c'
--- lib-src/hexl.c	2014-01-01 07:43:34 +0000
+++ lib-src/hexl.c	2014-07-13 05:24:43 +0000
@@ -24,15 +24,8 @@
 
 #include <stdio.h>
 #include <ctype.h>
-#ifdef DOS_NT
-#include <fcntl.h>
-#if __DJGPP__ >= 2
-#include <io.h>
-#endif
-#endif
-#ifdef WINDOWSNT
-#include <io.h>
-#endif
+
+#include <binary-io.h>
 
 #define DEFAULT_GROUPING	0x01
 #define DEFAULT_BASE		16
@@ -155,20 +148,12 @@
 
       if (un_flag)
 	{
-	  char buf[18];
+	  SET_BINARY (fileno (stdout));
 
-#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
 	  for (;;)
 	    {
-	      register int i, c = 0, d;
+	      int i, c = 0, d;
+	      char buf[18];
 
 #define hexchar(x) (isdigit (x) ? x - '0' : x - 'a' + 10)
 
@@ -210,15 +195,7 @@
 	}
       else
 	{
-#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
+	  SET_BINARY (fileno (fp));
 	  address = 0;
 	  string[0] = ' ';
 	  string[17] = '\0';

=== modified file 'lib-src/make-docfile.c'
--- lib-src/make-docfile.c	2014-07-14 00:45:19 +0000
+++ lib-src/make-docfile.c	2014-07-14 00:51:19 +0000
@@ -38,17 +38,16 @@
 
 #include <stdio.h>
 #include <stdlib.h>   /* config.h unconditionally includes this anyway */
-#ifdef MSDOS
-#include <fcntl.h>
-#endif /* MSDOS */
+
 #ifdef WINDOWSNT
 /* Defined to be sys_fopen in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.  */
 #undef fopen
-#include <fcntl.h>
 #include <direct.h>
 #endif /* WINDOWSNT */
 
+#include <binary-io.h>
+
 #ifdef DOS_NT
 /* Defined to be sys_chdir in ms-w32.h, but only #ifdef emacs, so this
    is really just insurance.
@@ -167,19 +166,7 @@
       ++i;
     }
 
-  /* Don't put CRs in the output 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
-#endif /* MSDOS */
-#ifdef WINDOWSNT
-  _fmode = O_BINARY;
-  _setmode (fileno (stdout), O_BINARY);
-#endif /* WINDOWSNT */
+  set_binary_mode (fileno (stdout), O_BINARY);
 
   if (generate_globals)
     start_globals ();

=== modified file 'lib-src/movemail.c'
--- lib-src/movemail.c	2014-01-01 07:43:34 +0000
+++ lib-src/movemail.c	2014-07-14 02:02:15 +0000
@@ -191,11 +191,6 @@
   uid_t real_gid = getgid ();
   uid_t priv_gid = getegid ();
 
-#ifdef WINDOWSNT
-  /* Ensure all file i/o is in binary mode. */
-  _fmode = _O_BINARY;
-#endif
-
   delete_lockname = 0;
 
   while ((c = getopt (argc, argv, ARGSTR)) != EOF)
@@ -304,7 +299,7 @@
 
 	  memcpy (tempname, inname, inname_dirlen);
 	  strcpy (tempname + inname_dirlen, "EXXXXXX");
-	  desc = mkostemp (tempname, 0);
+	  desc = mkostemp (tempname, O_BINARY);
 	  if (desc < 0)
 	    {
 	      int mkostemp_errno = errno;
@@ -358,12 +353,12 @@
 
 #ifndef MAIL_USE_MMDF
 #ifdef MAIL_USE_SYSTEM_LOCK
-      indesc = open (inname, O_RDWR);
+      indesc = open (inname, O_RDWR | O_BINARY);
 #else  /* if not MAIL_USE_SYSTEM_LOCK */
-      indesc = open (inname, O_RDONLY);
+      indesc = open (inname, O_RDONLY | O_BINARY);
 #endif /* not MAIL_USE_SYSTEM_LOCK */
 #else  /* MAIL_USE_MMDF */
-      indesc = lk_open (inname, O_RDONLY, 0, 0, 10);
+      indesc = lk_open (inname, O_RDONLY | O_BINARY, 0, 0, 10);
 #endif /* MAIL_USE_MMDF */
 
       if (indesc < 0)
@@ -372,7 +367,7 @@
       /* Make sure the user can read the output file.  */
       umask (umask (0) & 0377);
 
-      outdesc = open (outname, O_WRONLY | O_CREAT | O_EXCL, 0666);
+      outdesc = open (outname, O_WRONLY | O_BINARY | O_CREAT | O_EXCL, 0666);
       if (outdesc < 0)
 	pfatal_with_name (outname);
 
@@ -675,7 +670,7 @@
       return EXIT_SUCCESS;
     }
 
-  mbfi = open (outfile, O_WRONLY | O_CREAT | O_EXCL, 0666);
+  mbfi = open (outfile, O_WRONLY | O_BINARY | O_CREAT | O_EXCL, 0666);
   if (mbfi < 0)
     {
       pop_close (server);

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2014-07-13 15:50:35 +0000
+++ src/ChangeLog	2014-07-14 02:02:15 +0000
@@ -1,3 +1,22 @@
+2014-07-14  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Use binary-io module, O_BINARY, and "b" flag (Bug#18006).
+	* callproc.c (create_temp_file): Use mkostemp's O_BINARY flag.
+	* emacs.c [MSDOS]:
+	* emacs.c (main) [DOS_NT]: Don't mess with _fmode.
+	(main) [MSDOS]: Use SET_BINARY instead of setmode.
+	* minibuf.c: Include binary-io.h instead of fcntl.h.
+	(read_minibuf_noninteractive):
+	Use set_binary_mode instead of handcrafted code.
+	Don't call emacs_set_tty if emacs_get_tty failed.
+	* process.c: Include binary-io.h.
+	(Fmake_network_process, network_interface_list)
+	(network_interface_info, server_accept_connection):
+	Use set_binary_mode on newly-created sockets.
+	* sysdep.c, systty.h (emacs_get_tty): Return int, not void.
+	* sysdep.c (emacs_open, emacs_pipe): Use O_BINARY.
+	* w32.c (pipe2): Adjust eassert to include O_BINARY.
+
 2014-07-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Improve behavior of 'bzr up; cd src; make -k'.

=== modified file 'src/callproc.c'
--- src/callproc.c	2014-06-08 00:35:27 +0000
+++ src/callproc.c	2014-07-14 02:02:15 +0000
@@ -982,7 +982,7 @@
 
     count = SPECPDL_INDEX ();
     record_unwind_protect_nothing ();
-    fd = mkostemp (tempfile, O_CLOEXEC);
+    fd = mkostemp (tempfile, O_BINARY | O_CLOEXEC);
     if (fd < 0)
       report_file_error ("Failed to open temporary file using pattern",
 			 pattern);

=== modified file 'src/emacs.c'
--- src/emacs.c	2014-06-03 20:08:08 +0000
+++ src/emacs.c	2014-07-14 02:02:15 +0000
@@ -51,6 +51,10 @@
 #include "cygw32.h"
 #endif
 
+#ifdef MSDOS
+#include <binary-io.h>
+#endif
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -918,20 +922,10 @@
 
 #endif	/* not SYSTEM_MALLOC */
 
-#if defined (MSDOS) || defined (WINDOWSNT)
-  /* We do all file input/output as binary files.  When we need to translate
-     newlines, we do that manually.  */
-  _fmode = O_BINARY;
-#endif /* MSDOS || WINDOWSNT */
-
 #ifdef MSDOS
-  if (!isatty (fileno (stdin)))
-    setmode (fileno (stdin), O_BINARY);
-  if (!isatty (fileno (stdout)))
-    {
-      fflush (stdout);
-      setmode (fileno (stdout), O_BINARY);
-    }
+  SET_BINARY (fileno (stdin));
+  fflush (stdout);
+  SET_BINARY (fileno (stdout));
 #endif /* MSDOS */
 
   /* Skip initial setlocale if LC_ALL is "C", as it's not needed in that case.

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2014-07-12 07:47:40 +0000
+++ src/minibuf.c	2014-07-13 05:24:43 +0000
@@ -22,9 +22,7 @@
 #include <errno.h>
 #include <stdio.h>
 
-#ifdef WINDOWSNT
-#include <fcntl.h>	/* For O_BINARY, O_TEXT. */
-#endif
+#include <binary-io.h>
 
 #include "lisp.h"
 #include "commands.h"
@@ -231,6 +229,7 @@
   int c;
   unsigned char hide_char = 0;
   struct emacs_tty etty;
+  bool etty_valid;
 
   /* Check, whether we need to suppress echoing.  */
   if (CHARACTERP (Vread_hide_char))
@@ -239,11 +238,9 @@
   /* Manipulate tty.  */
   if (hide_char)
     {
-      emacs_get_tty (fileno (stdin), &etty);
-#ifdef WINDOWSNT
-      if (isatty (fileno (stdin)))
-	_setmode (fileno (stdin), O_BINARY);
-#endif
+      etty_valid = emacs_get_tty (fileno (stdin), &etty) == 0;
+      if (etty_valid)
+	set_binary_mode (fileno (stdin), O_BINARY);
       suppress_echo_on_tty (fileno (stdin));
     }
 
@@ -281,11 +278,11 @@
   if (hide_char)
     {
       fprintf (stdout, "\n");
-      emacs_set_tty (fileno (stdin), &etty, 0);
-#ifdef WINDOWSNT
-      if (isatty (fileno (stdin)))
-	_setmode (fileno (stdin), O_TEXT);
-#endif
+      if (etty_valid)
+	{
+	  emacs_set_tty (fileno (stdin), &etty, 0);
+	  set_binary_mode (fileno (stdin), O_TEXT);
+	}
     }
 
   if (len || c == '\n' || c == '\r')

=== 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);
+
 #ifdef DATAGRAM_SOCKETS
       if (!is_server && socktype == SOCK_DGRAM)
 	break;
@@ -3535,6 +3538,7 @@
     return Qnil;
   count = SPECPDL_INDEX ();
   record_unwind_protect_int (close_file_unwind, s);
+  set_binary_mode (s, O_BINARY);
 
   do
     {
@@ -3687,6 +3691,7 @@
     return Qnil;
   count = SPECPDL_INDEX ();
   record_unwind_protect_int (close_file_unwind, s);
+  set_binary_mode (s, O_BINARY);
 
   elt = Qnil;
 #if defined (SIOCGIFFLAGS) && defined (HAVE_STRUCT_IFREQ_IFR_FLAGS)
@@ -4134,6 +4139,7 @@
   chan_process[s] = proc;
 
   fcntl (s, F_SETFL, O_NONBLOCK);
+  set_binary_mode (s, O_BINARY);
 
   p = XPROCESS (proc);
 

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2014-07-11 17:55:24 +0000
+++ src/sysdep.c	2014-07-14 02:02:15 +0000
@@ -775,8 +775,9 @@
 /* Getting and setting emacs_tty structures.  */
 
 /* Set *TC to the parameters associated with the terminal FD,
-   or clear it if the parameters are not available.  */
-void
+   or clear it if the parameters are not available.
+   Return 0 on success, -1 (setting errno) on failure.  */
+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;
 #else	/* !DOS_NT */
   /* We have those nifty POSIX tcmumbleattr functions.  */
-  tcgetattr (fd, &settings->main);
+  return tcgetattr (fd, &settings->main);
 #endif
 }
 
@@ -2198,6 +2200,7 @@
 #endif
 
 /* 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)
     QUIT;
   if (! O_CLOEXEC && 0 <= fd)
@@ -2254,7 +2257,7 @@
 #ifdef MSDOS
   return pipe (fd);
 #else  /* !MSDOS */
-  int result = pipe2 (fd, O_CLOEXEC);
+  int result = pipe2 (fd, O_BINARY | O_CLOEXEC);
   if (! O_CLOEXEC && result == 0)
     {
       fcntl (fd[0], F_SETFD, FD_CLOEXEC);

=== modified file 'src/systty.h'
--- src/systty.h	2014-07-11 17:55:24 +0000
+++ src/systty.h	2014-07-12 20:47:50 +0000
@@ -80,7 +80,7 @@
 };
 \f
 /* From sysdep.c or w32.c  */
-extern void emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
+extern int emacs_get_tty (int, struct emacs_tty *) EXTERNALLY_VISIBLE;
 extern int emacs_set_tty (int, struct emacs_tty *, bool) EXTERNALLY_VISIBLE;
 extern void suppress_echo_on_tty (int);
 extern int serial_open (Lisp_Object);

=== modified file 'src/w32.c'
--- src/w32.c	2014-07-10 19:09:26 +0000
+++ src/w32.c	2014-07-14 02:02:15 +0000
@@ -882,7 +882,7 @@
 	  g_b_init_set_named_security_info_a = 1;
 	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
 	  s_pfn_Set_Named_Security_InfoA =
-	    (SetNamedSecurityInfoA_Proc) GetProcAddress (hm_advapi32,
+	    (SetNamedSecurityInfoA_Proc) GetProcAddress (hm_advapi32,
 							 "SetNamedSecurityInfoA");
 	}
       if (s_pfn_Set_Named_Security_InfoA == NULL)
@@ -7865,7 +7865,7 @@
   int rc;
   unsigned flags;
 
-  eassert (pipe2_flags == O_CLOEXEC);
+  eassert (pipe2_flags == (O_BINARY | O_CLOEXEC));
 
   /* make pipe handles non-inheritable; when we spawn a child, we
      replace the relevant handle with an inheritable one.  Also put

bug#18006: Simplify via set_binary_mode

[Eli Zaretskii]

> Date: Sun, 13 Jul 2014 19:11:54 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 18006@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.

bug#18006: Simplify via set_binary_mode

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

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.

[-- Attachment #2: binary-io.patch --]
[-- Type: text/plain, Size: 3239 bytes --]

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-07-14 19:23:18 +0000
+++ lib-src/ChangeLog	2014-07-14 19:46:54 +0000
@@ -1,5 +1,9 @@
 2014-07-14  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Use "b" flag more consistently; avoid "t" (Bug#18006).
+	* make-docfile.c (READ_TEXT): Remove; all uses replaced by "r".
+	(READ_BINARY): Remove; all uses replaced by "rb".
+
 	Use binary-io module, O_BINARY, and "b" flag (Bug#18006).
 	* etags.c, hexl.c, make-docfile.c:
 	Include binary-io.h instead of fcntl.h and/or io.h.

=== modified file 'lib-src/make-docfile.c'
--- lib-src/make-docfile.c	2014-07-14 19:23:18 +0000
+++ lib-src/make-docfile.c	2014-07-14 19:46:54 +0000
@@ -55,12 +55,8 @@
    Similarly, msdos defines this as sys_chdir, but we're not linking with the
    file where that function is defined.  */
 #undef chdir
-#define READ_TEXT "rt"
-#define READ_BINARY "rb"
 #define IS_SLASH(c)  ((c) == '/' || (c) == '\\' || (c) == ':')
 #else  /* not DOS_NT */
-#define READ_TEXT "r"
-#define READ_BINARY "r"
 #define IS_SLASH(c)  ((c) == '/')
 #endif /* not DOS_NT */
 
@@ -216,11 +212,11 @@
   if (!generate_globals)
     put_filename (filename);
   if (len > 4 && !strcmp (filename + len - 4, ".elc"))
-    return scan_lisp_file (filename, READ_BINARY);
+    return scan_lisp_file (filename, "rb");
   else if (len > 3 && !strcmp (filename + len - 3, ".el"))
-    return scan_lisp_file (filename, READ_TEXT);
+    return scan_lisp_file (filename, "r");
   else
-    return scan_c_file (filename, READ_TEXT);
+    return scan_c_file (filename, "r");
 }
 
 static void

=== modified file 'src/ChangeLog'
--- 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@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".
+
 	Use binary-io module, O_BINARY, and "b" flag (Bug#18006).
 	* callproc.c (create_temp_file): Use mkostemp's O_BINARY flag.
 	* emacs.c [MSDOS]:

=== modified file 'src/lread.c'
--- src/lread.c	2014-07-02 03:26:19 +0000
+++ src/lread.c	2014-07-14 19:46:54 +0000
@@ -1061,10 +1061,6 @@
   const char *fmode = "r";
   int version;
 
-#ifdef DOS_NT
-  fmode = "rt";
-#endif /* DOS_NT */
-
   CHECK_STRING (file);
 
   /* If file name is magic, call the handler.  */
@@ -1169,7 +1165,7 @@
 	 Tramp does not catch `load' operations for such files, so we
 	 end up with a nil as the `load' handler above.  If we would
 	 continue with fd = -2, we will behave wrongly, and in
-	 particular try reading a .elc file in the "rt" mode instead
+	 particular try reading a .elc file in the "r" mode instead
 	 of "rb".  See bug #9311 for the results.  To work around
 	 this, we try to open the file locally, and go with that if it
 	 succeeds.  */

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2014-07-07 23:33:05 +0000
+++ src/xfaces.c	2014-07-14 19:46:54 +0000
@@ -6256,7 +6256,7 @@
   abspath = Fexpand_file_name (filename, Qnil);
 
   block_input ();
-  fp = emacs_fopen (SSDATA (abspath), "rt");
+  fp = emacs_fopen (SSDATA (abspath), "r");
   if (fp)
     {
       char buf[512];

bug#18006: Simplify via set_binary_mode

[Eli Zaretskii]

> Date: Mon, 14 Jul 2014 13:16:06 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 18006@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@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?

bug#18006: Simplify via set_binary_mode

[Paul Eggert]

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.