Re: [PATCH v2] Consolidate pthread_attr value validation

[Siddhesh Poyarekar <siddhesh dot poyarekar at gmail dot com>]

On 19 March 2013 23:43, Roland McGrath <roland@hack.frob.com> wrote:
>>       * pthreadP.h (check_sched_policy_attr): New macro to validate
>>       a scheduling policy value.
>>       (check_sched_priority_attr): New macro to validate scheduling
>>       priority for a policy.
>>       (check_stacksize_attr): New macro to validate thread stack
>>       size.
>
> They're functions.  "New inline function." is enough (you can describe
> them if you like, though the names are pretty self-explanatory).

Sorry, blind copy-paste-error.

>> +/* Returns 0 if POL is a valid scheduling policy.  */
>> +static inline int __attribute__((always_inline))
>> +check_sched_policy_attr (int pol)
>
> There's no need for always_inline.  If for some reason the compiler
> decided not to, let it.  Declaring with "inline" does the same job as
> __attribute__ ((unused)) for warning-suppression, so plain 'static
> inline' is fine for inlines defined in (internal) headers.

You had said that we should avoid using inline alone and wherever
absolutely necessary, use it along with
__attribute__((always_inline)).

http://sourceware.org/ml/libc-alpha/2013-02/msg00146.html

>>        /* Check for valid priorities.  */
>> -      int minprio = INTERNAL_SYSCALL (sched_get_priority_min, scerr, 1,
>> -                                   iattr->schedpolicy);
>> -      int maxprio = INTERNAL_SYSCALL (sched_get_priority_max, scerr, 1,
>> -                                   iattr->schedpolicy);
>> -      if (pd->schedparam.sched_priority < minprio
>> -       || pd->schedparam.sched_priority > maxprio)
>> +      if (check_sched_priority_attr (pd->schedparam.sched_priority,
>> +                                  iattr->schedpolicy))
>
> Unlike the other cases, this is (barely) a change in behavior.
> check_sched_priority_attr calls sched_get_priority_{min,max}, which
> theoretically can fail and set errno.  This code used INTERNAL_SYSCALL,
> which will never set errno.  It's not something that really matters in
> this context, but to be strict, any change in behavior ought to be done
> separately from pure reorganization.
>
> When we're changing behavior, I wonder why we have this test in
> pthread_create at all.  pthread_attr_setschedparam already does the same
> test.  The case I see is if one called pthread_attr_setschedparam
> followed by pthread_addr_setschedpolicy, and the priority was no longer
> valid for the new policy.  It's not clear to me from the wording in the
> spec (1003.1-2008) whether pthread_addr_setschedpolicy should (or even
> may) check the parameters and fail if they would be invalid for the new
> policy.  If it is not permitted to do that check, then the check in
> pthread_create is in fact required.

OK, I'll deal with these bits in a separate change.

Siddhesh
-- 
http://siddhesh.in