Re: [PATCH,HURD][RFC] hurdselect: Step7x, almost complete rewrite finished

[Samuel Thibault]

Svante Signell, le Tue 12 Feb 2013 23:46:00 +0100, a écrit :
> On Tue, 2013-02-12 at 22:55 +0100, Samuel Thibault wrote:
> > Svante Signell, le Tue 12 Feb 2013 09:09:25 +0100, a écrit :
> > > Attached is a patch wrt the latest patch (step2) on January 24 2013. The
> > > indentation is different, therefore the diff blocks are a little large
> > > (but easier to read). Hopefully this is better. Including also the
> > > resulting file hurdselect_step7x.c. Explanations later in a separate
> > > mail.
> > 
> > Explanations don't need to be very long. All we need is the rationale
> > and the way you handle it in the patch.
> > 
> > AIUI, there are three things:
> > 
> > - pass 1ms as timeout. This will be fixed by Richard's patch.
> > - in the poll() case, do not fail completely if an FD is bogus. Instead,
> >   poll on the remaining FDs, and later on set POLLNVAL. It seems your
> >   patch does this by using the i_index array indirection.
> > - in the poll() case, on error returned by io_select, instead of setting
> >   readiness, set POLLHUP or POLLERR according to the error.
> > 
> > Is that right?
> 
> Yes, mainly.

Ok, there were indeed details I had missed from your patch (goes better
with explanations...).  I have rewritten your changes as seen below,
could you read it to see whether the fixes you have done are apparently
all there? (this is completely untested, not even compiled, but it's
reviewable)

Samuel

Date:   Wed Feb 13 01:11:28 2013 +0100

    Fix poll and select POSIX compliancy details about errors
    
    This fixes the following:
    
    - Poll must not return immediately on error, including EBADF, and instead
      report POLLHUP/POLLERR/POLLNVAL
    - Select must report EBADF if some set contains an invalid FD.
    
    The idea is to move error management to after all select calls, in the
    poll/select final treatment. The error is instead recorded in a new `error'
    field, and a new SELECT_ERROR bit set.
    
    Thanks Svante Signell for the initial version of the patch.
    
    * hurd/hurdselect.c (SELECT_ERROR): New macro.
    (_hurd_select):
    - Add `error' field to `d' structures array.
    - If a poll descriptor is bogus, set EBADF, but continue.
    - Go through the whole fd_set, not only until _hurd_dtablesize. Return 
EBADF if
    there is any bit set above _hurd_dtablesize.
    - Do not request io_select on bogus descriptors (SELECT_ERROR).
    - On io_select request error, record the error.
    - On io_select bogus reply, use EIO error code.
    - On io_select bogus or error reply, record the error.
    - Do not destroy reply port for bogus FDs.
    - On error, make poll set POLLHUP in the EPIPE case, POLLNVAL in the EBADF
    case, or else POLLERR.
    - On error, make select simulated readiness.

diff --git a/hurd/hurdselect.c b/hurd/hurdselect.c
index 58d5987..63739f3 100644
--- a/hurd/hurdselect.c
+++ b/hurd/hurdselect.c
@@ -33,6 +33,7 @@
 /* Used to record that a particular select rpc returned.  Must be distinct
    from SELECT_ALL (which better not have the high bit set).  */
 #define SELECT_RETURNED ((SELECT_ALL << 1) & ~SELECT_ALL)
+#define SELECT_ERROR (SELECT_RETURNED << 1)
 
 /* Check the first NFDS descriptors either in POLLFDS (if nonnnull) or in
    each of READFDS, WRITEFDS, EXCEPTFDS that is nonnull.  If TIMEOUT is not
@@ -60,6 +61,7 @@ _hurd_select (int nfds,
       mach_port_t io_port;
       int type;
       mach_port_t reply_port;
+      int error;
     } d[nfds];
   sigset_t oset;
 
@@ -155,25 +157,14 @@ _hurd_select (int nfds,
                  continue;
              }
 
-           /* If one descriptor is bogus, we fail completely.  */
-           while (i-- > 0)
-             if (d[i].type != 0)
-               _hurd_port_free (&d[i].cell->port,
-                                &d[i].ulink, d[i].io_port);
-           break;
+           /* Bogus descriptor, make it EBADF already.  */
+           d[i].error = EBADF;
+           d[i].type = SELECT_ERROR;
          }
 
       __mutex_unlock (&_hurd_dtable_lock);
       HURD_CRITICAL_END;
 
-      if (i < nfds)
-       {
-         if (sigmask)
-           __sigprocmask (SIG_SETMASK, &oset, NULL);
-         errno = EBADF;
-         return -1;
-       }
-
       lastfd = i - 1;
       firstfd = i == 0 ? lastfd : 0;
     }
