VFS: select(2) fixes

- 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

@@ -5581,6 +5581,7 @@
 ./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

servers/vfs/select.c

@@ -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
+	 * give other character drivers the chance to handle select for any of
-	 * beyond the POSIX spec. */
+	 * 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)) {
+  if ((se->nreadyfds > 0 || se->error != OK || !se->block) &&
-	/* fd's were found that were ready to go right away, and/or
+		!is_deferred(se)) {
-	 * we were instructed not to block at all. Must return
+	/* An error occurred, or fd's were found that were ready to go right
-	 * immediately.
+	 * 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
+/* See if this filp is a handle on a character device. This function MUST NOT
- * select(). This function MUST NOT block its calling thread. The given filp
+ * block its calling thread. The given filp may or may not be locked.
- * may or may not be locked.
  */
-  unsigned int m;
 
-  if (!(f && f->filp_vno)) return(FALSE);
+  return (f && f->filp_vno && S_ISCHR(f->filp_vno->v_mode));
-  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);
 }
 
 /*===========================================================================*
- *				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
+/* Check readiness status on a character device. Unless suitable results are
- * results are available right now, this will only initiate the polling
+ * available right now, this will only initiate the polling process, causing
- * process, causing result processing to be deferred. This function MUST NOT
+ * result processing to be deferred. This function MUST NOT block its calling
- * block its calling thread. The given filp may or may not be locked.
+ * 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 (se->error != OK)
-  if (r1 != OK)
-	r = r1;
-  else 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);
 }
 

test/Makefile

@@ -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

test/t40c.c

@@ -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;
+  sleep(2);
-  tv.tv_usec = 0;
-  select(0, NULL, NULL, NULL, &tv);
 
   /* 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;
+  sleep(1);
-  (void) select(0,NULL, NULL, NULL, &tv);
+
+  /* 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;
+  fd_set fds_read, fds_read2, fds_write, fds_error;
-  int retval;
+  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);

test/t40g.c

@@ -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);
+}

test/test40.c

@@ -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 {