This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4 07/12] nptl: pthread_rwlock: Move timeout validation into _full functions
On 18/06/2019 13:33, 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 (__pthread_rwlock_rdlock_full):
> Check validity of abstime parameter.
> (__pthread_rwlock_rwlock_full): Likewise.
>
> * nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> Remove check for validity of abstime parameter.
>
> * nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock):
> Likewise.
LGTM, I will commit this for you.
> ---
> ChangeLog | 14 ++++++++++++++
> nptl/pthread_rwlock_common.c | 20 ++++++++++++++++++++
> nptl/pthread_rwlock_timedrdlock.c | 10 ----------
> nptl/pthread_rwlock_timedwrlock.c | 10 ----------
> 4 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 8386d5b..452d047 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,19 @@
> 2019-05-27 Mike Crowe <mac@mcrowe.com>
>
> + nptl: pthread_rwlock: Move timeout validation into _full functions
> +
> + * nptl/pthread_rwlock_common.c (__pthread_rwlock_rdlock_full):
> + Check validity of abstime parameter.
> + (__pthread_rwlock_rwlock_full): Likewise.
> +
> + * nptl/pthread_rwlock_timedrdlock.c (pthread_rwlock_timedrdlock):
> + Remove check for validity of abstime parameter.
> +
> + * nptl/pthread_rwlock_timedwrlock.c (pthread_rwlock_timedwrlock):
> + Likewise.
> +
> +2019-05-27 Mike Crowe <mac@mcrowe.com>
> +
> nptl: Add POSIX-proposed pthread_cond_clockwait which behaves just
> like pthread_cond_timedwait except it always measures abstime
> against the supplied clockid.
> 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;
> +
> /* 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);
> }
>