]> Zhao Yanbai Git Server - minix.git/commitdiff
VFS: select(2) fixes 75/975/2
authorDavid van Moolenbroek <david@minix3.org>
Tue, 10 Sep 2013 23:13:59 +0000 (01:13 +0200)
committerLionel Sambuc <lionel@minix3.org>
Sat, 1 Mar 2014 08:04:51 +0000 (09:04 +0100)
- check each file descriptor's open access mode (filp_mode);
- treat an error returned by a character driver as a select error;
- check all filps in each set before finishing select;
- do not copy back file descriptor sets if an error occurred;
- remove the hardcoded list of supported character major devices,
  since all drivers should now be capable of responding properly;
- add tests to test40 and fix its error count aggregation.

Change-Id: I57ef58d3afb82640fc50b59c859ee4b25f02db17

distrib/sets/lists/minix/mi
servers/vfs/select.c
test/Makefile
test/t40c.c
test/t40g.c [new file with mode: 0644]
test/test40.c

index fe1621c5e78ac7853d876d4cad036f8d82a785b0..4f5a1493d00491340fc2221af1b8fc2680a5d1b2 100644 (file)
 ./usr/tests/minix-posix/t40d           minix-sys
 ./usr/tests/minix-posix/t40e           minix-sys
 ./usr/tests/minix-posix/t40f           minix-sys
+./usr/tests/minix-posix/t40g           minix-sys
 ./usr/tests/minix-posix/t60a           minix-sys
 ./usr/tests/minix-posix/t60b           minix-sys
 ./usr/tests/minix-posix/t67a           minix-sys
index 073d1cea374f4f36dfb431566860de9903ba56d8..ace48a3979bf37e708d03971dc23e49f21993068 100644 (file)
@@ -55,10 +55,10 @@ static void restart_proc(struct selectentry *se);
 static void ops2tab(int ops, int fd, struct selectentry *e);
 static int is_regular_file(struct filp *f);
 static int is_pipe(struct filp *f);
-static int is_supported_major(struct filp *f);
+static int is_char_device(struct filp *f);
 static void select_lock_filp(struct filp *f, int ops);
 static int select_request_file(struct filp *f, int *ops, int block);
-static int select_request_major(struct filp *f, int *ops, int block);
+static int select_request_char(struct filp *f, int *ops, int block);
 static int select_request_pipe(struct filp *f, int *ops, int block);
 static void select_cancel_all(struct selectentry *e);
 static void select_cancel_filp(struct filp *f);
@@ -71,18 +71,11 @@ static struct fdtype {
        int (*select_request)(struct filp *, int *ops, int block);
        int (*type_match)(struct filp *f);
 } fdtypes[] = {
-       { select_request_major, is_supported_major },
+       { select_request_char, is_char_device },
        { select_request_file, is_regular_file },
        { select_request_pipe, is_pipe },
 };
 #define SEL_FDS                (sizeof(fdtypes) / sizeof(fdtypes[0]))
