Messages in this thread | | | From | OGAWA Hirofumi <> | Subject | Re: [PATCH v3 1/2] Add function to convert between calendar time and broken-down time for universal use | Date | Mon, 27 Jul 2009 15:04:21 +0900 |
| |
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>
| |