Re: [PATCH v3 1/2] Add function to convert between calendar time and broken-down time for universal use

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

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>