-static int select_majors[] = { /* List of majors that support selecting on */
-       TTY_MAJOR,
-       INET_MAJOR,
-       UDS_MAJOR,
-       LOG_MAJOR,
-};
-#define SEL_MAJORS     (sizeof(select_majors) / sizeof(select_majors[0]))
 
 /*===========================================================================*
  *                             do_select                                    *
@@ -197,9 +190,11 @@ int do_select(message *UNUSED(m_out))
         * TTY major and sockets by either INET major (socket type AF_INET) or
         * PFS major (socket type AF_UNIX). PFS acts as an FS when it handles
         * pipes and as a driver when it handles sockets. Additionally, we
-        * support select on the LOG major to handle kernel logging, which is
-        * beyond the POSIX spec. */
-
+        * give other character drivers the chance to handle select for any of
+        * their device nodes. Some may not implement support for select and
+        * let libchardriver return EBADF, which we then pass to the calling
+        * process once we receive the reply.
+        */
        se->type[fd] = -1;
        for (type = 0; type < SEL_FDS; type++) {
                if (fdtypes[type].type_match(f)) {
@@ -222,9 +217,21 @@ int do_select(message *UNUSED(m_out))
        if (!(ops = tab2ops(fd, se)))
                continue; /* No operations set; nothing to do for this fd */
 
+       /* File descriptors selected for reading that are not opened for
+        * reading should be marked as readable, as read calls would fail
+        * immediately. The same applies to writing.
+        */
+       f = se->filps[fd];
+       if ((ops & SEL_RD) && !(f->filp_mode & R_BIT)) {
+               ops2tab(SEL_RD, fd, se);
+               ops &= ~SEL_RD;
+       }
+       if ((ops & SEL_WR) && !(f->filp_mode & W_BIT)) {
+               ops2tab(SEL_WR, fd, se);
+               ops &= ~SEL_WR;
+       }
        /* Test filp for select operations if not already done so. e.g.,
         * processes sharing a filp and both doing a select on that filp. */
-       f = se->filps[fd];
        if ((f->filp_select_ops & ops) != ops) {
                int wantops;
 
@@ -235,7 +242,6 @@ int do_select(message *UNUSED(m_out))
                unlock_filp(f);
                if (r != OK && r != SUSPEND) {
                        se->error = r;
-                       se->block = 0;  /* Stop blocking to return asap */
                        break; /* Error or bogus return code; abort */
                }
 
@@ -250,20 +256,21 @@ int do_select(message *UNUSED(m_out))
   /* At this point there won't be any blocking calls anymore. */
   se->starting = FALSE;
 
-  if ((se->nreadyfds > 0 || !se->block) && !is_deferred(se)) {
-       /* fd's were found that were ready to go right away, and/or
-        * we were instructed not to block at all. Must return
-        * immediately.
+  if ((se->nreadyfds > 0 || se->error != OK || !se->block) &&
+               !is_deferred(se)) {
+       /* An error occurred, or fd's were found that were ready to go right
+        * away, and/or we were instructed not to block at all. Must return
+        * immediately. Do not copy FD sets if an error occurred.
         */
-       r = copy_fdsets(se, se->nfds, TO_PROC);
+       if (se->error != OK)
+               r = se->error;
+       else
+               r = copy_fdsets(se, se->nfds, TO_PROC);
        select_cancel_all(se);
        se->requestor = NULL;
 
        if (r != OK)
                return(r);
-       else if (se->error != OK)
-               return(se->error);
-
        return(se->nreadyfds);
   }
 
@@ -335,35 +342,26 @@ static int is_pipe(struct filp *f)
 }
 
 /*===========================================================================*
- *                             is_supported_major                           *
+ *                             is_char_device                               *
  *===========================================================================*/
-static int is_supported_major(struct filp *f)
+static int is_char_device(struct filp *f)
 {
-/* See if this filp is a handle on a character device on which we support
- * select(). This function MUST NOT block its calling thread. The given filp
- * may or may not be locked.
+/* See if this filp is a handle on a character device. This function MUST NOT
+ * block its calling thread. The given filp may or may not be locked.
  */
-  unsigned int m;
-
-  if (!(f && f->filp_vno)) return(FALSE);
-  if (!S_ISCHR(f->filp_vno->v_mode)) return(FALSE);
-
-  for (m = 0; m < SEL_MAJORS; m++)
-       if (major(f->filp_vno->v_sdev) == select_majors[m])
-               return(TRUE);
 
-  return(FALSE);
+  return (f && f->filp_vno && S_ISCHR(f->filp_vno->v_mode));
 }
 
 /*===========================================================================*
- *                             select_request_major                         *
+ *                             select_request_char                          *
  *===========================================================================*/
-static int select_request_major(struct filp *f, int *ops, int block)
+static int select_request_char(struct filp *f, int *ops, int block)
 {
-/* Check readiness status on a supported character device. Unless suitable
- * results are available right now, this will only initiate the polling
- * process, causing result processing to be deferred. This function MUST NOT
- * block its calling thread. The given filp may or may not be locked.
+/* Check readiness status on a character device. Unless suitable results are
+ * available right now, this will only initiate the polling process, causing
+ * result processing to be deferred. This function MUST NOT block its calling
+ * thread. The given filp may or may not be locked.
  */
   int r, rops, major;
   struct dmap *dp;
@@ -450,12 +448,6 @@ static int select_request_pipe(struct filp *f, int *ops, int block)
                r |= SEL_RD;
        if (err < 0 && err != SUSPEND)
                r |= SEL_ERR;
-       if (err == SUSPEND && !(f->filp_mode & R_BIT)) {
-               /* A "meaningless" read select, therefore ready
-                * for reading and no error set. */
-               r |= SEL_RD;
-               r &= ~SEL_ERR;
-       }
   }
 
   if ((*ops & (SEL_WR|SEL_ERR))) {
@@ -467,12 +459,6 @@ static int select_request_pipe(struct filp *f, int *ops, int block)
                r |= SEL_WR;
        if (err < 0 && err != SUSPEND)
                r |= SEL_ERR;
-       if (err == SUSPEND && !(f->filp_mode & W_BIT)) {
-               /* A "meaningless" write select, therefore ready
-                   for writing and no error set. */
-               r |= SEL_WR;
-               r &= ~SEL_ERR;
-       }
   }
 
   /* Some options we collected might not be requested. */
@@ -628,7 +614,7 @@ static void select_cancel_filp(struct filp *f)
         * character device, mark the query as stale, so that this filp will
         * not be checked when the result arrives.
         */
-       if (is_supported_major(f)) {
+       if (is_char_device(f)) {
                major = major(f->filp_vno->v_sdev);
                if (dmap[major].dmap_sel_busy &&
                        dmap[major].dmap_sel_filp == f)
@@ -645,18 +631,17 @@ static void select_return(struct selectentry *se)
 /* Return the results of a select call to the user process and revive the
  * process. This function MUST NOT block its calling thread.
  */
-  int r, r1;
+  int r;
 
   assert(!is_deferred(se));    /* Not done yet, first wait for async reply */
 
   select_cancel_all(se);
 
-  r1 = copy_fdsets(se, se->nfds, TO_PROC);
-  if (r1 != OK)
-       r = r1;
-  else if (se->error != OK)
+  if (se->error != OK)
        r = se->error;
   else
+       r = copy_fdsets(se, se->nfds, TO_PROC);
+  if (r == OK)
        r = se->nreadyfds;
 
   revive(se->req_endpt, r);
@@ -850,7 +835,7 @@ int status;
         */
        if (!(f->filp_select_flags & (FSF_UPDATE|FSF_BLOCKED)))
                f->filp_select_ops = 0;         /* done selecting */
-       else if (!(f->filp_select_flags & FSF_UPDATE))
+       else if (status > 0 && !(f->filp_select_flags & FSF_UPDATE))
                /* there may be operations pending */
                f->filp_select_ops &= ~status;
 
@@ -888,7 +873,7 @@ int status;
  * the select request is 'blocking' until an operation becomes ready. This
  * function MUST NOT block its calling thread.
  */
-  int major, slot, fd;
+  int major, slot, found, fd;
   dev_t dev;
   struct filp *f;
   struct dmap *dp;
@@ -915,6 +900,7 @@ int status;
        se = &selecttab[slot];
        if (se->requestor == NULL) continue;    /* empty slot */
 
+       found = FALSE;
        for (fd = 0; fd < se->nfds; fd++) {
                if ((f = se->filps[fd]) == NULL) continue;
                if ((vp = f->filp_vno) == NULL) continue;
@@ -937,10 +923,16 @@ int status;
                        ops2tab(status, fd, se);
                } else {
                        f->filp_select_flags &= ~FSF_BLOCKED;
-                       ops2tab(SEL_RD|SEL_WR|SEL_ERR, fd, se);
+                       se->error = status;
                }
-               if (se->nreadyfds > 0) restart_proc(se);
+               found = TRUE;
        }
+       /* Even if 'found' is set now, nothing may have changed for this call,
+        * as it may not have been interested in the operations that were
+        * reported as ready. Let restart_proc check.
+        */
+       if (found)
+               restart_proc(se);
   }
 
   select_restart_filps();
@@ -984,13 +976,12 @@ static void select_restart_filps(void)
                 * particular, checking pipes the same way would introduce a
                 * serious locking problem.
                 */
-               assert(is_supported_major(f));
+               assert(is_char_device(f));
 
                wantops = ops = f->filp_select_ops;
-               r = select_request_major(f, &wantops, se->block);
+               r = select_request_char(f, &wantops, se->block);
                if (r != OK && r != SUSPEND) {
                        se->error = r;
-                       se->block = 0;  /* Stop blocking to return asap */
                        restart_proc(se);
                        break; /* Error or bogus return code; abort */
                }
@@ -1009,21 +1000,24 @@ int status;
 /* Tell processes that need to know about the status of this filp. This
  * function MUST NOT block its calling thread.
  */
-  int fd, slot;
+  int fd, slot, found;
   struct selectentry *se;
 
   for (slot = 0; slot < MAXSELECTS; slot++) {
        se = &selecttab[slot];
        if (se->requestor == NULL) continue; /* empty slot */
 
+       found = FALSE;
        for (fd = 0; fd < se->nfds; fd++) {
                if (se->filps[fd] != f) continue;
                if (status < 0)
-                       ops2tab(SEL_RD|SEL_WR|SEL_ERR, fd, se);
+                       se->error = status;
                else
                        ops2tab(status, fd, se);
-               restart_proc(se);
+               found = TRUE;
        }
+       if (found)
+               restart_proc(se);
   }
 }
 
@@ -1037,7 +1031,7 @@ struct selectentry *se;
  * pending. This function MUST NOT block its calling thread.
  */
 
-  if ((se->nreadyfds > 0 || !se->block) && !is_deferred(se))
+  if ((se->nreadyfds > 0 || se->error != OK || !se->block) && !is_deferred(se))
        select_return(se);
 }
 
