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.