1999-08-19 Roland McGrath <roland@baalperazim.frob.com>

	* sysdeps/mach/hurd/i386/intr-msg.h (INTR_MSG_TRAP): Mark OPTION and
	TIMEOUT as outputs of the asm to indicate that the signal thread
	might mutate them.
	* hurd/intr-msg.c (_hurd_intr_rpc_mach_msg): Short circuit to plain
	mach_msg if only sending or only receiving (i.e., not an RPC).  When
	making an RPC that might get interrupted, save OPTION and the portion
	of the message buffer that gets clobbered by an EINTR reply message,
	and properly restore them before attempting to retry the request
	message send.

1 files changed, 132 insertions, 56 deletions

diff --git a/hurd/intr-msg.c b/hurd/intr-msg.c
index 2efde2c..a847cff 100644
--- a/hurd/intr-msg.c
+++ b/hurd/intr-msg.c
@@ -21,6 +21,7 @@
 #include <mach/mig_errors.h>
 #include <mach/mig_support.h>
 #include <hurd/signal.h>
+#include <assert.h>
 
 #include "intr-msg.h"
 
@@ -36,39 +37,75 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg,
 {
   error_t err;
   struct hurd_sigstate *ss;
-  int user_timeout;
-
-  if (_hurd_msgport_thread == MACH_PORT_NULL)
+  const mach_msg_option_t user_option = option;
+  const mach_msg_timeout_t user_timeout = timeout;
+
+  struct clobber { int i[2]; };
+  union msg
+  {
+    mach_msg_header_t header;
+    mig_reply_header_t reply;
+    struct
+    {
+      mach_msg_header_t header;
+      int type;
+      int code;
+    } check;
+    struct
     {
-      /* The signal thread is not set up yet, so we cannot do the
-	 normal signal magic.  Do a simple uninterruptible RPC instead.  */
-      return __mach_msg (msg, option, send_size, rcv_size, rcv_name,
+      mach_msg_header_t header;
+      struct clobber data;
+    } request;
+  };
+  union msg *const m = (void *) msg;
+  mach_msg_bits_t msgh_bits;
+  mach_port_t remote_port;
+  mach_msg_id_t msgid;
+  struct clobber save_data;
+
+  if ((option & (MACH_SEND_MSG|MACH_RCV_MSG)) != (MACH_SEND_MSG|MACH_RCV_MSG)
+      || _hurd_msgport_thread == MACH_PORT_NULL)
+    {
+      /* Either this is not an RPC (i.e., only a send or only a receive),
+	 so it can't be interruptible; or, the signal thread is not set up
+	 yet, so we cannot do the normal signal magic.  Do a normal,
+	 uninterruptible mach_msg call instead.  */
+      return __mach_msg (&m->header, option, send_size, rcv_size, rcv_name,
 			 timeout, notify);
     }
 
   ss = _hurd_self_sigstate ();
 
-  /* Notice now if the user requested a timeout.  OPTION may have the bit
-     added by interruption semantics, and we must distinguish.  */
-  user_timeout = option & MACH_RCV_TIMEOUT;
+  /* Save state that gets clobbered by an EINTR reply message.
+     We will need to restore it if we want to retry the RPC.  */
+  msgh_bits = m->header.msgh_bits;
+  remote_port = m->header.msgh_remote_port;
+  msgid = m->header.msgh_id;
+  assert (rcv_size >= sizeof m->request);
+  save_data = m->request.data;
 
   /* Tell the signal thread that we are doing an interruptible RPC on
      this port.  If we get a signal and should return EINTR, the signal
      thread will set this variable to MACH_PORT_NULL.  The RPC might
      return EINTR when some other thread gets a signal, in which case we
      want to restart our call.  */
-  ss->intr_port = msg->msgh_remote_port;
+  ss->intr_port = m->header.msgh_remote_port;
 
-  /* A signal may arrive here, after intr_port is set, but before
-     the mach_msg system call.  The signal handler might do an
-     interruptible RPC, and clobber intr_port; then it would not be
-     set properly when we actually did send the RPC, and a later
-     signal wouldn't interrupt that RPC.  So,
-     _hurd_setup_sighandler saves intr_port in the sigcontext, and
-     sigreturn restores it.  */
+  /* A signal may arrive here, after intr_port is set, but before the
+     mach_msg system call.  The signal handler might do an interruptible
+     RPC, and clobber intr_port; then it would not be set properly when we
+     actually did send the RPC, and a later signal wouldn't interrupt that
+     RPC.  So, _hurd_setup_sighandler saves intr_port in the sigcontext,
+     and sigreturn restores it.  */
 
  message:
 
+  /* XXX
+     At all points here (once SS->intr_port is set), the signal thread
+     thinks we are "about to enter the syscall", and might mutate our
+     return-value register.  This is bogus.
+   */
+
   if (ss->cancel)
     {
       /* We have been cancelled.  Don't do an RPC at all.  */
@@ -77,13 +114,14 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg,
       return EINTR;
     }
 
+  /* Note that the signal trampoline code might modify our OPTION!  */
   err = INTR_MSG_TRAP (msg, option, send_size,
 		       rcv_size, rcv_name, timeout, notify);
 
   switch (err)
     {
     case MACH_RCV_TIMED_OUT:
-      if (user_timeout)
+      if (user_option & MACH_RCV_TIMEOUT)
 	/* The real user RPC timed out.  */
 	break;
       else
@@ -92,18 +130,34 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg,
 	goto interrupted;
 
     case MACH_SEND_INTERRUPTED: /* RPC didn't get out.  */
-    case EINTR:			/* Server not cooperating with interrupt.  */
+      if (!(option & MACH_SEND_MSG))
+	{
+	  /* Oh yes, it did!  Since we were not doing a message send,
+	     this return code cannot have come from the kernel!
+	     Instead, it was the signal thread mutating our state to tell
+	     us not to enter this RPC.  However, we are already in the receive!
+	     Since the signal thread thought we weren't in the RPC yet,
+	     it didn't do an interrupt_operation.
+	     XXX */
+	  goto retry_receive;
+	}
+      /* FALLTHROUGH */
+
+    case EINTR:
+      /* Either the process was stopped and continued,
+	 or the server doesn't support interrupt_operation.  */
       if (ss->intr_port != MACH_PORT_NULL)
 	/* If this signal was for us and it should interrupt calls, the
 	   signal thread will have cleared SS->intr_port.
 	   Since it's not cleared, the signal was for another thread,
 	   or SA_RESTART is set.  Restart the interrupted call.  */
 	{
-	restart:
-	  if (rcv_name != MACH_PORT_NULL)
-	    /* Make sure we have a valid reply port.  The one we were using
-	       may have been destroyed by interruption.  */
-	    msg->msgh_local_port = rcv_name = __mig_get_reply_port ();
+	  /* Make sure we have a valid reply port.  The one we were using
+	     may have been destroyed by interruption.  */
+	  m->header.msgh_local_port = rcv_name = __mig_get_reply_port ();
+	  m->header.msgh_bits = msgh_bits;
+	  option = user_option;
+	  timeout = user_timeout;
 	  goto message;
 	}
       /* FALLTHROUGH */
@@ -122,52 +176,74 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg,
 
     case MACH_RCV_INTERRUPTED:	/* RPC sent; no reply.  */
       option &= ~MACH_SEND_MSG;	/* Don't send again.  */
+    retry_receive:
       if (ss->intr_port == MACH_PORT_NULL)
 	{
 	  /* This signal or cancellation was for us.  We need to wait for
              the reply, but not hang forever.  */
 	  option |= MACH_RCV_TIMEOUT;
 	  /* Never decrease the user's timeout.  */
-	  if (!user_timeout || timeout > _hurd_interrupted_rpc_timeout)
+	  if (!(user_option & MACH_RCV_TIMEOUT)
+	      || timeout > _hurd_interrupted_rpc_timeout)
 	    timeout = _hurd_interrupted_rpc_timeout;
 	}
+      else
+	{
+	  option = user_option;
+	  timeout = user_timeout;
+	}
       goto message;		/* Retry the receive.  */
 
     case MACH_MSG_SUCCESS:
-      if (option & MACH_RCV_MSG)
+      {
+	/* We got a reply.  Was it EINTR?  */
+	const union
 	{
-	  /* We got a reply.  Was it EINTR?  */
-	  mig_reply_header_t *const reply = (void *) msg;
-	  const union
-	    {
-	      mach_msg_type_t t;
-	      integer_t i;
-	    } check =
-	      { t: {
-		MACH_MSG_TYPE_INTEGER_T,
-		sizeof (integer_t) * 8,
-		1,
-		TRUE,
-		FALSE,
-		FALSE,
-		0
-	      } };
-	  if (msg->msgh_size == sizeof *reply &&
-	      !(msg->msgh_bits & MACH_MSGH_BITS_COMPLEX) &&
-	      *(int *) &reply->RetCodeType == check.i &&
-	      reply->RetCode == EINTR)
-	    {
-	      /* It is indeed EINTR.  Is the interrupt for us?  */
-	      if (ss->intr_port != MACH_PORT_NULL)
+	  mach_msg_type_t t;
+	  int i;
+	} check =
+	  { t: { MACH_MSG_TYPE_INTEGER_T, sizeof (integer_t) * 8,
+		 1, TRUE, FALSE, FALSE, 0 } };
+
+        if (m->reply.RetCode == EINTR &&
+	    m->header.msgh_size == sizeof m->reply &&
+	    m->check.type == check.i &&
+	    !(m->header.msgh_bits & MACH_MSGH_BITS_COMPLEX))
+	  {
+	    /* It is indeed EINTR.  Is the interrupt for us?  */
+	    if (ss->intr_port != MACH_PORT_NULL)
+	      {
 		/* Nope; repeat the RPC.
 		   XXX Resources moved? */
-		goto restart;
-	      else
-		/* The EINTR return indicates cancellation, so clear the
-                   flag.  */
-		ss->cancel = 0;
-	    }
-	}
+
+		assert (m->header.msgh_id == msgid + 100);
+
+		/* We know we have a valid reply port, because we just
+		   received the EINTR reply on it.  Restore it and the
+		   other fields in the message header needed for send,
+		   since the header now reflects receipt of the reply.  */
+		m->header.msgh_local_port = rcv_name;
+		m->header.msgh_remote_port = remote_port;
+		m->header.msgh_id = msgid;
+		m->header.msgh_bits = msgh_bits;
+		/* Restore the two words clobbered by the reply data.  */
+		m->request.data = save_data;
+
+		/* Restore the original mach_msg options.
+		   OPTION may have had MACH_RCV_TIMEOUT added,
+		   and/or MACH_SEND_MSG removed.  */
+		option = user_option;
+		timeout = user_timeout;
+
+		/* Now we are ready to repeat the original message send.  */
+		goto message;
+	      }
+	    else
+	      /* The EINTR return indicates cancellation,
+		 so clear the flag.  */
+	      ss->cancel = 0;
+	  }
+      }
       break;
 
     default:			/* Quiet -Wswitch-enum.  */