lkml.org 
[lkml]   [2009]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 1/2] Add function to convert between calendar time and broken-down time for universal use
Date
Zhaolei <zhaolei@cn.fujitsu.com> writes:

>>> + /* the number of years since 1900 */
>>> + int tm_year;
>>
>> Why isn't this "long"? "int" can overflow.
>
> I selected int to make it same with user-space struct tm.
>
> In 32-bit machine, long have same length with int, and still can overflow,
> It we want to avoid it in all platform, we should use long long,
> and make division complex.
>
> Maybe make it same with user-space struct tm is ok.

time_t is also "long" on 32-bit machine, it does overflow?

>>> + /* the number of days since Sunday, in the range 0 to 6 */
>>> + int tm_wday;
>>> + /* the number of days since January 1, in the range 0 to 365 */
>>> + int tm_yday;
>>> +};
>>
>> Those are needed?
>
> Those are not needed by FAT-fs's code, but as a library,
> I keep them for generic use and keep same with user-space struct tm.

Yes. But, if there is no users, why need? I guess it makes slow thing,
and it is slow than FAT's one (because FAT's one is optimized for
FAT's timestamp range more or less).

>>> +static inline struct tm *localtime_r(__kernel_time_t totalsecs,
>>> + struct tm *result)
>>> +{
>>> + return __offtime(totalsecs, -sys_tz.tz_minuteswest * 60, result);
>>> +}
>>
>> I think those are confusable. The real function of those needs to handle
>> timezone database. Especially, sys_tz.tz_minuteswest in localtime_r() is
>> known as wrong.
>>
>> Are you going to fix it? Otherwise I don't think it would not be good to
>> use it easily as generic function like this.
>
> Actually, it is hard to select.
>
> I don't schedule to introduce complex timezone database into kernel,
> but as you said, localtime() without timezone database is not complete.
> But localtime_r is easy to use and understood, it is similar with
> userspace same-name function...

Actually it is both (gmtime() is also needed to handle timezone).

I worry someone uses it and display/export to userland. After that, the
user will report the bug of it.

>>> +/*
>>> + * Nonzero if YEAR is a leap year (every 4 years,
>>> + * except every 100th isn't, and every 400th is).
>>> + */
>>> +static int __isleap(unsigned int year)
>>
>> long year. This breaks negative time_t.

Please "long year".

>>> +struct tm *__offtime(__kernel_time_t totalsecs, int offset, struct tm *result)
>>> +{
>>
>> So, I suggest to consolidate this code only, and don't provide
>> gmtime_r()/localtime_r(), and use more good function name for
>> __offtime() (I'm not sure, however, personally I feel __offtime is not
>> obvious what's doing).
>
> What about just use gmtime_r(rename __offtime->gmtime_r)?

gmtime() also need to handle timezone actually.

> In fact I think both way(hode original localtime/gmtime or delete them) have
> merit and demerit.
> Hode them will make it easy to use, delete them will make function more exact.

Yes. But, how do you handle bug report?

Thanks.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


\
 
 \ /
  Last update: 2009-07-27 08:07    [W:0.179 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site