Subject: Re: bug#18006: Simplify via set_binary_mode

From: Paul Eggert <[email-masked]>
Date: Sat, 12 Jul 2014 22:28:56 -0700

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.
=== 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  <[email-masked]>
+
+	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  <[email-masked]>
 
 	* 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  <[email-masked]>
 
+	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  <[email-masked]>

=== 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  <[email-masked]>
+
+	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  <[email-masked]>
 
 	* 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 @@
 };
 
 /* 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);