Patchwork nptl: Add more coverage in tst-cancel4

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date June 14, 2016, 3:42 p.m.
Message ID <1465918948-13371-1-git-send-email-adhemerval.zanella@linaro.org>
Download mbox | patch
Permalink /patch/13072/
State New
Headers show

Comments

Adhemerval Zanella Netto - June 14, 2016, 3:42 p.m.
This patch adds early cancel test for open syscall through a FIFO
(thus makign subsequent call to open block until the other end is
also opened).

It also cleanup the sigpause tests by using sigpause along with
SIGINT instead of __xpg_sigpause and SIGCANCEL.  Since the idea
is just to test the cancellation handling there is no need to expose
internal glibc implementation details to the test through pthreadP.h
inclusion.

Tested x86_64.

	* nptl/tst-cancel4-common.c (do_test): Add temporary fifo creation.
	* nptl/tst-cancel4-common.h (fifoname): New variable.
	(fifofd): Likewise.
	(cl_fifo): New function.
	* nptl/tst-cancel4.c (tf_sigpause): Replace SIGCANCEL usage by
	SIGINT.
	(tf_open): Add early cancel test.
---
 ChangeLog                 | 10 ++++++++++
 nptl/tst-cancel4-common.c |  6 ++++++
 nptl/tst-cancel4-common.h | 16 ++++++++++++++++
 nptl/tst-cancel4.c        | 40 ++++++++++++++++++++--------------------
 4 files changed, 52 insertions(+), 20 deletions(-)
Adhemerval Zanella Netto - July 1, 2016, 6:33 p.m.
If no one opposes, I will commit it soon.

