Re: [PATCH v2 10/19] nptl: Use tidlock when accessing TID on pthread_getaffinity_np

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 26/08/2021 11:24, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Checked on x86_64-linux-gnu.
>> ---
>>  nptl/pthread_getaffinity.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/nptl/pthread_getaffinity.c b/nptl/pthread_getaffinity.c
>> index 18261ddae0..5268d86e6e 100644
>> --- a/nptl/pthread_getaffinity.c
>> +++ b/nptl/pthread_getaffinity.c
>> @@ -29,12 +29,24 @@
>>  int
>>  __pthread_getaffinity_np (pthread_t th, size_t cpusetsize, cpu_set_t *cpuset)
>>  {
>> -  const struct pthread *pd = (const struct pthread *) th;
>> +  struct pthread *pd = (struct pthread *) th;
>>  
>> -  int res = INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
>> -				   MIN (INT_MAX, cpusetsize), cpuset);
>> -  if (INTERNAL_SYSCALL_ERROR_P (res))
>> -    return INTERNAL_SYSCALL_ERRNO (res);
>> +  /* Block all signal, since the lock is recursive and used on pthread_cancel
>> +     (which should be async-signal-safe).  */
> 
> The pthread_cancel reference looks like a cut-and-paste-bug.

It is, but I think it is applicable. Maybe:

  /* Block all signal, since the lock is not recursive and used on                                                                                             
     async-signal-safe functions. */   

> 
>> +  sigset_t oldmask;
>> +  __libc_signal_block_all (&oldmask);
>> +  lll_lock (pd->tidlock, LLL_PRIVATE);
>> +
>> +  int res = pd->tid == 0
>> +	    ? -ESRCH
>> +	    : INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
>> +				     MIN (INT_MAX, cpusetsize), cpuset);
>> +
>> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
>> +  __libc_signal_restore_set (&oldmask);
>> +
>> +  if (res < 0)
>> +    return -res;
> 
> ESRCH doesn't look like the right error code here.  Should we return an
> affinity mask without any bits set?

Why not? Returning anything but an error does not improve things here
since the information won't make much sense.  Also it follows what other
symbols is already doing (such as pthread_cancel()).

> 
> Thanks,
> Florian
>