[ping][PATCH v3] Consolidate pthread_attr value validation

[Siddhesh Poyarekar <siddhesh@redhat.com>]

Ping!


On Tue, Mar 26, 2013 at 05:18:28PM +0530, Siddhesh Poyarekar wrote:
> On Fri, Mar 22, 2013 at 11:22:34AM -0700, Roland McGrath wrote:
> > The cases here are simply optimization choices.  Those should be left to
> > the compiler.  'inline' is a hint to the compiler about optimization
> > choice, though from the GCC documentation it's not clear to me it really
> > makes any difference in practice at -O2, but clear that it doesn't make any
> > difference at -O3.  Hence it's clear that 'inline' should never be used in
> > .c files.  That was the context of the previous discussion but I don't
> > think we were clear about the distinction between cases in .c files and
> > cases in .h files.
> 
> OK, thanks for the clarification.  Here's v3 of the cleanup patch.
> 
> Siddhesh
> 
> 	* pthreadP.h (check_sched_policy_attr): New inline function.
> 	(check_sched_priority_attr): Likewise.
> 	(check_stacksize_attr): Likewise.
> 	(__kernel_cpumask_size, __determine_cpumask_size): Declare
> 	extern.
> 	(check_cpuset_attr): New inline function.
> 	* pthread_attr_setschedparam (__pthread_attr_setschedparam):
> 	Use check_sched_priority_attr.
> 	* pthread_attr_setschedpolicy.c
> 	(__pthread_attr_setschedpolicy): Use check_sched_policy_attr.
> 	* pthread_attr_setstack.c (__pthread_attr_setstack): Use
> 	check_stacksize_attr.
> 	* pthread_attr_setstacksize.c (__pthread_attr_setstacksize):
> 	Likewise.
> 	* sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> 	(__pthread_attr_setaffinity_new): Use check_cpuset_attr.
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 39e930c..a34a889 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -31,7 +31,7 @@
>  #include <pthread-functions.h>
>  #include <atomic.h>
>  #include <kernel-features.h>
> -
> +#include <errno.h>
>  
>  /* Atomic operations on TLS memory.  */
>  #ifndef THREAD_ATOMIC_CMPXCHG_VAL
> @@ -591,4 +591,66 @@ extern void __wait_lookup_done (void) attribute_hidden;
>  # define USE_REQUEUE_PI(mut) 0
>  #endif
>  
> +/* Returns 0 if POL is a valid scheduling policy.  */
> +static inline int
> +check_sched_policy_attr (int pol)
> +{
> +  if (pol == SCHED_OTHER || pol == SCHED_FIFO || pol == SCHED_RR)
> +    return 0;
> +
> +  return EINVAL;
> +}
> +
> +/* Returns 0 if PR is within the accepted range of priority values for
> +   the scheduling policy POL or EINVAL otherwise.  */
> +static inline int
> +check_sched_priority_attr (int pr, int pol)
> +{
> +  int min = __sched_get_priority_min (pol);
> +  int max = __sched_get_priority_max (pol);
> +
> +  if (min >= 0 && max >= 0 && pr >= min && pr <= max)
> +    return 0;
> +
> +  return EINVAL;
> +}
> +
> +/* Returns 0 if ST is a valid stack size for a thread stack and EINVAL
> +   otherwise.  */
> +static inline int
> +check_stacksize_attr (size_t st)
> +{
> +  if (st >= PTHREAD_STACK_MIN)
> +    return 0;
> +
> +  return EINVAL;
> +}
> +
> +/* Defined in pthread_setaffinity.c.  */
> +extern size_t __kernel_cpumask_size attribute_hidden;
> +extern int __determine_cpumask_size (pid_t tid);
> +
> +/* Returns 0 if CS and SZ are valid values for the cpuset and cpuset size
> +   respectively.  Otherwise it returns an error number.  */
> +static inline int
> +check_cpuset_attr (const cpu_set_t *cs, const size_t sz)
> +{
> +  if (__kernel_cpumask_size == 0)
> +    {
> +      int res = __determine_cpumask_size (THREAD_SELF->tid);
> +      if (res)
> +	return res;
> +    }
> +
> +  /* Check whether the new bitmask has any bit set beyond the
> +     last one the kernel accepts.  */
> +  for (size_t cnt = __kernel_cpumask_size; cnt < sz; ++cnt)
> +    if (((char *) cs)[cnt] != '\0')
> +      /* Found a nonzero byte.  This means the user request cannot be
> +	 fulfilled.  */
> +      return EINVAL;
> +
> +  return 0;
> +}
> +
>  #endif	/* pthreadP.h */
> diff --git a/nptl/pthread_attr_setschedparam.c b/nptl/pthread_attr_setschedparam.c
> index ec1a8bd..a167f15 100644
> --- a/nptl/pthread_attr_setschedparam.c
> +++ b/nptl/pthread_attr_setschedparam.c
> @@ -30,11 +30,10 @@ __pthread_attr_setschedparam (attr, param)
>    assert (sizeof (*attr) >= sizeof (struct pthread_attr));
>    struct pthread_attr *iattr = (struct pthread_attr *) attr;
>  
> -  int min = sched_get_priority_min (iattr->schedpolicy);
> -  int max = sched_get_priority_max (iattr->schedpolicy);
> -  if (min == -1 || max == -1
> -      || param->sched_priority > max || param->sched_priority < min)
> -    return EINVAL;
> +  int ret = check_sched_priority_attr (param->sched_priority,
> +				       iattr->schedpolicy);
> +  if (ret)
> +    return ret;
>  
>    /* Copy the new values.  */
>    memcpy (&iattr->schedparam, param, sizeof (struct sched_param));
> diff --git a/nptl/pthread_attr_setschedpolicy.c b/nptl/pthread_attr_setschedpolicy.c
> index 2fa6921..4fe0b8e 100644
> --- a/nptl/pthread_attr_setschedpolicy.c
> +++ b/nptl/pthread_attr_setschedpolicy.c
> @@ -32,8 +32,9 @@ __pthread_attr_setschedpolicy (attr, policy)
>    iattr = (struct pthread_attr *) attr;
>  
>    /* Catch invalid values.  */
> -  if (policy != SCHED_OTHER && policy != SCHED_FIFO && policy != SCHED_RR)
> -    return EINVAL;
> +  int ret = check_sched_policy_attr (policy);
> +  if (ret)
> +    return ret;
>  
>    /* Store the new values.  */
>    iattr->schedpolicy = policy;
> diff --git a/nptl/pthread_attr_setstack.c b/nptl/pthread_attr_setstack.c
> index 783cffe..4bd314e 100644
> --- a/nptl/pthread_attr_setstack.c
> +++ b/nptl/pthread_attr_setstack.c
> @@ -39,8 +39,9 @@ __pthread_attr_setstack (attr, stackaddr, stacksize)
>    iattr = (struct pthread_attr *) attr;
>  
>    /* Catch invalid sizes.  */
> -  if (stacksize < PTHREAD_STACK_MIN)
> -    return EINVAL;
> +  int ret = check_stacksize_attr (stacksize);
> +  if (ret)
> +    return ret;
>  
>  #ifdef EXTRA_PARAM_CHECKS
>    EXTRA_PARAM_CHECKS;
> diff --git a/nptl/pthread_attr_setstacksize.c b/nptl/pthread_attr_setstacksize.c
> index b7f2919..585bf08 100644
> --- a/nptl/pthread_attr_setstacksize.c
> +++ b/nptl/pthread_attr_setstacksize.c
> @@ -37,8 +37,9 @@ __pthread_attr_setstacksize (attr, stacksize)
>    iattr = (struct pthread_attr *) attr;
>  
>    /* Catch invalid sizes.  */
> -  if (stacksize < PTHREAD_STACK_MIN)
> -    return EINVAL;
> +  int ret = check_stacksize_attr (stacksize);
> +  if (ret)
> +    return ret;
>  
>    iattr->stacksize = stacksize;
>  
> diff --git a/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c b/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> index 8b7a499..b4335c5 100644
> --- a/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> +++ b/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> @@ -25,9 +25,6 @@
>  #include <shlib-compat.h>
>  
>  
> -/* Defined in pthread_setaffinity.c.  */
> -extern size_t __kernel_cpumask_size attribute_hidden;
> -extern int __determine_cpumask_size (pid_t tid);
>  
>  
>  int
> @@ -47,21 +44,10 @@ __pthread_attr_setaffinity_new (pthread_attr_t *attr, size_t cpusetsize,
>      }
>    else
>      {
> -      if (__kernel_cpumask_size == 0)
> -	{
> -	  int res = __determine_cpumask_size (THREAD_SELF->tid);
> -	  if (res != 0)
> -	    /* Some serious problem.  */
> -	    return res;
> -	}
> +      int ret = check_cpuset_attr (cpuset, cpusetsize);
>  
> -      /* Check whether the new bitmask has any bit set beyond the
> -	 last one the kernel accepts.  */
> -      for (size_t cnt = __kernel_cpumask_size; cnt < cpusetsize; ++cnt)
> -	if (((char *) cpuset)[cnt] != '\0')
> -	  /* Found a nonzero byte.  This means the user request cannot be
> -	     fulfilled.  */
> -	  return EINVAL;
> +      if (ret)
> +        return ret;
>  
>        if (iattr->cpusetsize != cpusetsize)
>  	{