[hurd,commited] hurd: Fix timeout handling in _hurd_select

[Samuel Thibault]

From: Richard Braun <address@hidden>

Rely on servers to implement timeouts, so that very short values (including
0) don't make mach_msg return before valid replies can be received. The
purpose of this scheme is to guarantee a full client-server round-trip,
whatever the timeout value.

This change depends on the new io_select_timeout RPC being implemented by
servers.

        * hurd/Makefile (user-interfaces): Add io_reply and io_request.
        * hurd/hurdselect.c: Include <sys/time.h>, <hurd/io_request.h> and
        <limits.h>.
        (_hurd_select): Replace the call to __io_select with either
        __io_select_request or __io_select_timeout_request, depending on the
        timeout. Count the number of ready descriptors (replies for which at
        least one type bit is set). Implement the timeout locally when there is
        no file descriptor.
---
 ChangeLog         |   9 +++-
 hurd/Makefile     |   3 +-
 hurd/hurdselect.c | 116 ++++++++++++++++++++++++++++++----------------
 3 files changed, 87 insertions(+), 41 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d2ec5d5ac0..37cbe28169 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,5 @@
 2019-08-30  Samuel Thibault  <address@hidden>
 
-
        * sysdeps/mach/hurd/getcwd.c
        (_hurd_canonicalize_directory_name_internal): Do not remove the heading
        slash if we got an unknown root directory. (__getcwd): Do not fail with
@@ -11,6 +10,14 @@
        * hurd/hurdselect.c (_hurd_select): Always call __io_select with no
        timeout.
        * sysdeps/mach/hurd/setitimer.c (setitimer_locked): Fix preemptor setup.
+       * hurd/Makefile (user-interfaces): Add io_reply and io_request.
+       * hurd/hurdselect.c: Include <sys/time.h>, <hurd/io_request.h> and
+       <limits.h>.
+       (_hurd_select): Replace the call to __io_select with either
+       __io_select_request or __io_select_timeout_request, depending on the
+       timeout. Count the number of ready descriptors (replies for which at
+       least one type bit is set). Implement the timeout locally when there is
+       no file descriptor.
 
 2019-08-29  Mihailo Stojanovic  <address@hidden>
 
diff --git a/hurd/Makefile b/hurd/Makefile
index 99d33f9562..6d9cbf57a5 100644
--- a/hurd/Makefile
+++ b/hurd/Makefile
@@ -33,7 +33,8 @@ user-interfaces               := $(addprefix hurd/,\
                                       process process_request \
                                       msg msg_reply msg_request \
                                       exec exec_startup crash interrupt \
-                                      fs fsys io term tioctl socket ifsock \
+                                      fs fsys io io_reply io_request \
+                                      term tioctl socket ifsock \
                                       login password pfinet pci \
                                       )
 server-interfaces      := hurd/msg faultexc
diff --git a/hurd/hurdselect.c b/hurd/hurdselect.c
index a5e6e26b9a..416e4a37bd 100644
--- a/hurd/hurdselect.c
+++ b/hurd/hurdselect.c
@@ -16,14 +16,17 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <sys/time.h>
 #include <sys/types.h>
 #include <sys/poll.h>
 #include <hurd.h>
 #include <hurd/fd.h>
+#include <hurd/io_request.h>
 #include <stdlib.h>
 #include <string.h>
 #include <assert.h>
 #include <stdint.h>
+#include <limits.h>
 
 /* All user select types.  */
 #define SELECT_ALL (SELECT_READ | SELECT_WRITE | SELECT_URG)
