Re: [PATCH v3 7/7] y2038: linux: Provide ___gettimeofday64 implementation

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

On 10/02/2020 13:57, Lukasz Majewski wrote:
>>>> 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.
> 
> Ok.
> 
>>
>> 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).
> 
> I do guess that you refer to:
> https://elixir.bootlin.com/linux/latest/source/lib/vdso/gettimeofday.c#L144

Yes (I was using current master branch instead of a tag release).

> 
> We could rely on the kernel if there weren't conversions (64 <->
> 32 bit time ) needed. 

My point was that kernel vDSO gettimeofday implementation already does 
the tv check before setting its value, so it should be an QoI to do this
on glibc as well.

> 
>> However, I think we should be done in a separated patch and for 'all'
>> implementation to get a concise behaviour.
> 
> Shall I:
> 
> 1. Extend this patch to add this extra check to all eligible
> gettimeofday places.
> 
> 2. Do not check if tv is NULL at all
> 
> 3. Do not check if tv is NULL in this particular patch and prepare next
> patch which would add check for tv != NULL to all gettimeofday
> implementations in glibc ?

I would prefer to remove the check on this patch and send an extra
patch to actually adding the check on all implementations.

> 
>>
>>
>>>
>>>
>>> Links:
>>>
>>> [1] - https://linux.die.net/man/2/gettimeofday
>>>
>>>
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>   
> 
> 
> 
> 
> 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
>