diff mbox

BZ#15819: introduce internal function to ease poll retry with timeout

Message ID orioiiahsf.fsf@free.home
State New
Headers show

Commit Message

Alexandre Oliva Nov. 13, 2014, 9:53 p.m. UTC 
Here's a formal submission of the patch I posted asking for feedback on
how to introduce this sort of wrapper, a few days ago.

Regression tested on x86_64-linux-gnu.  Ok to install?


for  ChangeLog

	BZ #15819
	* NEWS: Update.
	* include/sys/poll.h: Include errno.h and sys/time.h.
	(__poll_noeintr): New function.
	* nis/nis_callback.c (internal_nis_do_callback): Use new fn,
	drop TEMP_FAILURE_RETRY.
	* nscd/nscd_helper.c (wait_on_socket): Use new fn, drop loop
	and gettimeofday timeout recomputation.
	* sunrpc/clnt_udp.c (clntudp_call): Use new fn.
	* sunrpc/clnt_unix.c (readunix): Likewise.
	* sunrpc/rtime.c (rtime): Likewise.
	* sunrpc/svc_run.c (svc_run): Likewise.
	* sunrpc/svc_tcp.c (readtcp): Likewise.
	* sunrpc/svc_unix.c (readunix): Likewise.
---
 NEWS               |    8 +++----
 include/sys/poll.h |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 nis/nis_callback.c |    3 +-
 nscd/nscd_helper.c |   24 +-------------------
 sunrpc/clnt_udp.c  |    5 +---
 sunrpc/clnt_unix.c |   22 +++++++-----------
 sunrpc/rtime.c     |    4 +--
 sunrpc/svc_run.c   |    4 +--
 sunrpc/svc_tcp.c   |   29 +++++++++---------------
 sunrpc/svc_unix.c  |   29 +++++++++---------------
 10 files changed, 101 insertions(+), 90 deletions(-)

Comments

Roland McGrath Nov. 13, 2014, 10:18 p.m. UTC | #1 
> Here's a formal submission of the patch I posted asking for feedback on
> how to introduce this sort of wrapper, a few days ago.

Did you not see my feedback saying to prefer new local headers instead of
changes to include/ wrapper headers?