@@ -44,11 +47,13 @@ _hurd_select (int nfds,
 {
   int i;
   mach_port_t portset;
-  int got;
+  int got, ready;
   error_t err;
   fd_set rfds, wfds, xfds;
   int firstfd, lastfd;
-  mach_msg_timeout_t to = 0;
+  mach_msg_id_t reply_msgid;
+  mach_msg_timeout_t to;
+  struct timespec ts;
   struct
     {
       struct hurd_userlink ulink;
@@ -73,16 +78,39 @@ _hurd_select (int nfds,
       return -1;
     }
 
-  if (timeout != NULL)
+#define IO_SELECT_REPLY_MSGID (21012 + 100) /* XXX */
+#define IO_SELECT_TIMEOUT_REPLY_MSGID (21031 + 100) /* XXX */
+
+  if (timeout == NULL)
+    reply_msgid = IO_SELECT_REPLY_MSGID;
+  else
     {
-      if (timeout->tv_sec < 0 || timeout->tv_nsec < 0)
+      struct timeval now;
+
+      if (timeout->tv_sec < 0 || timeout->tv_nsec < 0 ||
+         timeout->tv_nsec >= 1000000000)
        {
          errno = EINVAL;
          return -1;
        }
 
-      to = (timeout->tv_sec * 1000
-            + (timeout->tv_nsec + 999999) / 1000000);
+      err = __gettimeofday(&now, NULL);
+      if (err)
+       return -1;
+
+      ts.tv_sec = now.tv_sec + timeout->tv_sec;
+      ts.tv_nsec = now.tv_usec * 1000 + timeout->tv_nsec;
+
+      if (ts.tv_nsec >= 1000000000)
+       {
+         ts.tv_sec++;
+         ts.tv_nsec -= 1000000000;
+       }
+
+      if (ts.tv_sec < 0)
+       ts.tv_sec = LONG_MAX; /* XXX */
+
+      reply_msgid = IO_SELECT_TIMEOUT_REPLY_MSGID;
     }
 
   if (sigmask && __sigprocmask (SIG_SETMASK, sigmask, &oset))
@@ -236,12 +264,13 @@ _hurd_select (int nfds,
          {
            int type = d[i].type;
            d[i].reply_port = __mach_reply_port ();
-           err = __io_select (d[i].io_port, d[i].reply_port, 0, &type);
-           switch (err)
+           if (timeout == NULL)
+             err = __io_select_request (d[i].io_port, d[i].reply_port, type);
+           else
+             err = __io_select_timeout_request (d[i].io_port, d[i].reply_port,
+                                                ts, type);
+           if (!err)
              {
-             case MACH_RCV_TIMED_OUT:
-               /* No immediate response.  This is normal.  */
-               err = 0;
                if (firstfd == lastfd)
                  /* When there's a single descriptor, we don't need a
                     portset, so just pretend we have one, but really
@@ -262,32 +291,24 @@ _hurd_select (int nfds,
                      __mach_port_move_member (__mach_task_self (),
                                               d[i].reply_port, portset);
                  }
-               break;
-
-             default:
-               /* No other error should happen.  Callers of select
+             }
+           else
+             {
+               /* No error should happen.  Callers of select
                   don't expect to see errors, so we simulate
                   readiness of the erring object and the next call
                   hopefully will get the error again.  */
-               type = SELECT_ALL;
-               /* FALLTHROUGH */
-
-             case 0:
-               /* We got an answer.  */
-               if ((type & SELECT_ALL) == 0)
-                 /* Bogus answer; treat like an error, as a fake positive.  */
-                 type = SELECT_ALL;
-
-               /* This port is already ready already.  */
-               d[i].type &= type;
                d[i].type |= SELECT_RETURNED;
                ++got;
-               break;
              }
            _hurd_port_free (&d[i].cell->port, &d[i].ulink, d[i].io_port);
          }
     }
 
+  /* GOT is the number of replies (or errors), while READY is the number of
+     replies with at least one type bit set.  */
+  ready = 0;
+
   /* Now wait for reply messages.  */
   if (!err && got == 0)
     {
@@ -329,22 +350,35 @@ _hurd_select (int nfds,
            } success;
 #endif
        } msg;
-      mach_msg_option_t options = (timeout == NULL ? 0 : MACH_RCV_TIMEOUT);
+      mach_msg_option_t options;
       error_t msgerr;
+
+      /* We rely on servers to implement the timeout, but when there are none,
+        do it on the client side.  */
+      if (timeout != NULL && firstfd == -1)
+       {
+         options = MACH_RCV_TIMEOUT;
+         to = timeout->tv_sec * 1000 + (timeout->tv_nsec + 999999) / 1000000;
+       }
+      else
+       {
+         options = 0;
+         to = MACH_MSG_TIMEOUT_NONE;
+       }
+
       while ((msgerr = __mach_msg (&msg.head,
                                   MACH_RCV_MSG | MACH_RCV_INTERRUPT | options,
                                   0, sizeof msg, portset, to,
                                   MACH_PORT_NULL)) == MACH_MSG_SUCCESS)
        {
          /* We got a message.  Decode it.  */
-#define IO_SELECT_REPLY_MSGID (21012 + 100) /* XXX */
 #ifdef MACH_MSG_TYPE_BIT
          const union typeword inttype =
          { type:
            { MACH_MSG_TYPE_INTEGER_T, sizeof (integer_t) * 8, 1, 1, 0, 0 }
          };
 #endif
-         if (msg.head.msgh_id == IO_SELECT_REPLY_MSGID
+         if (msg.head.msgh_id == reply_msgid
              && msg.head.msgh_size >= sizeof msg.error
              && !(msg.head.msgh_bits & MACH_MSGH_BITS_COMPLEX)
 #ifdef MACH_MSG_TYPE_BIT
@@ -362,12 +396,13 @@ _hurd_select (int nfds,
                  err = EINTR;
                  goto poll;
                }
+             /* Keep in mind msg.success.result can be 0 if a timeout
+                occurred.  */
              if (msg.error.err
-                 || msg.head.msgh_size != sizeof msg.success
 #ifdef MACH_MSG_TYPE_BIT
                  || msg.success.result_type.word != inttype.word
 #endif
-                 || (msg.success.result & SELECT_ALL) == 0)
+                 || msg.head.msgh_size != sizeof msg.success)
                {
                  /* Error or bogus reply.  Simulate readiness.  */
                  __mach_msg_destroy (&msg.head);
@@ -384,6 +419,9 @@ _hurd_select (int nfds,
                        && d[i].reply_port == msg.head.msgh_local_port)
                      {
                        d[i].type &= msg.success.result;
+                       if (d[i].type)
+                         ++ready;
+
                        d[i].type |= SELECT_RETURNED;
                        ++got;
                      }
@@ -408,7 +446,7 @@ _hurd_select (int nfds,
        /* Interruption on our side (e.g. signal reception).  */
        err = EINTR;
 
-      if (got)
+      if (ready)
        /* At least one descriptor is known to be ready now, so we will
           return success.  */
        err = 0;
@@ -452,9 +490,9 @@ _hurd_select (int nfds,
       }
   else
     {
-      /* Below we recalculate GOT to include an increment for each operation
+      /* Below we recalculate READY to include an increment for each operation
         allowed on each fd.  */
-      got = 0;
+      ready = 0;
 
       /* Set the user bitarrays.  We only ever have to clear bits, as all
         desired ones are initially set.  */
@@ -467,15 +505,15 @@ _hurd_select (int nfds,
              type = 0;
 
            if (type & SELECT_READ)
-             got++;
+             ready++;
            else if (readfds)
              FD_CLR (i, readfds);
            if (type & SELECT_WRITE)
-             got++;
+             ready++;
            else if (writefds)
              FD_CLR (i, writefds);
            if (type & SELECT_URG)
-             got++;
+             ready++;
            else if (exceptfds)
              FD_CLR (i, exceptfds);
          }
@@ -484,5 +522,5 @@ _hurd_select (int nfds,
   if (sigmask && __sigprocmask (SIG_SETMASK, &oset, NULL))
     return -1;
 
-  return got;
+  return ready;
 }
-- 
2.23.0.rc1