Subject: Re: Assume at least POSIX.1-1988 for fcntl.h

From: Eli Zaretskii <[email-masked]>
Date: Fri, 16 Nov 2012 11:49:51 +0200

> Date: Thu, 15 Nov 2012 12:07:37 -0800
> From: Paul Eggert <[email-masked]>
> CC: [email-masked]
> 
> 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.