Re: [PATCH 02/13] nptl: Fix testcases for new pthread cancellation mechanism

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

Thanks for the review.

On 07-10-2015 17:59, Roland McGrath wrote:
>> diff --git a/nptl/tst-cancel2.c b/nptl/tst-cancel2.c
>> index 1a74992..3d34169 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 ();
> 
> "due to the"
> "may still be pending"
> 
> The same comment with the same errors is repeated in several tests.

I will fix that.

> 
>> --- 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
> 
> What's the rationale for this change?  If the test needs SIGCANCEL then it
> is relying on implementation internals.  If that is necessary for this
> test, then it is better to use an internal header than to duplicate part of
> its content.  If some other part of pthreadP.h interferes with this code,
> or if you just want to avoid unnecessary internals, then you can just
> #include <nptl-signals.h> instead.

The rationale is to not tied the test with implementation internals.  However
the sigpause test itself does not really require to block SIGCANCEL, we can
rather use another signal.  I will change to 'sigpause (sigmask (SIGINT))'
and remote both the inclusion of 'pthreadP.h' and the SIGCANCEL definitions.

> 
>> +  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 ();
> 
> You can create such a scenario in POSIX systems using mkfifo.
> 

Indeed (I just copies the block from previous tests), I will change that.