@@ -198,9 +189,6 @@ _hurd_select (int nfds,
       HURD_CRITICAL_BEGIN;
       __mutex_lock (&_hurd_dtable_lock);
 
-      if (nfds > _hurd_dtablesize)
-       nfds = _hurd_dtablesize;
-
       /* Collect the ports for interesting FDs.  */
       firstfd = lastfd = -1;
       for (i = 0; i < nfds; ++i)
@@ -215,9 +203,12 @@ _hurd_select (int nfds,
          d[i].type = type;
          if (type)
            {
-             d[i].cell = _hurd_dtable[i];
-             d[i].io_port = _hurd_port_get (&d[i].cell->port, &d[i].ulink);
-             if (d[i].io_port == MACH_PORT_NULL)
+             if (i < _hurd_dtablesize)
+               {
+                 d[i].cell = _hurd_dtable[i];
+                 d[i].io_port = _hurd_port_get (&d[i].cell->port, &d[i].ulink);
+               }
+             if (i >= _hurd_dtablesize || d[i].io_port == MACH_PORT_NULL)
                {
                  /* If one descriptor is bogus, we fail completely.  */
                  while (i-- > 0)
@@ -242,6 +233,9 @@ _hurd_select (int nfds,
          errno = EBADF;
          return -1;
        }
+
+      if (nfds > _hurd_dtablesize)
+       nfds = _hurd_dtablesize;
     }
 
 
@@ -259,7 +253,7 @@ _hurd_select (int nfds,
       portset = MACH_PORT_NULL;
 
       for (i = firstfd; i <= lastfd; ++i)
-       if (d[i].type)
+       if (d[i].type & ~SELECT_ERROR)
          {
            int type = d[i].type;
            d[i].reply_port = __mach_reply_port ();
@@ -293,11 +287,10 @@ _hurd_select (int nfds,
              }
            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.  */
-               d[i].type |= SELECT_RETURNED;
+               /* No error should happen, but record it for later
+                  processing.  */
+               d[i].error = err;
+               d[i].type |= SELECT_ERROR;
                ++got;
              }
            _hurd_port_free (&d[i].cell->port, &d[i].ulink, d[i].io_port);
@@ -397,9 +390,10 @@ _hurd_select (int nfds,
 #endif
                  msg.head.msgh_size != sizeof msg.success)
                {
-                 /* Error or bogus reply.  Simulate readiness.  */
+                 /* Error or bogus reply.  */
+                 if (!msg.error.err)
+                   msg.error.err = EIO;
                  __mach_msg_destroy (&msg.head);
-                 msg.success.result = SELECT_ALL;
                }
 
              /* Look up the respondent's reply port and record its
@@ -411,9 +405,18 @@ _hurd_select (int nfds,
                    if (d[i].type
                        && d[i].reply_port == msg.head.msgh_local_port)
                      {
-                       d[i].type &= msg.success.result;
-                       if (d[i].type)
-                         ++ready;
+                       if (msg.error.err)
+                         {
+                           d[i].error = msg.error.err;
+                           d[i].type = SELECT_ERROR;
+                           ++ready;
+                         }
+                       else
+                         {
+                           d[i].type &= msg.success.result;
+                           if (d[i].type)
+                             ++ready;
+                         }
 
                        d[i].type |= SELECT_RETURNED;
                        ++got;
@@ -454,7 +457,7 @@ _hurd_select (int nfds,
 
   if (firstfd != -1)
     for (i = firstfd; i <= lastfd; ++i)
-      if (d[i].type)
+      if (d[i].type & ~SELECT_ERROR)
        __mach_port_destroy (__mach_task_self (), d[i].reply_port);
   if (firstfd == -1 || (firstfd != lastfd && portset != MACH_PORT_NULL))
     /* Destroy PORTSET, but only if it's not actually the reply port for a
@@ -476,15 +479,29 @@ _hurd_select (int nfds,
        int type = d[i].type;
        int_fast16_t revents = 0;
 
-       if (type & SELECT_RETURNED)
-         {
-           if (type & SELECT_READ)
-             revents |= POLLIN;
-           if (type & SELECT_WRITE)
-             revents |= POLLOUT;
-           if (type & SELECT_URG)
-             revents |= POLLPRI;
-         }
+       if (type & SELECT_ERROR)
+         switch (d[i].error)
+           {
+             case EPIPE:
+               revents = POLLHUP;
+               break;
+             case EBADF:
+               revents = POLLNVAL
+               break;
+             default:
+               revents = POLLERR;
+               break;
+           }
+       else
+         if (type & SELECT_RETURNED)
+           {
+             if (type & SELECT_READ)
+               revents |= POLLIN;
+             if (type & SELECT_WRITE)
+               revents |= POLLOUT;
+             if (type & SELECT_URG)
+               revents |= POLLPRI;
+           }
 
        pollfds[i].revents = revents;
       }
@@ -504,6 +521,12 @@ _hurd_select (int nfds,
            if ((type & SELECT_RETURNED) == 0)
              type = 0;
 
+           /* 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.  */
+           if (type & SELECT_ERROR)
+             type = SELECT_ALL;
+
            if (type & SELECT_READ)
              ready++;
            else if (readfds)