all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: 12881@debbugs.gnu.org
Subject: bug#12881: Assume at least POSIX.1-1988 for fcntl.h
Date: Fri, 16 Nov 2012 11:49:51 +0200	[thread overview]
Message-ID: <83mwyhswqo.fsf@gnu.org> (raw)
In-Reply-To: <50A54B89.2080505@cs.ucla.edu>

> Date: Thu, 15 Nov 2012 12:07:37 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 12881@debbugs.gnu.org
> 
> On 11/15/12 09:42, Eli Zaretskii wrote:
> > Thanks, please hold on to it for a few days, until I or someone else
> > will have time to study it more closely.
> 
> I'll hold onto it for a bit, but there's no need for a close review
> process here.  The code works fine on POSIXish hosts and we've done a
> quick scan for plausible failures on Windows hosts and have come up
> dry.  Any problems that come up on Windows will likely be either
> easy-to-fix compilation issues, or further simplifications of the code
> that will not fix bugs.

Thanks for waiting.

The reason I prefer to review the patches such as this one is to
minimize downtime for several Windows users who track the development
trunk.  As you see below, our initial discussion notwithstanding, your
patches still break the Windows build in several places.  Experience
shows that if the Windows build is broken, it remains broken until I
get to fix it, with no one else stepping forward to do that.  It is
IMO bad mojo for several good people to depend on my scarce free time
or sleeping hours.

Here's the result of reviewing the combined patch you've posted in
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=12881#20:

  @@ -4847,23 +4795,16 @@
		else if (nread == -1 && errno == EWOULDBLOCK)
		  ;
   #endif
  -	      /* ISC 4.1 defines both EWOULDBLOCK and O_NONBLOCK,
  -		 and Emacs uses O_NONBLOCK, so what we get is EAGAIN.  */
  -#if O_NONBLOCK
  -	      else if (nread == -1 && errno == EAGAIN)
  -		;
  -#else
  -#if O_NDELAY
  -	      else if (nread == -1 && errno == EAGAIN)
  -		;
  +	      else if (nread == -1 && errno == EAGAIN)
  +		;
  +#ifdef DOS_NT
		/* Note that we cannot distinguish between no input
		   available now and a closed pipe.
		   With luck, a closed pipe will be accompanied by
		   subprocess termination and SIGCHLD.  */
		else if (nread == 0 && !NETCONN_P (proc) && !SERIALCONN_P (proc))
		  ;
  -#endif /* O_NDELAY */
  -#endif /* O_NONBLOCK */
  +#endif

This hunk from process.c patches should use WINDOWSNT, not DOS_NT,
since MSDOS does not compile that part of process.c.

  @@ -1278,8 +1276,7 @@
     fsync (fileno (tty_out->output));
   #endif

  -#ifdef F_SETFL
  -#ifdef F_SETOWN		/* F_SETFL does not imply existence of F_SETOWN */
  +#ifdef F_SETOWN
     if (interrupt_input)
       {
	 reset_sigio (fileno (tty_out->input));
  @@ -1287,11 +1284,8 @@
		old_fcntl_owner[fileno (tty_out->input)]);
       }
   #endif /* F_SETOWN */
  -#if O_NDELAY
     fcntl (fileno (tty_out->input), F_SETFL,
  -         fcntl (fileno (tty_out->input), F_GETFL, 0) & ~O_NDELAY);
  -#endif
  -#endif /* F_SETFL */
  +         fcntl (fileno (tty_out->input), F_GETFL, 0) & ~O_NONBLOCK);

     if (tty_out->old_tty)
       while (emacs_set_tty (fileno (tty_out->input),

In this hunk, the block that was conditioned on F_SETFL should now be
conditioned on !DOS_NT.  The reason is that, although the Windows
build does have F_SETFL defined, it is defined in sys/socket.h, which
is not included by sysdep.c in the MS-Windows build.  So this block of
code was never compiled on Windows, and should continue not being
compiled there, since it invokes fcntl on TTY handles, something
that's not supported by w32.c:fcntl emulation, and also uses F_GETFL
which is not defined on Windows.

  --- src/term.c	2012-11-14 04:55:41 +0000
  +++ src/term.c	2012-11-14 07:26:25 +0000
  @@ -55,14 +55,6 @@
   #include "xterm.h"
   #endif

  -#ifndef O_RDWR
  -#define O_RDWR 2
  -#endif
  -
  -#ifndef O_NOCTTY
  -#define O_NOCTTY 0
  -#endif
  -
   /* The name of the default console device.  */
   #ifdef WINDOWSNT

This hunk will break MS-Windows compilation because O_NOCTTY is no
longer defined.  I suggest adding the definition in the WINDOWSNT
block just below, where the default console device name is defined.

Also, you need to include fcntl.h in term.c, to get both O_RDRW and
O_NOCTTY actually defined.  I wonder how it compiled for you;
presumably some other header on your platform includes fcntl.h
internally, but we shouldn't depend on that, IMO.

With those changes, I think the patch is safe to go into the trunk.
Thanks.






  reply	other threads:[~2012-11-16  9:49 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-14  7:37 bug#12881: Assume at least POSIX.1-1988 for fcntl.h Paul Eggert
2012-11-14 17:33 ` Eli Zaretskii
2012-11-15  6:32   ` Paul Eggert
2012-11-15 17:42     ` Eli Zaretskii
2012-11-15 20:07       ` Paul Eggert
2012-11-16  9:49         ` Eli Zaretskii [this message]
2012-11-16 14:37           ` Stefan Monnier
2012-11-16 14:55             ` Juanma Barranquero
2012-11-16 15:26               ` Stefan Monnier
2012-11-16 16:00                 ` Eli Zaretskii
2012-11-16 16:29                   ` Juanma Barranquero
2012-11-16 17:41                     ` Eli Zaretskii
2012-11-16 17:10                   ` Stefan Monnier
2012-11-16 17:14                     ` Juanma Barranquero
2012-11-16 17:20                       ` Juanma Barranquero
2012-11-16 17:44                     ` Eli Zaretskii
2012-11-16 17:52                       ` Drew Adams
2012-11-17 22:17           ` Paul Eggert
2012-11-18  3:46             ` Eli Zaretskii
2012-11-18  4:41               ` Paul Eggert
2012-11-18 16:58                 ` Eli Zaretskii
2012-11-16  4:38 ` 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=83mwyhswqo.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=12881@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 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.