[gnu.emacs.bug] Process management bug patch - context diff

steve@ASC.SLB.COM (12/08/89)

process.c_orig is a copy of the version in dist18.54.
process.c is the patched version. Thanks for your attention.

diff -c /usr/local/emacs/src/process.c_orig /usr/local/emacs/src/process.c
*** /usr/local/emacs/src/process.c_orig	Fri Sep 29 14:01:18 1989
--- /usr/local/emacs/src/process.c	Sun Dec  3 18:56:52 1989
***************
*** 1,3 ****
--- 1,4 ----
+ 
  /* Asynchronous subprocess control for GNU Emacs.
     Copyright (C) 1985, 1986, 1987, 1988 Free Software Foundation, Inc.
  
***************
*** 1287,1292 ****
--- 1288,1294 ----
    int wait_channel = 0;
    struct Lisp_Process *wait_proc = 0;
    extern kbd_count;
+   int n;
  
    /* Detect when read_kbd is really the address of a Lisp_Process.  */
    if (read_kbd > 10 || read_kbd < -1)
***************
*** 1482,1488 ****
  		  ProcessCommChan (channel, proc);
  		  if (NULL (XPROCESS (proc)->command_channel_p))
  		    {
! 		      /* It has ceased to be a command channel! */
  		      int bytes_available;
  		      if (ioctl (channel, FIONREAD, &bytes_available) < 0)
  			bytes_available = 0;
--- 1484,1490 ----
  		  ProcessCommChan (channel, proc);
  		  if (NULL (XPROCESS (proc)->command_channel_p))
  		    {
!  		      /* It has ceased to be a command channel! */
  		      int bytes_available;
  		      if (ioctl (channel, FIONREAD, &bytes_available) < 0)
  			bytes_available = 0;
***************
*** 1495,1523 ****
  
  	      /* Read data from the process, starting with our
  		 buffered-ahead character if we have one.  */
! 
! 	      if (read_process_output (proc, channel) > 0)
! 		{
! 		  if (do_display)
  		    DoDsp (1);
  		}
! 	      else
! 		{
  		  /* Preserve status of processes already terminated.  */
  		  child_changed++;
  		  deactivate_process (proc);
  
! /*
!  * With ptys: when the parent process of a pty exits we are notified,
!  * just as we would be with any of our other children.  After the process
!  * exits, select() will indicate that we can read the channel.  When we
!  * do this, read() returns 0.  Upon receiving this, we close the channel.
!  *
!  * For external channels, when the peer closes the connection, select()
!  * will indicate that we can read the channel.  When we do this, read()
!  * returns -1 with errno = ECONNRESET.  Since we never get notified of
!  * this via wait3(), we must explictly mark the process as having exited.
!  */
  		  if ((XFASTINT (XPROCESS (proc)->flags) & PROC_STATUS)
  		      == RUNNING)
  		    {
--- 1497,1532 ----
  
  	      /* Read data from the process, starting with our
  		 buffered-ahead character if we have one.  */
! 	      if (n = read_process_output (proc, channel) > 0) 
! 		if (n > 0) {
! 		  if (do_display) {
  		    DoDsp (1);
+ 		  }
+ 		  break;
  		}
! 	      /* The next test is here because the pipe may be empty
! 		 even after the select call has returned. If so, simply
! 		 break out of the loop to call select again. */
! #ifdef BSD
! 		else if ((n == -1) && (errno == EWOULDBLOCK)) 
! 		  break;
! #endif BSD
! 		else {
  		  /* Preserve status of processes already terminated.  */
  		  child_changed++;
  		  deactivate_process (proc);
  
! 		  /*
! 		   * With ptys: when the parent process of a pty exits we are notified,
! 		   * just as we would be with any of our other children.  After the process
! 		   * exits, select() will indicate that we can read the channel.  When we
! 		   * do this, read() returns 0.  Upon receiving this, we close the channel.
! 		   *
! 		   * For external channels, when the peer closes the connection, select()
! 		   * will indicate that we can read the channel.  When we do this, read()
! 		   * returns -1 with errno = ECONNRESET.  Since we never get notified of
! 		   * this via wait3(), we must explictly mark the process as having exited.
! 		   */
  		  if ((XFASTINT (XPROCESS (proc)->flags) & PROC_STATUS)
  		      == RUNNING)
  		    {
***************
*** 1562,1568 ****
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return 0;
  
    outstream = p->filter;
    if (!NULL (outstream))
--- 1571,1577 ----
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return nchars; /* 0; */
  
    outstream = p->filter;
    if (!NULL (outstream))
***************
*** 1687,1692 ****
--- 1696,1703 ----
        type = VIPC_MESG;
        datalen = count;
        send_process_1 (proc, &header, sizeof header);
+       /* for debug */
+       fprintf(stderr, "%d %s", count, buf)
      }
  #endif vipc
    send_process_1 (proc, buf, count);
***************
*** 2082,2092 ****
  		   XFASTINT (p->flags) & COREDUMPED ? " (core dumped)" : "");
  	  line[0] = DOWNCASE (line[0]);
  
! 	  if ((XFASTINT (p->flags) & PROC_STATUS) == SIGNALED)
! 	    if (delete_exited_processes)
  	      remove_process (proc);
  	    else
  	      deactivate_process (proc);
  	}
        else if ((XFASTINT (p->flags) & PROC_STATUS) == EXITED)
  	{
--- 2093,2105 ----
  		   XFASTINT (p->flags) & COREDUMPED ? " (core dumped)" : "");
  	  line[0] = DOWNCASE (line[0]);
  
! 	  if ((XFASTINT (p->flags) & PROC_STATUS) == SIGNALED) {
! 	    if (delete_exited_processes) {
  	      remove_process (proc);
+ 	    }
  	    else
  	      deactivate_process (proc);
+ 	  }
  	}
        else if ((XFASTINT (p->flags) & PROC_STATUS) == EXITED)
  	{

steve@ASC.SLB.COM (12/10/89)

You are right about the first break. I've deleted it.
The second break, when an empty pipe is found, could be considered
correct if the empty pipe implies that the select call is everywhere
suspect. But I don't necessarily believe that, so I replaced that
break statement with a null statement. Even if the select is misleading
about all descriptors it indicates to be ready to read, this new test
will catch all of them. The original bug doesn't recur with these 
two changes.

The revised diff is below.

argon[steve]1> diff -c /usr/local/emacs/src/process.c_orig /usr/local/emacs/src/process.c
*** /usr/local/emacs/src/process.c_orig	Fri Sep 29 14:01:18 1989
--- /usr/local/emacs/src/process.c	Sat Dec  9 15:56:30 1989
***************
*** 1287,1292 ****
--- 1287,1293 ----
    int wait_channel = 0;
    struct Lisp_Process *wait_proc = 0;
    extern kbd_count;
+   int n;
  
    /* Detect when read_kbd is really the address of a Lisp_Process.  */
    if (read_kbd > 10 || read_kbd < -1)
***************
*** 1482,1488 ****
  		  ProcessCommChan (channel, proc);
  		  if (NULL (XPROCESS (proc)->command_channel_p))
  		    {
! 		      /* It has ceased to be a command channel! */
  		      int bytes_available;
  		      if (ioctl (channel, FIONREAD, &bytes_available) < 0)
  			bytes_available = 0;
--- 1483,1489 ----
  		  ProcessCommChan (channel, proc);
  		  if (NULL (XPROCESS (proc)->command_channel_p))
  		    {
!  		      /* It has ceased to be a command channel! */
  		      int bytes_available;
  		      if (ioctl (channel, FIONREAD, &bytes_available) < 0)
  			bytes_available = 0;
***************
*** 1495,1522 ****
  
  	      /* Read data from the process, starting with our
  		 buffered-ahead character if we have one.  */
! 
! 	      if (read_process_output (proc, channel) > 0)
! 		{
  		  if (do_display)
  		    DoDsp (1);
  		}
! 	      else
! 		{
  		  /* Preserve status of processes already terminated.  */
  		  child_changed++;
  		  deactivate_process (proc);
  
! /*
!  * With ptys: when the parent process of a pty exits we are notified,
!  * just as we would be with any of our other children.  After the process
!  * exits, select() will indicate that we can read the channel.  When we
!  * do this, read() returns 0.  Upon receiving this, we close the channel.
!  *
!  * For external channels, when the peer closes the connection, select()
!  * will indicate that we can read the channel.  When we do this, read()
!  * returns -1 with errno = ECONNRESET.  Since we never get notified of
!  * this via wait3(), we must explictly mark the process as having exited.
   */
  		  if ((XFASTINT (XPROCESS (proc)->flags) & PROC_STATUS)
  		      == RUNNING)
--- 1496,1529 ----
  
  	      /* Read data from the process, starting with our
  		 buffered-ahead character if we have one.  */
! 	      if (n = read_process_output (proc, channel) > 0) 
! 		if (n > 0) {
  		  if (do_display)
  		    DoDsp (1);
  		}
! 	      /* The next test is here because the pipe may be empty
! 		 even after the select call has returned. If so, simply
! 		 ignore this descriptor, and try again the next time
! 		 select is called. */
! #ifdef BSD
! 		else if ((n == -1) && (errno == EWOULDBLOCK)) 
! 		  ;
! #endif BSD
! 		else {
  		  /* Preserve status of processes already terminated.  */
  		  child_changed++;
  		  deactivate_process (proc);
  
!  /*
!   * With ptys: when the parent process of a pty exits we are notified,
!   * just as we would be with any of our other children.  After the process
!   * exits, select() will indicate that we can read the channel.  When we
!   * do this, read() returns 0.  Upon receiving this, we close the channel.
!   *
!   * For external channels, when the peer closes the connection, select()
!   * will indicate that we can read the channel.  When we do this, read()
!   * returns -1 with errno = ECONNRESET.  Since we never get notified of
!   * this via wait3(), we must explictly mark the process as having exited.
   */
  		  if ((XFASTINT (XPROCESS (proc)->flags) & PROC_STATUS)
  		      == RUNNING)
***************
*** 1562,1568 ****
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return 0;
  
    outstream = p->filter;
    if (!NULL (outstream))
--- 1569,1575 ----
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return nchars; /* 0; */
  
    outstream = p->filter;
    if (!NULL (outstream))
***************
*** 2082,2092 ****
  		   XFASTINT (p->flags) & COREDUMPED ? " (core dumped)" : "");
  	  line[0] = DOWNCASE (line[0]);
  
! 	  if ((XFASTINT (p->flags) & PROC_STATUS) == SIGNALED)
! 	    if (delete_exited_processes)
  	      remove_process (proc);
  	    else
  	      deactivate_process (proc);
  	}
        else if ((XFASTINT (p->flags) & PROC_STATUS) == EXITED)
  	{
--- 2089,2101 ----
  		   XFASTINT (p->flags) & COREDUMPED ? " (core dumped)" : "");
  	  line[0] = DOWNCASE (line[0]);
  
! 	  if ((XFASTINT (p->flags) & PROC_STATUS) == SIGNALED) {
! 	    if (delete_exited_processes) {
  	      remove_process (proc);
+ 	    }
  	    else
  	      deactivate_process (proc);
+ 	  }
  	}
        else if ((XFASTINT (p->flags) & PROC_STATUS) == EXITED)
  	{
argon[steve]2> 

steve@ASC.SLB.COM (12/22/89)

Thanks for your comments.

The change you propose is incomplete. The current version of 
read_process_output (I mean the version in dist-18.54) returns 0
whenever the actual read system call returns a non-positive value.
That's why I changed it to return the result of the read call:

***************
*** 1562,1568 ****
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return 0;
  
    outstream = p->filter;
    if (!NULL (outstream))
--- 1569,1575 ----
  	nchars = nchars + 1;
      }
  
!   if (nchars <= 0) return nchars; /* 0; */
  
    outstream = p->filter;
    if (!NULL (outstream))

This shouldn't affect other calls to read_process_output, since tests are
on its result being positive or non-positive, not 0. The following greps
indicate this:

boron[steve]42> grep 'read_process_output'  /usr/local/emacs/src/*.c
/usr/local/emacs/src/process.c:	      if (n = read_process_output (proc, channel) > 0) 
/usr/local/emacs/src/process.c:read_process_output (proc, channel)
/usr/local/emacs/src/process.c:	while (read_process_output (proc, XFASTINT (p->infd)) > 0);

boron[steve]48> grep 'read_process_output' /usr/local/emacs/src/*.h
boron[steve]49> 


Without this change to read_process_output, the test for EWOULDBLOCK would
never succeed.

Thanks as always.