Message ID | 5435AD7A.60400@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Wed, 8 Oct 2014, Adhemerval Zanella wrote: > For testcases that are not tested in blocked case (pread64 for instance) > tst-cancel4/5 tests for its cancellation point as 'early exit'. Although > this is not ideal, 'tst-cancel-wrappers.sh' also does not offer more > coverage. Ideally and as pointed out in testcases itself, the best option > would be to create a way to actually block some cancellable entrypoint. > However, for some syscalls calls this is very trick (pread64 for instance > does not work on pipe-type descriptors). I still don't understand exactly what the effects are on test coverage. Could you provide a list of all the functions tested in tst-cancel-wrappers.sh, with, for each function, a statement either of the other testcase that tests cancellation of that function, or of why it is not possible to test cancellation of that function in a useful way?
On 08-10-2014 20:12, Joseph S. Myers wrote: > On Wed, 8 Oct 2014, Adhemerval Zanella wrote: > >> For testcases that are not tested in blocked case (pread64 for instance) >> tst-cancel4/5 tests for its cancellation point as 'early exit'. Although >> this is not ideal, 'tst-cancel-wrappers.sh' also does not offer more >> coverage. Ideally and as pointed out in testcases itself, the best option >> would be to create a way to actually block some cancellable entrypoint. >> However, for some syscalls calls this is very trick (pread64 for instance >> does not work on pipe-type descriptors). > I still don't understand exactly what the effects are on test coverage. > > Could you provide a list of all the functions tested in > tst-cancel-wrappers.sh, with, for each function, a statement either of the > other testcase that tests cancellation of that function, or of why it is > not possible to test cancellation of that function in a useful way? > I understand your hesitation about remove tst-cancel-wrappers.sh testcase, however currently it does not offer any more coverage than current other tests. Below is a chart I compiled with the function tst-cancel-wrappers.sh is intended to check: syscall | testcase | blocked case | early exit | Notes ----------------|----------------|--------------|------------|------ accept | tst-cancel4.c | yes | yes | close | tst-cancel4.c | no | yes | connect | tst-cancel4.c | no | yes | creat | tst-cancel4.c | no | yes | fcntl | tst-cancel16.c | no | yes | * fdatasync | tst-cancel4.c | no | yes | fsync | tst-cancel4.c | no | yes | msgrcv | tst-cancel4.c | yes | yes | msgsnd | tst-cancel4.c | no | yes | msync | tst-cancel4.c | no | yes | nanosleep | tst-cancel4.c | yes | yes | open/open64 | tst-cancel4.c | no | yes | pause | tst-cancel4.c | yes | yes | poll | tst-cancel4.c | yes | yes | pread/pread64 | tst-cancel4.c | no | yes | pselect | tst-cancel4.c | yes | yes | pwrite/pwrite64 | tst-cancel4.c | no | yes | read | tst-cancel4.c | yes | yes | readv | tst-cancel4.c | yes | yes | recv | tst-cancel4.c | yes | yes | recvfrom | tst-cancel4.c | yes | yes | recvmsg | tst-cancel4.c | yes | yes | select | tst-cancel4.c | yes | yes | send | tst-cancel4.c | yes | yes | sendmsg | tst-cancel4.c | no | yes | sendto | tst-cancel4.c | no | yes | sigpause | tst-cancel4.c | yes | yes | sigsuspend | tst-cancel4.c | yes | yes | sigwait | tst-cancel4.c | yes | yes | sigwaitinfo | tst-cancel4.c | yes | yes | tcdrain | tst-cancel4.c | no | yes | wait | tst-cancel4.c | yes | yes | waitid | tst-cancel4.c | yes | yes | waitpid | tst-cancel4.c | yes | yes | write | tst-cancel4.c | yes | yes | writev | tst-cancel4.c | yes | yes | * tested through lockf (F_LOCK) which calls fctnl (F_SETLK) The 'blocked case' case refers to a cancellation signal send while the syscall is blocked and the test checks cancellation is done properly. As the 'tst-cancel4.c' states, some syscalls are hard to put in a blocked case (pread/close for instance) to exercise all code patch. If you have suggestion of how to add blocked cases I can add them in a following patch. The 'early exit' checks if the cancellation entrypoint will check for pending cancellation signal and act accordingly. As you can for this case 'tst-cancel4.c' already cover alls the syscalls.
On Thu, 9 Oct 2014, Adhemerval Zanella wrote: > open/open64 | tst-cancel4.c | no | yes | > pread/pread64 | tst-cancel4.c | no | yes | > pwrite/pwrite64 | tst-cancel4.c | no | yes | I don't think pairing like that is legitimate. Each of those pairs is two separate functions, and only one of each pair is tested in tst-cancel4.c. The *64 functions need testing to at least the same extent.
On 09-10-2014 11:55, Joseph S. Myers wrote: > On Thu, 9 Oct 2014, Adhemerval Zanella wrote: > >> open/open64 | tst-cancel4.c | no | yes | >> pread/pread64 | tst-cancel4.c | no | yes | >> pwrite/pwrite64 | tst-cancel4.c | no | yes | > I don't think pairing like that is legitimate. Each of those pairs is two > separate functions, and only one of each pair is tested in tst-cancel4.c. > The *64 functions need testing to at least the same extent. > Right, will be suffice to add proper tests for open64/pread64/pwrite64 using __USE_LARGEFILE64 on tst-cancel4.c to cover such cases?
On Thu, 9 Oct 2014, Adhemerval Zanella wrote: > On 09-10-2014 11:55, Joseph S. Myers wrote: > > On Thu, 9 Oct 2014, Adhemerval Zanella wrote: > > > >> open/open64 | tst-cancel4.c | no | yes | > >> pread/pread64 | tst-cancel4.c | no | yes | > >> pwrite/pwrite64 | tst-cancel4.c | no | yes | > > I don't think pairing like that is legitimate. Each of those pairs is two > > separate functions, and only one of each pair is tested in tst-cancel4.c. > > The *64 functions need testing to at least the same extent. > > > Right, will be suffice to add proper tests for open64/pread64/pwrite64 using > __USE_LARGEFILE64 on tst-cancel4.c to cover such cases? You shouldn't need __USE_LARGEFILE64 (_GNU_SOURCE is used by default, which implies __USE_LARGEFILE64). Yes, I think adding tests of those functions similar to the existing ones of open/pread/pwrite should suffice.
diff --git a/nptl/Makefile b/nptl/Makefile index 157fe62..cb4bdd7 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -377,8 +377,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 @@ -572,7 +571,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 \ @@ -587,18 +586,6 @@ generated += banner.h LDFLAGS-pthread.so += -e __nptl_main 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 e41ed51..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-2014 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","",$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-cancel2.c b/nptl/tst-cancel2.c index 2d834de..cb807c4 100644 --- a/nptl/tst-cancel2.c +++ b/nptl/tst-cancel2.c @@ -33,6 +33,9 @@ tf (void *arg) char buf[100000]; while (write (fd[1], buf, sizeof (buf)) > 0); + /* The write can return a value higher than 0 (meaning partial write) + due the SIGCANCEL, but the thread may still pending cancellation. */ + pthread_testcancel (); return (void *) 42l; } diff --git a/nptl/tst-cancel20.c b/nptl/tst-cancel20.c index 703e558..0d54b21 100644 --- a/nptl/tst-cancel20.c +++ b/nptl/tst-cancel20.c @@ -49,6 +49,9 @@ sh_body (void) puts ("read succeeded"); exit (1); } + /* The read can return a value higher than 0 (meaning partial reads) + due the SIGCANCEL, but the thread may still pending cancellation. */ + pthread_testcancel (); pthread_cleanup_pop (0); } @@ -84,7 +87,8 @@ tf_body (void) puts ("read succeeded"); exit (1); } - + /* Check for partial read. */ + pthread_testcancel (); read (fd[0], &c, 1); pthread_cleanup_pop (0); diff --git a/nptl/tst-cancel21.c b/nptl/tst-cancel21.c index ddcea90..35a56dc 100644 --- a/nptl/tst-cancel21.c +++ b/nptl/tst-cancel21.c @@ -50,6 +50,9 @@ sh_body (void) puts ("read succeeded"); exit (1); } + /* The write can return a value higher than 0 (meaning partial write) + due the SIGCANCEL, but the thread may still pending cancellation. */ + pthread_testcancel (); pthread_cleanup_pop (0); } @@ -85,6 +88,8 @@ tf_body (void) puts ("read succeeded"); exit (1); } + /* Check partial read. */ + pthread_testcancel (); read (fd[0], &c, 1); diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c index 93080b2..fd4ea42 100644 --- a/nptl/tst-cancel4.c +++ b/nptl/tst-cancel4.c @@ -38,8 +38,10 @@ #include <sys/un.h> #include <sys/wait.h> -#include "pthreadP.h" - +/* The signal used for asynchronous cancelation. */ +#ifndef SIGCANCEL +# define SIGCANCEL __SIGRTMIN +#endif /* Since STREAMS are not supported in the standard Linux kernel and there we don't advertise STREAMS as supported is no need to test @@ -247,6 +249,9 @@ 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 the SIGCANCEL, but the thread may still pending cancellation. */ + pthread_testcancel (); pthread_cleanup_pop (0); @@ -1139,6 +1144,8 @@ tf_send (void *arg) char mem[700000]; send (tempfd2, mem, arg == NULL ? sizeof (mem) : 1, 0); + /* Check for partial send. */ + pthread_testcancel (); pthread_cleanup_pop (0);