[4/7] nptl: pthread_rwlock: Move timeout validation into _full functions
Commit Message
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(-)
Comments
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);
> }
>
@@ -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)
@@ -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);
}
@@ -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);
}