Message ID | 20191108153344.10949-1-lukma@denx.de |
---|---|
State | Superseded |
Headers |
Received: (qmail 63637 invoked by alias); 8 Nov 2019 15:34:07 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 63622 invoked by uid 89); 8 Nov 2019 15:34:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: mail-out.m-online.net From: Lukasz Majewski <lukma@denx.de> To: Joseph Myers <joseph@codesourcery.com>, Paul Eggert <eggert@cs.ucla.edu> Cc: Alistair Francis <alistair23@gmail.com>, Alistair Francis <alistair.francis@wdc.com>, GNU C Library <libc-alpha@sourceware.org>, Adhemerval Zanella <adhemerval.zanella@linaro.org>, Florian Weimer <fweimer@redhat.com>, Florian Weimer <fw@deneb.enyo.de>, Zack Weinberg <zackw@panix.com>, Carlos O'Donell <carlos@redhat.com>, Lukasz Majewski <lukma@denx.de> Subject: [PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity Date: Fri, 8 Nov 2019 16:33:43 +0100 Message-Id: <20191108153344.10949-1-lukma@denx.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Lukasz Majewski
Nov. 8, 2019, 3:33 p.m. UTC
The check if passed nanoseconds via struct __timespec64's *tp pointer is also performed in the Linux kernel. Remove it from glibc to avoid duplication. --- sysdeps/unix/sysv/linux/clock_settime.c | 7 ------- 1 file changed, 7 deletions(-)
Comments
On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote: > > The check if passed nanoseconds via struct __timespec64's *tp pointer is > also performed in the Linux kernel. Remove it from glibc to avoid > duplication. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > sysdeps/unix/sysv/linux/clock_settime.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c > index bda113809b..6706dbb31f 100644 > --- a/sysdeps/unix/sysv/linux/clock_settime.c > +++ b/sysdeps/unix/sysv/linux/clock_settime.c > @@ -25,13 +25,6 @@ > int > __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp) > { > - /* Make sure the time cvalue is OK. */ > - if (! valid_nanoseconds (tp->tv_nsec)) > - { > - __set_errno (EINVAL); > - return -1; > - } > - > #ifdef __ASSUME_TIME64_SYSCALLS > # ifndef __NR_clock_settime64 > # define __NR_clock_settime64 __NR_clock_settime > -- > 2.20.1 >
Dear Joseph, Paul, > The check if passed nanoseconds via struct __timespec64's *tp pointer > is also performed in the Linux kernel. Remove it from glibc to avoid > duplication. > --- > sysdeps/unix/sysv/linux/clock_settime.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c > b/sysdeps/unix/sysv/linux/clock_settime.c index > bda113809b..6706dbb31f 100644 --- > a/sysdeps/unix/sysv/linux/clock_settime.c +++ > b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@ > int > __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp) > { > - /* Make sure the time cvalue is OK. */ > - if (! valid_nanoseconds (tp->tv_nsec)) > - { > - __set_errno (EINVAL); > - return -1; > - } > - I'm just wondering if this patch is OK, as with other patches, which convert time to use 64 bit syscalls we do rely on Linux kernel to check the nanoseconds (and return proper error). > #ifdef __ASSUME_TIME64_SYSCALLS > # ifndef __NR_clock_settime64 > # define __NR_clock_settime64 __NR_clock_settime Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Dear Joseph, Paul, > Dear Joseph, Paul, > > > The check if passed nanoseconds via struct __timespec64's *tp > > pointer is also performed in the Linux kernel. Remove it from glibc > > to avoid duplication. > > --- > > sysdeps/unix/sysv/linux/clock_settime.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c > > b/sysdeps/unix/sysv/linux/clock_settime.c index > > bda113809b..6706dbb31f 100644 --- > > a/sysdeps/unix/sysv/linux/clock_settime.c +++ > > b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@ > > int > > __clock_settime64 (clockid_t clock_id, const struct __timespec64 > > *tp) { > > - /* Make sure the time cvalue is OK. */ > > - if (! valid_nanoseconds (tp->tv_nsec)) > > - { > > - __set_errno (EINVAL); > > - return -1; > > - } > > - > > I'm just wondering if this patch is OK, as with other patches, which > convert time to use 64 bit syscalls we do rely on Linux kernel to > check the nanoseconds (and return proper error). Are there any comments regarding this patch? > > > #ifdef __ASSUME_TIME64_SYSCALLS > > # ifndef __NR_clock_settime64 > > # define __NR_clock_settime64 __NR_clock_settime > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 11/27/19 9:04 AM, Lukasz Majewski wrote: >> a/sysdeps/unix/sysv/linux/clock_settime.c +++ >> b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@ >> int >> __clock_settime64 (clockid_t clock_id, const struct __timespec64 >> *tp) { >> - /* Make sure the time cvalue is OK. */ >> - if (! valid_nanoseconds (tp->tv_nsec)) >> - { >> - __set_errno (EINVAL); >> - return -1; >> - } >> - > I'm just wondering if this patch is OK, as with other patches, which > convert time to use 64 bit syscalls we do rely on Linux kernel to > check the nanoseconds (and return proper error). Suppose tp->tv_sec == 1 && tp->tv_nsec == -1 && !defined __ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64. Then the current code will fail with errno == EINVAL, but with the proposed patch the code will succeed and set the time to 1 second after the Epoch. Code should always check for valid nanoseconds before calling valid_timespec64_to_timespec with possibly-invalid input. In this function, the check can be done at about the same time as the in_time_t_range check; that'd be better than what the current code does.
diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c index bda113809b..6706dbb31f 100644 --- a/sysdeps/unix/sysv/linux/clock_settime.c +++ b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@ int __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp) { - /* Make sure the time cvalue is OK. */ - if (! valid_nanoseconds (tp->tv_nsec)) - { - __set_errno (EINVAL); - return -1; - } - #ifdef __ASSUME_TIME64_SYSCALLS # ifndef __NR_clock_settime64 # define __NR_clock_settime64 __NR_clock_settime