Re: [PATCH 4/7] nptl: pthread_rwlock: Move timeout validation into _full functions

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 27/02/2019 15:23, Mike Crowe wrote:
> As recommended by the comments in the implementations of
> pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
> timeout validity checks into the corresponding pthread_rwlock_rdlock_full
> and pthread_rwlock_wrlock_full functions. Since these functions may be
> called with abstime == NULL, an extra check for that is necessary too.
> ---
>  nptl/pthread_rwlock_common.c      | 20 ++++++++++++++++++++
>  nptl/pthread_rwlock_timedrdlock.c | 10 ----------
>  nptl/pthread_rwlock_timedwrlock.c | 10 ----------
>  3 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
> index 89ba21a..120b880 100644
> --- a/nptl/pthread_rwlock_common.c
> +++ b/nptl/pthread_rwlock_common.c
> @@ -282,6 +282,16 @@ __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock,
>  {
>    unsigned int r;
>  
> +  /* Make sure any passed in timeout value is valid.  Note that the previous
> +     implementation assumed that this check *must* not be performed if there
> +     would in fact be no blocking; however, POSIX only requires that "the
> +     validity of the abstime parameter need not be checked if the lock can be
> +     immediately acquired" (i.e., we need not but may check it).  */
> +  if (abstime
> +      && __glibc_unlikely (abstime->tv_nsec >= 1000000000
> +      || abstime->tv_nsec < 0))
> +    return EINVAL;
> +

Couldn't we create a consolidate implementation for this check instead of
duplicate it?

>    /* Make sure we are not holding the rwlock as a writer.  This is a deadlock
>       situation we recognize and report.  */
>    if (__glibc_unlikely (atomic_load_relaxed (&rwlock->__data.__cur_writer)
> @@ -576,6 +586,16 @@ static __always_inline int
>  __pthread_rwlock_wrlock_full (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> +  /* Make sure any passed in timeout value is valid.  Note that the previous
> +     implementation assumed that this check *must* not be performed if there
> +     would in fact be no blocking; however, POSIX only requires that "the
> +     validity of the abstime parameter need not be checked if the lock can be
> +     immediately acquired" (i.e., we need not but may check it).  */
> +  if (abstime
> +      && __glibc_unlikely (abstime->tv_nsec >= 1000000000
> +      || abstime->tv_nsec < 0))
> +    return EINVAL;
> +
>    /* Make sure we are not holding the rwlock as a writer.  This is a deadlock
>       situation we recognize and report.  */
>    if (__glibc_unlikely (atomic_load_relaxed (&rwlock->__data.__cur_writer)
> diff --git a/nptl/pthread_rwlock_timedrdlock.c b/nptl/pthread_rwlock_timedrdlock.c
> index aa00530..84c1983 100644
> --- a/nptl/pthread_rwlock_timedrdlock.c
> +++ b/nptl/pthread_rwlock_timedrdlock.c
> @@ -23,15 +23,5 @@ int
>  pthread_rwlock_timedrdlock (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> -  /* Make sure the passed in timeout value is valid.  Note that the previous
> -     implementation assumed that this check *must* not be performed if there
> -     would in fact be no blocking; however, POSIX only requires that "the
> -     validity of the abstime parameter need not be checked if the lock can be
> -     immediately acquired" (i.e., we need not but may check it).  */
> -  /* ??? Just move this to __pthread_rwlock_rdlock_full?  */
> -  if (__glibc_unlikely (abstime->tv_nsec >= 1000000000
> -      || abstime->tv_nsec < 0))
> -    return EINVAL;
> -
>    return __pthread_rwlock_rdlock_full (rwlock, abstime);
>  }
> diff --git a/nptl/pthread_rwlock_timedwrlock.c b/nptl/pthread_rwlock_timedwrlock.c
> index 3c92e44..f0b745d 100644
> --- a/nptl/pthread_rwlock_timedwrlock.c
> +++ b/nptl/pthread_rwlock_timedwrlock.c
> @@ -23,15 +23,5 @@ int
>  pthread_rwlock_timedwrlock (pthread_rwlock_t *rwlock,
>      const struct timespec *abstime)
>  {
> -  /* Make sure the passed in timeout value is valid.  Note that the previous
> -     implementation assumed that this check *must* not be performed if there
> -     would in fact be no blocking; however, POSIX only requires that "the
> -     validity of the abstime parameter need not be checked if the lock can be
> -     immediately acquired" (i.e., we need not but may check it).  */
> -  /* ??? Just move this to __pthread_rwlock_wrlock_full?  */
> -  if (__glibc_unlikely (abstime->tv_nsec >= 1000000000
> -      || abstime->tv_nsec < 0))
> -    return EINVAL;
> -
>    return __pthread_rwlock_wrlock_full (rwlock, abstime);
>  }
>