From: Paul Eggert <eggert@cs.ucla.edu>
To: Eli Zaretskii <eliz@gnu.org>
Cc: 18006@debbugs.gnu.org
Subject: bug#18006: Simplify via set_binary_mode
Date: Sat, 12 Jul 2014 22:28:56 -0700 [thread overview]
Message-ID: <53C21918.9020001@cs.ucla.edu> (raw)
In-Reply-To: <837g3hanzp.fsf@gnu.org>
[-- 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);
next prev parent reply other threads:[~2014-07-13 5:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-12 20:52 bug#18006: Simplify via set_binary_mode Paul Eggert
2014-07-13 4:20 ` Eli Zaretskii
2014-07-13 5:28 ` Paul Eggert [this message]
2014-07-13 15:02 ` Eli Zaretskii
2014-07-14 2:11 ` Paul Eggert
2014-07-14 15:21 ` Eli Zaretskii
2014-07-14 20:16 ` Paul Eggert
2014-07-15 14:37 ` Eli Zaretskii
2014-07-15 19:39 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53C21918.9020001@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=18006@debbugs.gnu.org \
--cc=eliz@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.