public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* BZ#15819: introduce internal function to ease poll retry with timeout
@ 2014-11-13 21:54 Alexandre Oliva
  2014-11-13 22:18 ` Roland McGrath
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Oliva @ 2014-11-13 21:54 UTC (permalink / raw)
  To: libc-alpha

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(-)

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.
 \f
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;


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: BZ#15819: introduce internal function to ease poll retry with timeout
  2014-11-13 21:54 BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
@ 2014-11-13 22:18 ` Roland McGrath
  2014-11-14  0:38   ` feedback feedback (was: BZ#15819, BZ#15722) Alexandre Oliva
  2014-11-14  2:18   ` BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
  0 siblings, 2 replies; 6+ messages in thread
From: Roland McGrath @ 2014-11-13 22:18 UTC (permalink / raw)
  To: Alexandre Oliva; +Cc: libc-alpha

> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* feedback feedback (was: BZ#15819, BZ#15722)
  2014-11-13 22:18 ` Roland McGrath
@ 2014-11-14  0:38   ` Alexandre Oliva
  2014-11-14  2:18   ` BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
  1 sibling, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2014-11-14  0:38 UTC (permalink / raw)
  To: Roland McGrath; +Cc: libc-alpha

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, ;-)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: BZ#15819: introduce internal function to ease poll retry with timeout
  2014-11-13 22:18 ` Roland McGrath
  2014-11-14  0:38   ` feedback feedback (was: BZ#15819, BZ#15722) Alexandre Oliva
@ 2014-11-14  2:18   ` Alexandre Oliva
  2014-12-09 17:55     ` Alexandre Oliva
  2014-12-09 19:31     ` Roland McGrath
  1 sibling, 2 replies; 6+ messages in thread
From: Alexandre Oliva @ 2014-11-14  2:18 UTC (permalink / raw)
  To: Roland McGrath; +Cc: libc-alpha

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

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.
 \f
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;


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: BZ#15819: introduce internal function to ease poll retry with timeout
  2014-11-14  2:18   ` BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
@ 2014-12-09 17:55     ` Alexandre Oliva
  2014-12-09 19:31     ` Roland McGrath
  1 sibling, 0 replies; 6+ messages in thread
From: Alexandre Oliva @ 2014-12-09 17:55 UTC (permalink / raw)
  To: Roland McGrath; +Cc: libc-alpha

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.


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: BZ#15819: introduce internal function to ease poll retry with timeout
  2014-11-14  2:18   ` BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
  2014-12-09 17:55     ` Alexandre Oliva
@ 2014-12-09 19:31     ` Roland McGrath
  1 sibling, 0 replies; 6+ messages in thread
From: Roland McGrath @ 2014-12-09 19:31 UTC (permalink / raw)
  To: Alexandre Oliva; +Cc: libc-alpha

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-09 19:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-13 21:54 BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
2014-11-13 22:18 ` Roland McGrath
2014-11-14  0:38   ` feedback feedback (was: BZ#15819, BZ#15722) Alexandre Oliva
2014-11-14  2:18   ` BZ#15819: introduce internal function to ease poll retry with timeout Alexandre Oliva
2014-12-09 17:55     ` Alexandre Oliva
2014-12-09 19:31     ` Roland McGrath

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).