diff mbox

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

Message ID or4mu2zfsf.fsf@free.home
State New, archived
Headers

Commit Message

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

> prefer new local headers

*check*

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

I figured names outside the reserved namespace could be used in our
tests, and then they'd interfere.  I guess the reasoning is that, since
we control the tests too, we can just fix any such conflicts in the
tests, rigth?

> Drop the unnecessary prefixes to make the code easier to read.

*check*


This revised patch fixes both of these issues, and it poisons __poll and
poll at the end of include/poll-noeintr.h.


Retested on x86_64-linux-gnu.  Ok to install?


for  ChangeLog

	[BZ #15819]
	* NEWS: Update.
	* nis/nis_callback.c (internal_nis_do_callback): Call
	__poll_noeintr, drop TEMP_FAILURE_RETRY.
	* nscd/nscd_helper.c (wait_on_socket): Call __poll_noeintr,
	drop loop and gettimeofday timeout recomputation.
	* sunrpc/clnt_udp.c (clntudp_call): Call __poll_noeintr.
	* 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.
	* include/poll-noeintr.h: New header, included instead of
	sys/poll.h in all of the above.
---
 NEWS                   |    8 ++--
 include/poll-noeintr.h |   86 ++++++++++++++++++++++++++++++++++++++++++++++++
 nis/nis_callback.c     |    5 +--
 nscd/nscd_helper.c     |   26 +--------------
 sunrpc/clnt_udp.c      |    7 +---
 sunrpc/clnt_unix.c     |   24 +++++--------
 sunrpc/rtime.c         |    6 +--
 sunrpc/svc_run.c       |    6 +--
 sunrpc/svc_tcp.c       |   31 +++++++----------
 sunrpc/svc_unix.c      |   31 +++++++----------
 10 files changed, 133 insertions(+), 97 deletions(-)
 create mode 100644 include/poll-noeintr.h

Comments

Alexandre Oliva Dec. 9, 2014, 5:55 p.m. UTC | #1 
On Nov 14, 2014, Alexandre Oliva <aoliva@redhat.com> wrote:

> This revised patch fixes both of these issues, and it poisons __poll and
> poll at the end of include/poll-noeintr.h.

> Retested on x86_64-linux-gnu.  Ok to install?

Ping?
https://sourceware.org/ml/libc-alpha/2014-11/msg00334.html


> for  ChangeLog

> 	[BZ #15819]
> 	* NEWS: Update.
> 	* nis/nis_callback.c (internal_nis_do_callback): Call
> 	__poll_noeintr, drop TEMP_FAILURE_RETRY.
> 	* nscd/nscd_helper.c (wait_on_socket): Call __poll_noeintr,
> 	drop loop and gettimeofday timeout recomputation.
> 	* sunrpc/clnt_udp.c (clntudp_call): Call __poll_noeintr.
> 	* 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.
> 	* include/poll-noeintr.h: New header, included instead of
> 	sys/poll.h in all of the above.
Roland McGrath Dec. 9, 2014, 7:31 p.m. UTC | #2 
> > 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.
> 
> I figured names outside the reserved namespace could be used in our
> tests, and then they'd interfere.  I guess the reasoning is that, since
> we control the tests too, we can just fix any such conflicts in the
> tests, rigth?

How would they interfere?  We're not talking about external linkage names.

> This revised patch fixes both of these issues, and it poisons __poll and
> poll at the end of include/poll-noeintr.h.

We don't have any precedent for using '#pragma poison' in libc.  I'm not
particularly against it, but it is something we have not seen before.  It
needs comments explaining what it means and why it's there.  I think we
should also have a consensus policy on use of this feature, which would be
written up on the wiki.

> --- /dev/null
> +++ b/include/poll-noeintr.h
[...]
> +#ifndef _POLL_EINTR_H
> +#define _POLL_EINTR_H

Make the guard macro match the file name.

> +	  /* 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;

This could let TIMEOUT go negative, which has the wrong semantics.
You need to cap it at zero.

> +      tsend.tv_sec = tscur.tv_sec + timeout / 1000;
> +      tsend.tv_nsec = tscur.tv_nsec + timeout % 1000 * 1000000L + 500000;

We really should have some inlines/macros for these canonical time
conversion calculations, rather than repeating them.


Thanks,
Roland
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/poll-noeintr.h b/include/poll-noeintr.h
new file mode 100644
index 0000000..d94deb4
--- /dev/null
+++ b/include/poll-noeintr.h
@@ -0,0 +1,86 @@ 
+/* Wrapper for __poll that restarts on EINTR
+   Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _POLL_EINTR_H
+#define _POLL_EINTR_H
+
+#include <sys/poll.h>
+#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;
+}
+
+#pragma poison __poll
+#pragma poison poll
+
+#endif /* _POLL_EINTR_H */
diff --git a/nis/nis_callback.c b/nis/nis_callback.c
index e352733..b7492fb 100644
--- a/nis/nis_callback.c
+++ b/nis/nis_callback.c
@@ -26,7 +26,7 @@ 
 #include <string.h>
 #include <memory.h>
 #include <syslog.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
@@ -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..301be42 100644
--- a/nscd/nscd_helper.c
+++ b/nscd/nscd_helper.c
@@ -27,7 +27,7 @@ 
 #include <unistd.h>
 #include <stdint.h>
 #include <sys/mman.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/time.h>
@@ -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..38b11e1 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -37,7 +37,7 @@ 
 #include <rpc/rpc.h>
 #include <rpc/xdr.h>
 #include <rpc/clnt.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <netdb.h>
@@ -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..40eb269 100644
--- a/sunrpc/clnt_unix.c
+++ b/sunrpc/clnt_unix.c
@@ -51,7 +51,7 @@ 
 #include <libintl.h>
 #include <rpc/rpc.h>
 #include <sys/uio.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <sys/socket.h>
 #include <rpc/pmap_clnt.h>
 #include <wchar.h>
@@ -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..862493b 100644
--- a/sunrpc/rtime.c
+++ b/sunrpc/rtime.c
@@ -43,7 +43,7 @@ 
 #include <rpc/rpc.h>
 #include <rpc/clnt.h>
 #include <sys/types.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <sys/socket.h>
 #include <sys/time.h>
 #include <rpc/auth_des.h>
@@ -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..1f54d84 100644
--- a/sunrpc/svc_run.c
+++ b/sunrpc/svc_run.c
@@ -34,7 +34,7 @@ 
 #include <errno.h>
 #include <unistd.h>
 #include <libintl.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <rpc/rpc.h>
 
 /* This function can be used as a signal handler to terminate the
@@ -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..2ab98dc 100644
--- a/sunrpc/svc_tcp.c
+++ b/sunrpc/svc_tcp.c
@@ -58,7 +58,7 @@ 
 #include <libintl.h>
 #include <rpc/rpc.h>
 #include <sys/socket.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <errno.h>
 #include <stdlib.h>
 
@@ -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..cbc9891 100644
--- a/sunrpc/svc_unix.c
+++ b/sunrpc/svc_unix.c
@@ -59,7 +59,7 @@ 
 #include <rpc/svc.h>
 #include <sys/socket.h>
 #include <sys/uio.h>
-#include <sys/poll.h>
+#include <poll-noeintr.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <libintl.h>
@@ -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;