Re: [PATCH] ARM: Mes C Library: Add compile stub for time.

[Jan Nieuwenhuizen]

Danny Milosavljevic writes:

Hello Danny!

> this patch from you is required on mes master to support ARM.

Sure!

> On Tue,  2 Jun 2020 21:26:36 +0200
> Danny Milosavljevic <dannym@scratchpost.org> wrote:
>
>> * lib/linux/time.c (time)[SYS_gettimeofday]: Implement using gettimeofday.
>
> I've fixed a typo in "Implement".

Thanks!

>> (time)[!SYS_time && !SYS_gettimeofday]: Compile stub.
>> ---
>>  lib/linux/time.c | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>> 
>> diff --git a/lib/linux/time.c b/lib/linux/time.c
>> index 17af241..e33fe48 100644
>> --- a/lib/linux/time.c
>> +++ b/lib/linux/time.c
>> @@ -22,8 +22,45 @@
>>  #include <syscall.h>
>>  #include <time.h>
>>  
>> +#if SYS_time
>> +
>>  time_t
>>  time (time_t * result)
>>  {
>>    return _sys_call1 (SYS_time, (long) result);
>>  }
>
> Can this syscall fail?  I guess not much we can do if it does fail...

Probably...

> Can't result == NULL ?  According to man page it can.

> So seems to be fine to pass NULL.  Some weird interface IMO :)

Yes, seems "convenient" but big mistake?

> Reading this, I think it would be better if your patch preferred to use
> SYS_gettimeofday and fell back to SYS_time if that is unavailable.
> I think that's what Linux intended.

You want to do that, or otherwise please leave a comment so that what
you found doesn't get lost!

Thanks for not blindly applying and looking into this!  I tend to be
terribly sloppy and "easy" when I want to get "things to work".  0 is
also a fine number, time value, pointer, my mind says....well until
./configure tries to test make timestamps, for example, and the code
needs to be revisited... :-/

>> +  gettimeofday (&tv, &tz);
>
> Can't gettimeofday fail?  At least using TV is then usually pretty bad.

Probably...

> Otherwise LGTM!

:-)  Great, when done, please push!

Janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com