This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 7/7] y2038: linux: Provide ___gettimeofday64 implementation
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Lukasz Majewski <lukma at denx dot de>, Joseph Myers <joseph at codesourcery dot com>
- Cc: Paul Eggert <eggert at cs dot ucla dot edu>, Alistair Francis <alistair23 at gmail dot com>, Alistair Francis <alistair dot francis at wdc dot com>, GNU C Library <libc-alpha at sourceware dot org>, Siddhesh Poyarekar <siddhesh at gotplt dot org>, Florian Weimer <fweimer at redhat dot com>, Florian Weimer <fw at deneb dot enyo dot de>, Zack Weinberg <zackw at panix dot com>, Carlos O'Donell <carlos at redhat dot com>, Andreas Schwab <schwab at suse dot de>
- Date: Mon, 10 Feb 2020 13:15:43 -0300
- Subject: Re: [PATCH v3 7/7] y2038: linux: Provide ___gettimeofday64 implementation
- References: <20200129125914.11221-1-lukma@denx.de> <20200129125914.11221-7-lukma@denx.de> <1bfd0cce-e889-7fce-fe7b-d565ca1a1806@linaro.org> <20200205010552.6f0bac91@jawa> <20200208231545.038af74a@jawa>
On 08/02/2020 19:15, Lukasz Majewski wrote:
> Hi Adhemerval,
>
>>>> #else /* USE_IFUNC_GETTIMEOFDAY */
>>>> -# include <time/gettimeofday.c>
>>>> +/* Conversion of gettimeofday function to support 64 bit time on
>>>> archs
>>>> + with __WORDSIZE == 32 and __TIMESIZE == 32/64 */
>>>> +#include <errno.h>
>>>> +
>>>> +int
>>>> +___gettimeofday64 (struct __timeval64 *restrict tv, void
>>>> *restrict tz) +{
>>>> + if (__glibc_unlikely (tz != 0))
>>>> + memset (tz, 0, sizeof (struct timezone));
>>>> +
>>>> + struct __timespec64 ts64;
>>>> + int ret = __clock_gettime64 (CLOCK_REALTIME, &ts64);
>>>> +
>>>> + if (ret == 0 && tv)
>>>> + *tv = timespec64_to_timeval64 (ts64);
>>>
>>> No implicit checks. Also, we already set 'tv' with nonull
>>> attribute, so I am not sure if it is worth to add an extra check
>>> for 'tv' validity (specially because users tend to expect low
>>> latency for the symbol).
>>>
>>> In any case, if the idea is to add such check as QoI I think it
>>> would be better to do a early bail before actually issue
>>> __clock_gettime64.
>>
>> No, this was just my mistake. There was a discussion with Paul and
>> Joseph earlier. We shall _only_ check for NULL when it is required by
>> syscalls/command documentation. This is the case for e.g. setitimer's
>> *old_value pointer.
>
> I've double check this and in the documentation/manual [1] for
> gettimeofday there is a sentence:
>
> ----8<--------
> If either tv or tz is NULL, the corresponding structure is not set or
> returned. (However, compilation warnings will result if tv is NULL.)
> ---->8--------
>
> That was the rationale to add the check
> if (ret == 0 && tv)
>
>
> I also think that the code as is now is correct - it returns the result
> of getting the time from Linux, but it is not updating the tv structure.
The man-pages is not really the glibc manual, but rather documents de facto
glibc/kernel behaviour. The glibc 'gettimeofday' entry in manual
(manual/time.texi) is also not explicit about this, and the generic
implementation (time/gettimeofday.c) also does not add this test.
But again, I am not against of this change as QoI and it is what kernel vDSO
symbol does anyway (lib/vdso/gettimeofday.c:270). However, I think we should
be done in a separated patch and for 'all' implementation to get a concise
behaviour.
>
>
> Links:
>
> [1] - https://linux.die.net/man/2/gettimeofday
>
>
> Best regards,
>
> Lukasz Majewski
>