Re: [PATCH] nptl: Provide a way to block all signals for the timer helper thread

[Carlos O'Donell <carlos@redhat.com>]

On 5/12/20 3:17 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> +/* Exactly like pthread_create if NEW_SIGMASK == NULL.  Otherwise, do
>>> +   not use the current signal mask for the new thread, but set it to
>>> +   *NEW_SIGMASK instead (without unblocking internal signals).  */


Suggest:

/* Exactly like pthread_create if NEW_SIGMASK is NULL.
   Create the new thread using the thread descriptor at NEWTHREAD,
   and the thread attributes from *ATTR, executing provided START_ROUTINE
   with a single void argument ARG.  The last argument is not present in
   the public API for pthread_create, but is part of the internal glibc API.
   The last argument is the signal mask to be restored in the new thread,
   and is normally NULL, in which case the parent's signal mask is restored
   as mandated by the standard's requirement for pthread_create.  In some cases
   though you may wish to keep certain signals blocked to avoid race cases,
   and today this includes the helper thread created by SIGEV_THREAD where all
   signals are blocked except SIGCANCEL.  Thus the new_sigmask argument is
   primarily intended for the creation of helper threads not user threads.  */

>>> +extern int __pthread_create_internal (pthread_t *newthread,
>>> +				      const pthread_attr_t *attr,
>>> +				      void *(*start_routine) (void *),
>>> +				      void *arg, const sigset_t *new_sigmask);
>>> +#if IS_IN (libpthread)
>>> +hidden_proto (__pthread_create_internal)
>>> +#endif
>>
>> OK.
>>
>>>  
>>>  /* Functions with versioned interfaces.  */
>>>  extern int __pthread_create_2_1 (pthread_t *newthread,
>>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>>> index afd379e89a..2430d65723 100644
>>> --- a/nptl/pthread_create.c
>>> +++ b/nptl/pthread_create.c
>>> @@ -603,10 +603,10 @@ report_thread_creation (struct pthread *pd)
>>>    return false;
>>>  }
>>>  
>>> -


>>>  int
>>> -__pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>> -		      void *(*start_routine) (void *), void *arg)
>>
>> Could you please provide a detailed explanation of why we have a signal 
>> mask here?
> 
> Why isn't the comment in the header file sufficient?
> 
> I find your requirement puzzling.

I am asking for task oriented documentation for our internal APIs.

Task oriented documentation allows a non-expert to carry out an operation without
the need of an expert.

Simple API documentation lacks sufficient detail for a non-expert to use the API 
and to know what they would need to pass in this argument.

See my suggestion above.

>> Future reviewers of this code will be left wondering why the interface was
>> designed as it is, and that's difficult to explain without a more thorough
>> code audit.
> 
> Hmm.  NULL == use some default is pretty standard?  It's already used
> for the attr argument.

I hope my clarification of task-oriented vs. API helps.

>>> @@ -873,6 +880,15 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>>  
>>>    return retval;
>>>  }
>>> +hidden_def (__pthread_create_internal)
>>> +
>>> +int
>>> +__pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>> +		      void *(*start_routine) (void *), void *arg)
>>> +{
>>> +  return __pthread_create_internal (newthread, attr, start_routine, arg,
>>> +				    false);
>>
>> My preference is to use NULL with an informative cast.
>>
>> Unless you can show existing practice. I'm not a fan of using false
>> like this.
> 
> No, this is an accident.

Perfect.

-- 
Cheers,
Carlos.