Thanks for cleaning up my code. It's much better now.

I didn't know the userspace names of kernel types. I didn't think a 32bit userspace calling a 64bit kernel would break because there's a compat_sys_pselect6() in linux.

-N

On Sun, Dec 27, 2015 at 9:46 AM, Leonid Lisovskiy <lly.dev@gmail.com> wrote:
Commit e3c3bf2b58 introduce use of pselect6, but has following disadvantages:

* Use of userspace types in args67 structure - it breaks, for example,
  configs when 32-bit uClibc-ng compiled against 64-bit kernel. Syscall
  will always return EINVAL. We must use __kernel_* types and
  __SYSCALL_SIGSET_T_SIZE.

* It have excess checks for NSEC_PER_SEC. Original code from select()
  implementation has struct timeval => struct timespec conversion,
  kernel select() syscall implementation do the same.
  But none of libc versions (glibc, eglibc, musl) I know, perform similar
  checks for pselect() - there is no structure fields conversions,
  just struct timespec through all the calls.
  To have such checks in uClibc-ng we need one example, at least.

* It is possible to avoid extra userspace reads from kernel code if
  sigmask == NULL. I suggest to do it, for a few bytes cost.

* Commit didn't add test case to testsuite.

Signed-off-by: Leonid Lisovskiy <lly.dev@gmail.com>
---
 libc/sysdeps/linux/common/pselect.c | 67 ++++++++++++-------------------------
 test/unistd/tst-pselect.c           | 51 ++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 45 deletions(-)
 create mode 100644 test/unistd/tst-pselect.c

diff --git a/libc/sysdeps/linux/common/pselect.c b/libc/sysdeps/linux/common/pselect.c
index 3f1dd28..fbe85b7 100644
--- a/libc/sysdeps/linux/common/pselect.c
+++ b/libc/sysdeps/linux/common/pselect.c
@@ -31,55 +31,32 @@ static int __NC(pselect)(int nfds, fd_set *readfds, fd_set *writefds,
                         const sigset_t *sigmask)
 {
 #ifdef __NR_pselect6
-#define NSEC_PER_SEC 1000000000L
-       struct timespec _ts, *ts = 0;
-       if (timeout) {
-               /* The Linux kernel can in some situations update the timeout value.
-                * We do not want that so use a local variable.
-                */
+       /* The Linux kernel can in some situations update the timeout value.
+        * We do not want that so use a local variable.
+        */
+       struct timespec _ts;
+
+       if (timeout != NULL) {
                _ts = *timeout;
+               timeout = &_ts;
+       }
+       /* Note: the system call expects 7 values but on most architectures
+          we can only pass in 6 directly.  If there is an architecture with
+          support for more parameters a new version of this file needs to
+          be created.  */
+       struct {
+               __kernel_ulong_t ss;
+               __kernel_size_t  ss_len;
+       } data;

-               /* GNU extension: allow for timespec values where the sub-sec
-               * field is equal to or more than 1 second.  The kernel will
-               * reject this on us, so take care of the time shift ourself.
-               * Some applications (like readline and linphone) do this.
-               * See 'clarification on select() type calls and invalid timeouts'
-               * on the POSIX general list for more information.
-               */
-               if (_ts.tv_nsec >= NSEC_PER_SEC) {
-                       _ts.tv_sec += _ts.tv_nsec / NSEC_PER_SEC;
-                       _ts.tv_nsec %= NSEC_PER_SEC;
-               }
-
-               ts = &_ts;
+       if (sigmask != NULL) {
+               data.ss = (__kernel_ulong_t) sigmask;
+               data.ss_len = __SYSCALL_SIGSET_T_SIZE;
+
+               sigmask = (void *)&data;
        }

-       /* The pselect6 syscall API is strange. It wants a 7th arg to be
-        * the sizeof(*sigmask). However syscalls with > 6 arguments aren't
-        * supported on linux. So arguments 6 and 7 are stuffed in a struct
-        * and a pointer to that struct is passed as the 6th argument to
-        * the syscall.
-        * Glibc stuffs arguments 6 and 7 in a ulong[2]. Linux reads
-        * them as if there were a struct { sigset_t*; size_t } in
-        * userspace. There woudl be trouble if userspace and the kernel are
-        * compiled differently enough that size_t isn't the same as ulong,
-        * but not enough to trigger the compat layer in linux. I can't
-        * think of such a case, so I'm using linux's struct.
-        * Furthermore Glibc sets the sigsetsize to _NSIG/8. However linux
-        * checks for sizeof(sigset_t), which internally is a ulong array.
-        * This means that if _NSIG isn't a multiple of BITS_PER_LONG then
-        * linux will refuse glibc's value. So I prefer sizeof(sigset_t) for
-        * the value of sigsetsize.
-        */
-       struct {
-               const sigset_t *sigmask;
-               size_t sigsetsize;
-       } args67 = {
-               sigmask,
-               sizeof(sigset_t),
-       };
-
-       return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds, exceptfds, ts, &args67);
+       return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds, exceptfds, timeout, sigmask);
 #else
        struct timeval tval;
        int retval;
diff --git a/test/unistd/tst-pselect.c b/test/unistd/tst-pselect.c
new file mode 100644
index 0000000..cab9451
--- /dev/null
+++ b/test/unistd/tst-pselect.c
@@ -0,0 +1,51 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <signal.h>
+#include <sys/types.h>
+#include <sys/select.h>
+
+// our SIGALRM handler
+void handler(int signum) {
+       (void)signum;
+       puts("got signal\n");
+}
+
+static int
+do_test (void)
+{
+       int rc;
+       sigset_t wait_mask, mask_sigchld;
+       struct sigaction act;
+
+       // block SIGALRM. We want to handle it only when we're ready
+       sigemptyset(&mask_sigchld);
+       sigaddset(&mask_sigchld, SIGALRM);
+       sigprocmask(SIG_BLOCK,  &mask_sigchld, &wait_mask);
+       sigdelset(&wait_mask, SIGALRM);
+
+       // register a signal handler so we can see when the signal arrives
+       memset(&act, 0, sizeof(act));
+       sigemptyset(&act.sa_mask); // just in case an empty set isn't all 0's (total paranoia)
+       act.sa_handler = handler;
+       sigaction(SIGALRM, &act, NULL);
+
+       // send ourselves a SIGARLM. It will pend until we unblock that signal in pselect()
+       printf("sending ourselves a signal\n");
+       kill(getpid(), SIGALRM);
+
+       printf("signal is pending; calling pselect()\n");
+       rc = pselect(0, NULL, NULL, NULL, NULL, &wait_mask);
+       if (rc != -1 || errno != EINTR) {
+               int e = errno;
+               printf("pselect() returned %d, errno %d (%s)\n", rc, e, strerror(e));
+               exit(1);
+       }
+
+       return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include <test-skeleton.c>
--
1.8.5.6