index c3cef47ed036be16d40c2f2bb2695189b8cdb333..331cb3b3e124dcd92be71a6703d8dda5da6c24ee 100644 (file)
@@ -67,7 +67,7 @@ MINIX_TESTS+= \
 PROGS+= test${t}
 .endfor
   
-PROGS+=        t10a t11a t11b t40a t40b t40c t40d t40e t40f t60a t60b \
+PROGS+=        t10a t11a t11b t40a t40b t40c t40d t40e t40f t40g t60a t60b \
        t67a t67b t68a t68b
 
 SCRIPTS+= run testinterp.sh testsh1.sh testsh2.sh testmfs.sh testisofs.sh
index 6a71c4d9daba5591908b3c4c6db0c29d3b9867fa..d78f981aa447764572e0ea2ac331d08834daaa70 100644 (file)
@@ -60,26 +60,28 @@ static void open_terminal(int *child_fd, int *parent_fd) {
 }
 
 static int do_child(int terminal) {
-  struct timeval tv;
-
   /* Going to sleep for two seconds to allow the parent proc to get ready */
-  tv.tv_sec = 2;
-  tv.tv_usec = 0;
-  select(0, NULL, NULL, NULL, &tv);
+  sleep(2);
 
   /* Try to write. Doesn't matter how many bytes we actually send. */
   (void) write(terminal, SENDSTRING, strlen(SENDSTRING));
-  close(terminal);
 
   /* Wait for another second to allow the parent to process incoming data */
-  tv.tv_usec = 1000000;
-  (void) select(0,NULL, NULL, NULL, &tv);
+  sleep(1);
+
+  /* Write some more, and wait some more. */
+  (void) write(terminal, SENDSTRING, strlen(SENDSTRING));
+
+  sleep(1);
+
+  close(terminal);
   exit(0);
 }
 
 static int do_parent(int child, int terminal) {
-  fd_set fds_read, fds_write, fds_error;
-  int retval;
+  fd_set fds_read, fds_read2, fds_write, fds_error;
+  int retval, terminal2, highest;
+  char buf[256];
 
   /* Clear bit masks */
   FD_ZERO(&fds_read); FD_ZERO(&fds_write); FD_ZERO(&fds_error);
@@ -96,7 +98,6 @@ static int do_parent(int child, int terminal) {
   
   if(retval != 1) em(1, "incorrect amount of ready file descriptors");
 
-
   if(FD_ISSET(terminal, &fds_read)) em(2, "read should NOT be set");
   if(!FD_ISSET(terminal, &fds_write)) em(3, "write should be set");
   if(FD_ISSET(terminal, &fds_error)) em(4, "error should NOT be set");
@@ -110,7 +111,6 @@ static int do_parent(int child, int terminal) {
   if(!FD_ISSET(terminal, &fds_read)) em(6, "read should be set");
   if(FD_ISSET(terminal, &fds_error)) em(7, "error should not be set");
 
-
   FD_ZERO(&fds_read); FD_ZERO(&fds_error);
   FD_SET(terminal, &fds_write);
   retval = select(terminal+1, NULL, &fds_write, NULL, NULL);
@@ -118,6 +118,33 @@ static int do_parent(int child, int terminal) {
    * immediately with fd set in fds_write. */
   if(retval != 1) em(8, "incorrect amount or ready file descriptors");
 
+  /* See if selecting on the same object with two different fds results in both
+   * fds being returned as ready, immediately.
+   */
+  terminal2 = dup(terminal);
+  if (terminal2 < 0) em(9, "unable to dup file descriptor");
+
+  FD_ZERO(&fds_read);
+  FD_SET(terminal, &fds_read);
+  FD_SET(terminal2, &fds_read);
+  fds_read2 = fds_read;
+  highest = terminal > terminal2 ? terminal : terminal2;
+
+  retval = select(highest+1, &fds_read, NULL, NULL, NULL);
+  if (retval != 2) em(10, "incorrect amount of ready file descriptors");
+  if (!FD_ISSET(terminal, &fds_read)) em(11, "first fd missing from set");
+  if (!FD_ISSET(terminal2, &fds_read)) em(12, "second fd missing from set");
+
+  /* Empty the buffer. */
+  if (read(terminal, buf, sizeof(buf)) <= 0) em(13, "unable to read data");
+
+  /* Repeat the test, now with a delay. */
+  retval = select(highest+1, &fds_read2, NULL, NULL, NULL);
+  if (retval != 2) em(10, "incorrect amount of ready file descriptors");
+  if (!FD_ISSET(terminal, &fds_read2)) em(11, "first fd missing from set");
+  if (!FD_ISSET(terminal2, &fds_read2)) em(12, "second fd missing from set");
+
+  close(terminal2);
   close(terminal);
   waitpid(child, &retval, 0);
   exit(errct);
diff --git a/test/t40g.c b/test/t40g.c
new file mode 100644 (file)
index 0000000..038a907
--- /dev/null
@@ -0,0 +1,52 @@
+/* t40g.c
+ *
+ * Test select on character driver that does not support select
+ *
+ * We use /dev/zero for this right now.  If the memory driver ever implements
+ * select support, this test should be changed to use another character driver.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/select.h>
+#include <sys/wait.h>
+#include <sys/time.h>
+#include <time.h>
+#include <errno.h>
+#include <string.h>
+#include <signal.h>
+#include <fcntl.h>
+
+#include "common.h"
+
+int
+main(int argc, char **argv)
+{
+       fd_set set;
+       int fd, retval;
+
+       /* Get subtest number */
+       if (argc != 2 || sscanf(argv[1], "%d", &subtest) != 1) {
+               printf("Usage: %s subtest_no\n", argv[0]);
+               exit(-2);
+       }
+
+       /*
+        * Do a select on /dev/zero, with the expectation that it will fail
+        * with an EBADF error code.
+        */
+       fd = open("/dev/zero", O_RDONLY);
+       if (fd < 0) em(1, "unable to open /dev/zero");
+
+       FD_ZERO(&set);
+       FD_SET(fd, &set);
+
+       retval = select(fd + 1, &set, NULL, NULL, NULL);
+       if (retval != -1) em(2, "select call was expected to fail");
+       if (errno != EBADF) em(3, "error code other than EBADF returned");
+       if (!FD_ISSET(fd, &set)) em(4, "file descriptor set was modified");
+
+       exit(errct);
+}
index 686f02268ad4c15938075f891946d7ebbeda3dd6..15a36e3ca9a01355ce952c112e3f03055bdee14a 100644 (file)
@@ -16,9 +16,9 @@ int max_error = 5;
 
 
 int main(int argc, char **argv) {
-  char *tests[] = {"t40a", "t40b", "t40c", "t40d", "t40e", "t40f"};
+  char *tests[] = {"t40a", "t40b", "t40c", "t40d", "t40e", "t40f", "t40g"};
   char copy_command[8+PATH_MAX+1];
-  int no_tests, i, forkres, status = 0, errorct = 0;
+  int no_tests, i, forkres, status = 0;
 
   no_tests = sizeof(tests) / sizeof(char *);
   
@@ -39,7 +39,7 @@ int main(int argc, char **argv) {
       exit(-2);
     } else if(forkres > 0) { /* Parent */
       if(waitpid(forkres, &status, 0) > 0 && WEXITSTATUS(status) < 20) {
-       errorct += WEXITSTATUS(status); /* Count errors */
+       errct += WEXITSTATUS(status); /* Count errors */
       }
       status = 0; /* Reset */
     } else {