Re: [PATCH v3] nptl: Fix testcases for new pthread cancellation mechanism

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Ping (x2).

On 19-10-2015 16:09, Adhemerval Zanella wrote:
> Ping.
> 
> On 14-10-2015 16:32, Adhemerval Zanella wrote:
>> Changed from previous version:
>>
>> - Removed tst-cancel{2,20,21} changes since they are not required.
>>
>> With upcoming fix for BZ#12683, pthread cancellation does not act for:
>>
>> 1. If syscall is blocked but with some side effects already having taken
>>    place (e.g. a partial read or write)
>> 2. After the syscall has returned.
>>
>> It is because program need to act on such cases (for instance, to avoid
>> leak of allocated resources our handling partial read/write).
>>
>> This patches fixes the NPTL testcase that assumes the old behavior and
>> also remove the tst-cancel-wrappers.sh test (which checks for symbols
>> that does not exist anymore).
>>
>> It also add two more testcase to check both the returned and the errno
>> value on case of a cancelled syscall.
>>
>> Tested on i686, x86_64, x32, powerpc64le, and aarch64.
>>
>> 	* nptl/Makefile [$(run-built-tests) = yes] (tests-special): Remove
>> 	tst-cancel-wrappers.sh.
>> 	* nptl/tst-cancel-wrappers.sh: Remove file.
>> 	* nptl/tst-cancel4.c (tf_Write): Likewise.
>> 	(tf_send): Likewise.
>> 	(cl_fifo): New function: pipe handling for open/open64.
>> 	(tf_sigpause): Use sigpause instead of __xpg_sigpausea and use
>> 	SIGINT instead of SIGCANCEL.
>> 	(tf_open): Use mkfifo to check for early cancel.
>> 	(tf_open64): New test: check for open64 cancellable syscall.
>> 	(tf_pread64): New test: check for pread64 cancellable syscall.
>> 	(tf_pwrite64): New test: check for pwrite64 cancellable syscall.
>> ---
>>  ChangeLog                   |  15 ++++
>>  nptl/Makefile               |  17 +---
>>  nptl/tst-cancel-wrappers.sh |  92 ----------------------
>>  nptl/tst-cancel4.c          | 185 ++++++++++++++++++++++++++++++++++++++++----
>>  4 files changed, 185 insertions(+), 124 deletions(-)
>>  delete mode 100644 nptl/tst-cancel-wrappers.sh
>>
>> diff --git a/nptl/Makefile b/nptl/Makefile
>> index 311b1a7..b7ff3f1 100644
>> --- a/nptl/Makefile
>> +++ b/nptl/Makefile
>> @@ -398,8 +398,7 @@ tests-reverse += tst-cancel5 tst-cancel23 tst-vfork1x tst-vfork2x
>>  ifeq ($(run-built-tests),yes)
>>  tests-special += $(objpfx)tst-stack3-mem.out $(objpfx)tst-oddstacklimit.out
>>  ifeq ($(build-shared),yes)
>> -tests-special += $(objpfx)tst-tls6.out $(objpfx)tst-cleanup0-cmp.out \
>> -		 $(objpfx)tst-cancel-wrappers.out
>> +tests-special += $(objpfx)tst-tls6.out $(objpfx)tst-cleanup0-cmp.out
>>  endif
>>  endif
>>  
>> @@ -615,7 +614,7 @@ $(objpfx)$(multidir)/crtn.o: $(objpfx)crtn.o $(objpfx)$(multidir)/
>>  endif
>>  
>>  generated += libpthread_nonshared.a \
>> -	     multidir.mk tst-atfork2.mtrace tst-cancel-wrappers.out \
>> +	     multidir.mk tst-atfork2.mtrace \
>>  	     tst-tls6.out
>>  
>>  generated += $(objpfx)tst-atfork2.mtrace \
>> @@ -632,18 +631,6 @@ LDFLAGS-pthread.so += -e __nptl_main
>>  $(objpfx)pt-interp.os: $(common-objpfx)runtime-linker.h
>>  endif
>>  
>> -ifeq ($(run-built-tests),yes)
>> -ifeq (yes,$(build-shared))
>> -$(objpfx)tst-cancel-wrappers.out: tst-cancel-wrappers.sh
>> -	$(SHELL) $< '$(NM)' \
>> -		    $(common-objpfx)libc_pic.a \
>> -		    $(common-objpfx)libc.a \
>> -		    $(objpfx)libpthread_pic.a \
>> -		    $(objpfx)libpthread.a > $@; \
>> -	$(evaluate-test)
>> -endif
>> -endif
>> -
>>  tst-exec4-ARGS = $(host-test-program-cmd)
>>  
>>  $(objpfx)tst-execstack: $(libdl)
>> diff --git a/nptl/tst-cancel-wrappers.sh b/nptl/tst-cancel-wrappers.sh
>> deleted file mode 100644
>> index d492a54..0000000
>> --- a/nptl/tst-cancel-wrappers.sh
>> +++ /dev/null
>> @@ -1,92 +0,0 @@
>> -#! /bin/sh
>> -# Test whether all cancelable functions are cancelable.
>> -# Copyright (C) 2002-2015 Free Software Foundation, Inc.
>> -# This file is part of the GNU C Library.
>> -# Contributed by Jakub Jelinek <jakub@redhat.com>, 2002.
>> -
>> -# 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/>.
>> -
>> -NM="$1"; shift
>> -while [ $# -gt 0 ]; do
>> -  ( $NM -P $1; echo 'end[end]:' ) | gawk ' BEGIN {
>> -C["accept"]=1
>> -C["close"]=1
>> -C["connect"]=1
>> -C["creat"]=1
>> -C["fcntl"]=1
>> -C["fdatasync"]=1
>> -C["fsync"]=1
>> -C["msgrcv"]=1
>> -C["msgsnd"]=1
>> -C["msync"]=1
>> -C["nanosleep"]=1
>> -C["open"]=1
>> -C["open64"]=1
>> -C["pause"]=1
>> -C["poll"]=1
>> -C["pread"]=1
>> -C["pread64"]=1
>> -C["pselect"]=1
>> -C["pwrite"]=1
>> -C["pwrite64"]=1
>> -C["read"]=1
>> -C["readv"]=1
>> -C["recv"]=1
>> -C["recvfrom"]=1
>> -C["recvmsg"]=1
>> -C["select"]=1
>> -C["send"]=1
>> -C["sendmsg"]=1
>> -C["sendto"]=1
>> -C["sigpause"]=1
>> -C["sigsuspend"]=1
>> -C["sigwait"]=1
>> -C["sigwaitinfo"]=1
>> -C["tcdrain"]=1
>> -C["wait"]=1
>> -C["waitid"]=1
>> -C["waitpid"]=1
>> -C["write"]=1
>> -C["writev"]=1
>> -C["__xpg_sigpause"]=1
>> -}
>> -/:$/ {
>> -  if (seen)
>> -    {
>> -      if (!seen_enable || !seen_disable)
>> -	{
>> -	  printf "in '$1'(%s) %s'\''s cancellation missing\n", object, seen
>> -	  ret = 1
>> -	}
>> -    }
>> -  seen=""
>> -  seen_enable=""
>> -  seen_disable=""
>> -  object=gensub(/^.*\[(.*)\]:$/, "\\1", 1, $0)
>> -  next
>> -}
>> -{
>> -  if (C[$1] && $2 ~ /^[TW]$/)
>> -    seen=$1
>> -  else if ($1 ~ /^([.]|)__(libc|pthread)_enable_asynccancel$/ && $2 == "U")
>> -    seen_enable=1
>> -  else if ($1 ~ /^([.]|)__(libc|pthread)_disable_asynccancel$/ && $2 == "U")
>> -    seen_disable=1
>> -}
>> -END {
>> -  exit ret
>> -}' || exit
>> -  shift
>> -done
>> diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c
>> index e50afd7..d63ddfd 100644
>> --- a/nptl/tst-cancel4.c
>> +++ b/nptl/tst-cancel4.c
>> @@ -38,8 +38,6 @@
>>  #include <sys/un.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
>> @@ -117,7 +115,20 @@ 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
>> +cl_fifo (void *arg)
>> +{
>> +  ++cl_called;
>> +
>> +  unlink (fifoname);
>> +  close (fifofd);
>> +  fifofd = -1;
>> +}
>>  
>>  static void *
>>  tf_read  (void *arg)
>> @@ -247,6 +258,10 @@ tf_write  (void *arg)
>>    char buf[WRITE_BUFFER_SIZE];
>>    memset (buf, '\0', sizeof (buf));
>>    s = write (fd, buf, sizeof (buf));
>> +  /* The write can return a value higher than 0 (meaning partial write)
>> +     due to the SIGCANCEL, but the thread may still be pending
>> +     cancellation.  */
>> +  pthread_testcancel ();
>>  
>>    pthread_cleanup_pop (0);
>>  
>> @@ -781,13 +796,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);
>>  
>> @@ -1143,6 +1152,10 @@ tf_send (void *arg)
>>    char mem[700000];
>>  
>>    send (tempfd2, mem, arg == NULL ? sizeof (mem) : 1, 0);
>> +  /* Thez send can return a value higher than 0 (meaning partial send)
>> +     due to the SIGCANCEL, but the thread may still be pending
>> +     cancellation.  */
>> +  pthread_testcancel ();
>>  
>>    pthread_cleanup_pop (0);
>>  
>> @@ -1396,9 +1409,23 @@ 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 ();
>> +    {
>> +      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);
>> +	}
>> +    }
>>  
>>    int r = pthread_barrier_wait (&b2);
>>    if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>> @@ -1407,16 +1434,49 @@ tf_open (void *arg)
>>        exit (1);
>>      }
>>  
>> -  r = pthread_barrier_wait (&b2);
>> +  pthread_cleanup_push (cl_fifo, NULL);
>> +
>> +  open (arg ? "Makefile" : fifoname, O_RDONLY);
>> +
>> +  pthread_cleanup_pop (0);
>> +
>> +  printf ("%s: open returned\n", __FUNCTION__);
>> +
>> +  exit (1);
>> +}
>> +
>> +static void *
>> +tf_open64 (void *arg)
>> +{
>> +  if (arg == NULL)
>> +    {
>> +      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);
>> +	}
>> +    }
>> +
>> +  int r = pthread_barrier_wait (&b2);
>>    if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>>      {
>> -      printf ("%s: 2nd barrier_wait failed\n", __FUNCTION__);
>> +      printf ("%s: barrier_wait failed\n", __FUNCTION__);
>>        exit (1);
>>      }
>>  
>> -  pthread_cleanup_push (cl, NULL);
>> +  pthread_cleanup_push (cl_fifo, NULL);
>>  
>> -  open ("Makefile", O_RDONLY);
>> +  open64 (arg ? "Makefile" : fifoname, O_RDONLY);
>>  
>>    pthread_cleanup_pop (0);
>>  
>> @@ -1510,6 +1570,46 @@ tf_pread (void *arg)
>>    exit (1);
>>  }
>>  
>> +static void *
>> +tf_pread64 (void *arg)
>> +{
>> +  if (arg == NULL)
>> +    // XXX If somebody can provide a portable test case in which pread()
>> +    // blocks we can enable this test to run in both rounds.
>> +    abort ();
>> +
>> +  tempfd = open64 ("Makefile", O_RDONLY);
>> +  if (tempfd == -1)
>> +    {
>> +      printf ("%s: cannot open64 Makefile\n", __FUNCTION__);
>> +      exit (1);
>> +    }
>> +
>> +  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);
>> +  if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>> +    {
>> +      printf ("%s: 2nd barrier_wait failed\n", __FUNCTION__);
>> +      exit (1);
>> +    }
>> +
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  char mem[10];
>> +  pread64 (tempfd, mem, sizeof (mem), 0);
>> +
>> +  pthread_cleanup_pop (0);
>> +
>> +  printf ("%s: pread64 returned\n", __FUNCTION__);
>> +
>> +  exit (1);
>> +}
>>  
>>  static void *
>>  tf_pwrite (void *arg)
>> @@ -1554,6 +1654,48 @@ tf_pwrite (void *arg)
>>    exit (1);
>>  }
>>  
>> +static void *
>> +tf_pwrite64 (void *arg)
>> +{
>> +  if (arg == NULL)
>> +    // XXX If somebody can provide a portable test case in which pwrite()
>> +    // blocks we can enable this test to run in both rounds.
>> +    abort ();
>> +
>> +  char fname[] = "/tmp/tst-cancel4-fd-XXXXXX";
>> +  tempfd = mkstemp (fname);
>> +  if (tempfd == -1)
>> +    {
>> +      printf ("%s: mkstemp failed\n", __FUNCTION__);
>> +      exit (1);
>> +    }
>> +  unlink (fname);
>> +
>> +  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);
>> +  if (r != 0 && r != PTHREAD_BARRIER_SERIAL_THREAD)
>> +    {
>> +      printf ("%s: 2nd barrier_wait failed\n", __FUNCTION__);
>> +      exit (1);
>> +    }
>> +
>> +  pthread_cleanup_push (cl, NULL);
>> +
>> +  char mem[10];
>> +  pwrite64 (tempfd, mem, sizeof (mem), 0);
>> +
>> +  pthread_cleanup_pop (0);
>> +
>> +  printf ("%s: pwrite64 returned\n", __FUNCTION__);
>> +
>> +  exit (1);
>> +}
>>  
>>  static void *
>>  tf_fsync (void *arg)
>> @@ -2140,10 +2282,13 @@ static struct
>>    ADD_TEST (recv, 2, 0),
>>    ADD_TEST (recvfrom, 2, 0),
>>    ADD_TEST (recvmsg, 2, 0),
>> -  ADD_TEST (open, 2, 1),
>> +  ADD_TEST (open, 2, 0),
>> +  ADD_TEST (open64, 2, 0),
>>    ADD_TEST (close, 2, 1),
>>    ADD_TEST (pread, 2, 1),
>> +  ADD_TEST (pread64, 2, 1),
>>    ADD_TEST (pwrite, 2, 1),
>> +  ADD_TEST (pwrite64, 2, 1),
>>    ADD_TEST (fsync, 2, 1),
>>    ADD_TEST (fdatasync, 2, 1),
>>    ADD_TEST (msync, 2, 1),
>> @@ -2185,6 +2330,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)
>>