This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Consolidate pthread_attr value validation
- From: Siddhesh Poyarekar <siddhesh dot poyarekar at gmail dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org
- Date: Wed, 20 Mar 2013 09:24:42 +0530
- Subject: Re: [PATCH v2] Consolidate pthread_attr value validation
- References: <20130318111716 dot GE3969 at spoyarek dot pnq dot redhat dot com> <20130318165441 dot 575E52C08A at topped-with-meat dot com> <20130319113803 dot GB25837 at spoyarek dot pnq dot redhat dot com> <20130319181311 dot CC6542C081 at topped-with-meat 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