From: Eli Zaretskii <eliz@gnu.org>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: 18006@debbugs.gnu.org
Subject: bug#18006: Simplify via set_binary_mode
Date: Mon, 14 Jul 2014 18:21:49 +0300 [thread overview]
Message-ID: <83pph87ype.fsf@gnu.org> (raw)
In-Reply-To: <53C33C6A.6060601@cs.ucla.edu>
> 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.
next prev parent reply other threads:[~2014-07-14 15:21 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
2014-07-13 15:02 ` Eli Zaretskii
2014-07-14 2:11 ` Paul Eggert
2014-07-14 15:21 ` Eli Zaretskii [this message]
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
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83pph87ype.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=18006@debbugs.gnu.org \
--cc=eggert@cs.ucla.edu \
/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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).