Re: [PATCH v2] Consolidate pthread_attr value validation

[Roland McGrath <roland at hack dot frob dot com>]

> 	* 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).

> @@ -589,4 +589,68 @@ extern void __wait_lookup_done (void) attribute_hidden;
>  # define USE_REQUEUE_PI(mut) 0
>  #endif
>  
> +#include <errno.h>

Put this at the top with the others.

> +/* 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.

> +  int ret;
> +  if ((ret = check_sched_priority_attr (param->sched_priority,
> +					iattr->schedpolicy)) != 0)
> +    return ret;

Don't use assignments in tests unless it's really the most concise.
Here it's shorter to write:

	int ret = ...;
	if (ret)
	  return ret;

(Boolean coercion is OK for errno-or-zero tests though not for others.
Write "ret != 0" if you prefer.)

> +check_sched_priority_attr (int pr, int pol)
> +{
> +  int min = sched_get_priority_min (pol);
> +  int max = sched_get_priority_max (pol);

Call the __ versions.  There may not be any name space issue,
but might as well use internal symbols when we already have them.

> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -532,12 +532,8 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg)
>  	}
>  
>        /* 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.


Thanks,
Roland