> +static inline int
> +__poll_noeintr (struct pollfd *__fds, unsigned long int __nfds,
> +		int __timeout)
> +{
> +  int __ret;
> +
> + __retry_poll:
> +  __ret = __poll (__fds, __nfds, __timeout);

The __ treatment is never necessary for local-scope names in internal
source files (including headers).  It only matters for global names, or
uses in installed headers.  Drop the unnecessary prefixes to make the code
easier to read.


Thanks,
Roland
Alexandre Oliva Nov. 14, 2014, 12:38 a.m. UTC | #2 
On Nov 13, 2014, Roland McGrath <roland@hack.frob.com> wrote:

> This too has apparently ignored my feedback

Now, now, that's quite an unfair assessment!  ;-)  See below.


On Nov 13, 2014, Roland McGrath <roland@hack.frob.com> wrote:

> Did you not see my feedback

I did, and I'm afraid I followed it strictly to the letter, even where
it made for more work for myself :-(


> saying to prefer new local headers instead of
> changes to include/ wrapper headers?

You wrote new local headers should be preferred over magic in internal
headers.  I reasoned that, since I had not suggested adding any magic to
internal headers, this was general guidance that didn't apply.  Even
more so given that you'd suggested I should untangle the headers, which
would only make sense as part of these changes if we were to keep on
using the same headers, rather than introducing new ones.  Untangling
the headers would require the usual __need_* idiom, and that was higher
in your comments (and thus priorities, I reasoned) than introducing new
headers.  So...  I didn't ignore your feedback, I just got something
different from them than what you meant :-(


Anyway...  Now that I (hope I) know what you really meant to suggest,
I'll simplify and adjust the patches to match my current understanding.
Biab, ;-)
diff mbox

Patch

diff --git a/NEWS b/NEWS
index 9ed697c..af86a93 100644
--- a/NEWS
+++ b/NEWS
@@ -9,10 +9,10 @@  Version 2.21
 
 * The following bugs are resolved with this release:
 
-  6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 17266, 17344,
-  17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508,
-  17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584,
-  17585, 17589.
+  6652, 12926, 14132, 14138, 14171, 14498, 15215, 15819, 15884, 17266,
+  17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506,
+  17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583,
+  17584, 17585, 17589.
 
 * New locales: tu_IN, bh_IN.
 
diff --git a/include/sys/poll.h b/include/sys/poll.h
index a42bc93..bfb5d8a 100644
--- a/include/sys/poll.h
+++ b/include/sys/poll.h
@@ -6,6 +6,67 @@  extern int __poll (struct pollfd *__fds, unsigned long int __nfds,
 		   int __timeout);
 libc_hidden_proto (__poll)
 libc_hidden_proto (ppoll)
-#endif
+
+#include <errno.h>
+#include <time.h>
+
+static inline int
+__poll_noeintr (struct pollfd *__fds, unsigned long int __nfds,
+		int __timeout)
+{
+  int __ret;
+
+ __retry_poll:
+  __ret = __poll (__fds, __nfds, __timeout);
+
+  if (__ret == -1 && __glibc_unlikely (errno == EINTR))
+    {
+      /* Handle the case where the poll() call is interrupted by a
+	 signal.  We cannot just use TEMP_FAILURE_RETRY since it might
+	 lead to infinite loops.  We can't tell how long poll has
+	 already waited, and we can't assume the existence of a
+	 higher-precision clock, but that's ok-ish: the timeout is a
+	 lower bound, we just have to make sure we don't wait
+	 indefinitely.  */
+      struct timespec __tscur, __tsend;
+      /* Remember which clock to use.  */
+      static clockid_t __xclk = CLOCK_MONOTONIC;
+      clockid_t __clk = __xclk;
+
+    __try_another_clock:
+      if (__glibc_unlikely (__clock_gettime (__clk, &__tscur) == -1))
+	{
+	  if (errno == EINVAL && __clk == CLOCK_MONOTONIC)
+	    {
+	      __xclk = __clk = CLOCK_REALTIME;
+	      goto __try_another_clock;
+	    }
+
+	  /* At least CLOCK_REALTIME should always be supported, but
+	     if clock_gettime fails for any other reason, the best we
+	     can do is to retry with a slightly lower timeout, until
+	     we complete without interruption.  */
+	  __timeout--;
+	  goto __retry_poll;
+	}
+	  
+      __tsend.tv_sec = __tscur.tv_sec + __timeout / 1000;
+      __tsend.tv_nsec = __tscur.tv_nsec + __timeout % 1000 * 1000000L + 500000;
+	  
+      while (1)
+	{
+	  __ret = __poll (__fds, __nfds,
+			  (__tsend.tv_sec - __tscur.tv_sec) * 1000
+			  + (__tsend.tv_nsec - __tscur.tv_nsec) / 1000000);
+	  if (__ret != -1 || errno != EINTR)
+	    break;
+	      
+	  (void) __clock_gettime (__clk, &__tscur);
+	}
+    }
+
+  return __ret;
+}
+#endif /* _ISOMAC */
 
 #endif
diff --git a/nis/nis_callback.c b/nis/nis_callback.c
index e352733..baa8d75 100644
--- a/nis/nis_callback.c
+++ b/nis/nis_callback.c
@@ -215,8 +215,7 @@  internal_nis_do_callback (struct dir_binding *bptr, netobj *cookie,
           my_pollfd[i].revents = 0;
         }
 
-      switch (i = TEMP_FAILURE_RETRY (__poll (my_pollfd, svc_max_pollfd,
-					      25*1000)))
+      switch (i = __poll_noeintr (my_pollfd, svc_max_pollfd, 25*1000))
         {
 	case -1:
 	  return NIS_CBERROR;
diff --git a/nscd/nscd_helper.c b/nscd/nscd_helper.c
index ee3b67f..fe5f096 100644
--- a/nscd/nscd_helper.c
+++ b/nscd/nscd_helper.c
@@ -53,29 +53,7 @@  wait_on_socket (int sock, long int usectmo)
   struct pollfd fds[1];
   fds[0].fd = sock;
   fds[0].events = POLLIN | POLLERR | POLLHUP;
-  int n = __poll (fds, 1, usectmo);
-  if (n == -1 && __builtin_expect (errno == EINTR, 0))
-    {
-      /* Handle the case where the poll() call is interrupted by a
-	 signal.  We cannot just use TEMP_FAILURE_RETRY since it might
-	 lead to infinite loops.  */
-      struct timeval now;
-      (void) __gettimeofday (&now, NULL);
-      long int end = now.tv_sec * 1000 + usectmo + (now.tv_usec + 500) / 1000;
-      long int timeout = usectmo;
-      while (1)
-	{
-	  n = __poll (fds, 1, timeout);
-	  if (n != -1 || errno != EINTR)
-	    break;
-
-	  /* Recompute the timeout time.  */
-	  (void) __gettimeofday (&now, NULL);
-	  timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000);
-	}
-    }
-
-  return n;
+  return __poll_noeintr (fds, 1, usectmo);
 }
 
 
diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
index 6ffa5f2..f1e6500 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -378,9 +378,8 @@  send_again:
   anyup = 0;
   for (;;)
     {
-      switch (__poll (&fd, 1, milliseconds))
+      switch (__poll_noeintr (&fd, 1, milliseconds))
 	{
-
 	case 0:
 	  if (anyup == 0)
 	    {
@@ -407,8 +406,6 @@  send_again:
 	   * updated.
 	   */
 	case -1:
-	  if (errno == EINTR)
-	    continue;
 	  cu->cu_error.re_errno = errno;
 	  return (cu->cu_error.re_status = RPC_CANTRECV);
 	}
diff --git a/sunrpc/clnt_unix.c b/sunrpc/clnt_unix.c
index 32d88b9..aff6fa5 100644
--- a/sunrpc/clnt_unix.c
+++ b/sunrpc/clnt_unix.c
@@ -551,22 +551,16 @@  readunix (char *ctptr, char *buf, int len)
 
   fd.fd = ct->ct_sock;
   fd.events = POLLIN;
-  while (TRUE)
+  switch (__poll_noeintr (&fd, 1, milliseconds))
     {
-      switch (__poll (&fd, 1, milliseconds))
-	{
-	case 0:
-	  ct->ct_error.re_status = RPC_TIMEDOUT;
-	  return -1;
+    case 0:
+      ct->ct_error.re_status = RPC_TIMEDOUT;
+      return -1;
 
-	case -1:
-	  if (errno == EINTR)
-	    continue;
-	  ct->ct_error.re_status = RPC_CANTRECV;
-	  ct->ct_error.re_errno = errno;
-	  return -1;
-	}
-      break;
+    case -1:
+      ct->ct_error.re_status = RPC_CANTRECV;
+      ct->ct_error.re_errno = errno;
+      return -1;
     }
   switch (len = __msgread (ct->ct_sock, buf, len))
     {
diff --git a/sunrpc/rtime.c b/sunrpc/rtime.c
index d224624..79d55d1 100644
--- a/sunrpc/rtime.c
+++ b/sunrpc/rtime.c
@@ -102,9 +102,7 @@  rtime (struct sockaddr_in *addrp, struct rpc_timeval *timep,
       milliseconds = (timeout->tv_sec * 1000) + (timeout->tv_usec / 1000);
       fd.fd = s;
       fd.events = POLLIN;
-      do
-	res = __poll (&fd, 1, milliseconds);
-      while (res < 0 && errno == EINTR);
+      res = __poll_noeintr (&fd, 1, milliseconds);
       if (res <= 0)
 	{
 	  if (res == 0)
diff --git a/sunrpc/svc_run.c b/sunrpc/svc_run.c
index 90dfc94..5e20aae 100644
--- a/sunrpc/svc_run.c
+++ b/sunrpc/svc_run.c
@@ -83,11 +83,9 @@  svc_run (void)
 	  my_pollfd[i].revents = 0;
 	}
 
-      switch (i = __poll (my_pollfd, max_pollfd, -1))
+      switch (i = __poll_noeintr (my_pollfd, max_pollfd, -1))
 	{
 	case -1:
-	  if (errno == EINTR)
-	    continue;
 	  perror (_("svc_run: - poll failed"));
 	  break;
 	case 0:
diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c
index 913f05f..92886f0 100644
--- a/sunrpc/svc_tcp.c
+++ b/sunrpc/svc_tcp.c
@@ -317,26 +317,19 @@  readtcp (char *xprtptr, char *buf, int len)
   int milliseconds = 35 * 1000;
   struct pollfd pollfd;
 
-  do
+  pollfd.fd = sock;
+  pollfd.events = POLLIN;
+  switch (__poll_noeintr (&pollfd, 1, milliseconds))
     {
-      pollfd.fd = sock;
-      pollfd.events = POLLIN;
-      switch (__poll (&pollfd, 1, milliseconds))
-	{
-	case -1:
-	  if (errno == EINTR)
-	    continue;
-	  /*FALLTHROUGH*/
-	case 0:
-	  goto fatal_err;
-	default:
-	  if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP)
-	      || (pollfd.revents & POLLNVAL))
-	    goto fatal_err;
-	  break;
-	}
+    case -1:
+    case 0:
+      goto fatal_err;
+    default:
+      if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP)
+	  || (pollfd.revents & POLLNVAL))
+	goto fatal_err;
+      break;
     }
-  while ((pollfd.revents & POLLIN) == 0);
 
   if ((len = __read (sock, buf, len)) > 0)
     return len;
diff --git a/sunrpc/svc_unix.c b/sunrpc/svc_unix.c
index 963276b2..6d93c5f 100644
--- a/sunrpc/svc_unix.c
+++ b/sunrpc/svc_unix.c
@@ -419,26 +419,19 @@  readunix (char *xprtptr, char *buf, int len)
   int milliseconds = 35 * 1000;
   struct pollfd pollfd;
 
-  do
+  pollfd.fd = sock;
+  pollfd.events = POLLIN;
+  switch (__poll_noeintr (&pollfd, 1, milliseconds))
     {
-      pollfd.fd = sock;
-      pollfd.events = POLLIN;
-      switch (__poll (&pollfd, 1, milliseconds))
-	{
-	case -1:
-	  if (errno == EINTR)
-	    continue;
-	  /*FALLTHROUGH*/
-	case 0:
-	  goto fatal_err;
-	default:
-	  if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP)
-	      || (pollfd.revents & POLLNVAL))
-	    goto fatal_err;
-	  break;
-	}
+    case -1:
+    case 0:
+      goto fatal_err;
+    default:
+      if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP)
+	  || (pollfd.revents & POLLNVAL))
+	goto fatal_err;
+      break;
     }
-  while ((pollfd.revents & POLLIN) == 0);
 
   if ((len = __msgread (sock, buf, len)) > 0)
     return len;