On 14/06/2016 12:42, Adhemerval Zanella wrote:
> This patch adds early cancel test for open syscall through a FIFO
> (thus makign subsequent call to open block until the other end is
> also opened).
> 
> It also cleanup the sigpause tests by using sigpause along with
> SIGINT instead of __xpg_sigpause and SIGCANCEL.  Since the idea
> is just to test the cancellation handling there is no need to expose
> internal glibc implementation details to the test through pthreadP.h
> inclusion.
> 
> Tested x86_64.
> 
> 	* nptl/tst-cancel4-common.c (do_test): Add temporary fifo creation.
> 	* nptl/tst-cancel4-common.h (fifoname): New variable.
> 	(fifofd): Likewise.
> 	(cl_fifo): New function.
> 	* nptl/tst-cancel4.c (tf_sigpause): Replace SIGCANCEL usage by
> 	SIGINT.
> 	(tf_open): Add early cancel test.
> ---
>  ChangeLog                 | 10 ++++++++++
>  nptl/tst-cancel4-common.c |  6 ++++++
>  nptl/tst-cancel4-common.h | 16 ++++++++++++++++
>  nptl/tst-cancel4.c        | 40 ++++++++++++++++++++--------------------
>  4 files changed, 52 insertions(+), 20 deletions(-)
> 
> diff --git a/nptl/tst-cancel4-common.c b/nptl/tst-cancel4-common.c
> index f235d53..cab99c4 100644
> --- a/nptl/tst-cancel4-common.c
> +++ b/nptl/tst-cancel4-common.c
> @@ -44,6 +44,12 @@ do_test (void)
>      }
>    setsockopt (fds[1], SOL_SOCKET, SO_SNDBUF, &val, sizeof(val));
>  
> +  if (mktemp (fifoname) == NULL)
> +    {
> +      printf ("%s: cannot generate temp file name\n", __FUNCTION__);
> +      exit (1);
> +    }
> +
>    int result = 0;
>    size_t cnt;
>    for (cnt = 0; cnt < ntest_tf; ++cnt)
> diff --git a/nptl/tst-cancel4-common.h b/nptl/tst-cancel4-common.h
> index e1683c4..06ed0dc 100644
> --- a/nptl/tst-cancel4-common.h
> +++ b/nptl/tst-cancel4-common.h
> @@ -67,6 +67,22 @@ cl (void *arg)
>    ++cl_called;
>  }
>  
> +/* Named pipe used to check for blocking open.  It should be closed
> +   after the cancellation handling.  */
> +static char fifoname[] = "/tmp/tst-cancel4-fifo-XXXXXX";
> +static int fifofd;
> +
> +static void
> +__attribute__ ((used))
> +cl_fifo (void *arg)
> +{
> +  ++cl_called;
> +
> +  unlink (fifoname);
> +  close (fifofd);
> +  fifofd = -1;
> +}
> +
>  struct cancel_tests
>  {
>    const char *name;
> diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c
> index 4221af4..c1fac1b 100644
> --- a/nptl/tst-cancel4.c
> +++ b/nptl/tst-cancel4.c
> @@ -36,8 +36,6 @@
>  #include <sys/poll.h>
>  #include <sys/wait.h>
>  
> -#include "pthreadP.h"
> -
>  
>  /* Since STREAMS are not supported in the standard Linux kernel and
>     there we don't advertise STREAMS as supported is no need to test
> @@ -67,6 +65,7 @@
>  
>  #include "tst-cancel4-common.h"
>  
> +
>  #ifndef IPC_ADDVAL
>  # define IPC_ADDVAL 0
>  #endif
> @@ -734,13 +733,7 @@ tf_sigpause (void *arg)
>  
>    pthread_cleanup_push (cl, NULL);
>  
> -#ifdef SIGCANCEL
> -  /* Just for fun block the cancellation signal.  We need to use
> -     __xpg_sigpause since otherwise we will get the BSD version.  */
> -  __xpg_sigpause (SIGCANCEL);
> -#else
> -  pause ();
> -#endif
> +  sigpause (sigmask (SIGINT));
>  
>    pthread_cleanup_pop (0);
>  
> @@ -1348,27 +1341,34 @@ static void *
>  tf_open (void *arg)
>  {
>    if (arg == NULL)
> -    // XXX If somebody can provide a portable test case in which open()
> -    // blocks we can enable this test to run in both rounds.
> -    abort ();
> -
> -  int r = pthread_barrier_wait (&b2);
> -  if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>      {
> -      printf ("%s: barrier_wait failed\n", __FUNCTION__);
> -      exit (1);
> +      fifofd = mkfifo (fifoname, S_IWUSR | S_IRUSR);
> +      if (fifofd == -1)
> +	{
> +	  printf ("%s: mkfifo failed\n", __FUNCTION__);
> +	  exit (1);
> +	}
> +    }
> +  else
> +    {
> +      int r = pthread_barrier_wait (&b2);
> +      if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
> +	{
> +	  printf ("%s: barrier_wait failed\n", __FUNCTION__);
> +	  exit (1);
> +	}
>      }
>  
> -  r = pthread_barrier_wait (&b2);
> +  int r = pthread_barrier_wait (&b2);
>    if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>      {
>        printf ("%s: 2nd barrier_wait failed\n", __FUNCTION__);
>        exit (1);
>      }
>  
> -  pthread_cleanup_push (cl, NULL);
> +  pthread_cleanup_push (cl_fifo, NULL);
>  
> -  open ("Makefile", O_RDONLY);
> +  open (arg ? "Makefile" : fifoname, O_RDONLY);
>  
>    pthread_cleanup_pop (0);
>  
>
Mike Frysinger - July 4, 2016, 2:27 a.m.
On 14 Jun 2016 12:42, Adhemerval Zanella wrote:
> +      printf ("%s: cannot generate temp file name\n", __FUNCTION__);

__func__ is the portable name isn't it ?

ignoring that, should this (all the other error messages you've added)
end with %m ?
-mike
Adhemerval Zanella Netto - July 4, 2016, 12:24 p.m.
On 03/07/2016 23:27, Mike Frysinger wrote:
> On 14 Jun 2016 12:42, Adhemerval Zanella wrote:
>> +      printf ("%s: cannot generate temp file name\n", __FUNCTION__);
> 
> __func__ is the portable name isn't it ?
> 
> ignoring that, should this (all the other error messages you've added)
> end with %m ?

I am not sure about '__func__', but I think %m seems reasonable. 

> -mike
>
Mike Frysinger - July 5, 2016, 1:13 a.m.
On 04 Jul 2016 09:24, Adhemerval Zanella wrote:
> On 03/07/2016 23:27, Mike Frysinger wrote:
> > On 14 Jun 2016 12:42, Adhemerval Zanella wrote:
> >> +      printf ("%s: cannot generate temp file name\n", __FUNCTION__);
> > 
> > __func__ is the portable name isn't it ?
> > 
> > ignoring that, should this (all the other error messages you've added)
> > end with %m ?
> 
> I am not sure about '__func__', but I think %m seems reasonable. 

__FUNCTION__ is a gcc extension.  the standards added __func__.
-mike

Patch

diff --git a/nptl/tst-cancel4-common.c b/nptl/tst-cancel4-common.c
index f235d53..cab99c4 100644
--- a/nptl/tst-cancel4-common.c
+++ b/nptl/tst-cancel4-common.c
@@ -44,6 +44,12 @@  do_test (void)
     }
   setsockopt (fds[1], SOL_SOCKET, SO_SNDBUF, &val, sizeof(val));
 
+  if (mktemp (fifoname) == NULL)
+    {
+      printf ("%s: cannot generate temp file name\n", __FUNCTION__);
+      exit (1);
+    }
+
   int result = 0;
   size_t cnt;
   for (cnt = 0; cnt < ntest_tf; ++cnt)
diff --git a/nptl/tst-cancel4-common.h b/nptl/tst-cancel4-common.h
index e1683c4..06ed0dc 100644
--- a/nptl/tst-cancel4-common.h
+++ b/nptl/tst-cancel4-common.h
@@ -67,6 +67,22 @@  cl (void *arg)
   ++cl_called;
 }
 
+/* Named pipe used to check for blocking open.  It should be closed
+   after the cancellation handling.  */
+static char fifoname[] = "/tmp/tst-cancel4-fifo-XXXXXX";
+static int fifofd;
+
+static void
+__attribute__ ((used))
+cl_fifo (void *arg)
+{
+  ++cl_called;
+
+  unlink (fifoname);
+  close (fifofd);
+  fifofd = -1;
+}
+
 struct cancel_tests
 {
   const char *name;
diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c
index 4221af4..c1fac1b 100644
--- a/nptl/tst-cancel4.c
+++ b/nptl/tst-cancel4.c
@@ -36,8 +36,6 @@ 
 #include <sys/poll.h>
 #include <sys/wait.h>
 
-#include "pthreadP.h"
-
 
 /* Since STREAMS are not supported in the standard Linux kernel and
    there we don't advertise STREAMS as supported is no need to test
@@ -67,6 +65,7 @@ 
 
 #include "tst-cancel4-common.h"
 
+
 #ifndef IPC_ADDVAL
 # define IPC_ADDVAL 0
 #endif
@@ -734,13 +733,7 @@  tf_sigpause (void *arg)
 
   pthread_cleanup_push (cl, NULL);
 
-#ifdef SIGCANCEL
-  /* Just for fun block the cancellation signal.  We need to use
-     __xpg_sigpause since otherwise we will get the BSD version.  */
-  __xpg_sigpause (SIGCANCEL);
-#else
-  pause ();
-#endif
+  sigpause (sigmask (SIGINT));
 
   pthread_cleanup_pop (0);
 
@@ -1348,27 +1341,34 @@  static void *
 tf_open (void *arg)
 {
   if (arg == NULL)
-    // XXX If somebody can provide a portable test case in which open()
-    // blocks we can enable this test to run in both rounds.
-    abort ();
-
-  int r = pthread_barrier_wait (&b2);
-  if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      printf ("%s: barrier_wait failed\n", __FUNCTION__);
-      exit (1);
+      fifofd = mkfifo (fifoname, S_IWUSR | S_IRUSR);
+      if (fifofd == -1)
+	{
+	  printf ("%s: mkfifo failed\n", __FUNCTION__);
+	  exit (1);
+	}
+    }
+  else
+    {
+      int r = pthread_barrier_wait (&b2);
+      if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
+	{
+	  printf ("%s: barrier_wait failed\n", __FUNCTION__);
+	  exit (1);
+	}
     }
 
-  r = pthread_barrier_wait (&b2);
+  int r = pthread_barrier_wait (&b2);
   if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
     {
       printf ("%s: 2nd barrier_wait failed\n", __FUNCTION__);
       exit (1);
     }
 
-  pthread_cleanup_push (cl, NULL);
+  pthread_cleanup_push (cl_fifo, NULL);
 
-  open ("Makefile", O_RDONLY);
+  open (arg ? "Makefile" : fifoname, O_RDONLY);
 
   pthread_cleanup_pop (0);