Date:	Tue, 28 Jul 2009 11:05:01 +0800
From:	Zhaolei <zhaolei@...fujitsu.com>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>,
	Andrew Morton <akpm@...ux-foundation.org>, mingo@...e.hu
CC:	Pavel Machek <pavel@....cz>, tglx@...utronix.de,
	linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH v3 1/2] Add function to convert between calendar time
 and broken-down time for universal use

OGAWA Hirofumi wrote:
> Zhaolei <zhaolei@...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?

Hello, Ogawa-san:
CC: Andrew, ingo:

Yes, time_t on 32-bit machine will overflow on year-2038.
So tm.tm_year will NEVER overflow on 32-bit machine.

And in 64-bit machine, int type of tm.tm_year will overflow in
year 2,147,485,548, Is that enough?

I also like your suggestion to use long for year, so gmtime/localtime
will never return false on both 32 and 64 bit machine.

(btw, I don't understand why time_t is long, it have different length(limit)
in different arch, and make disunity)

>>>> +	/* 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).

There is no users of printk("%pf"), why not remove this?
And at least, I found one: drivers/char/efirtc.c
If I continue searching, maybe more.

>>>> +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.

IHMO, user of these functions should understand what these functions is and
use these functions for right way.
If we give user a cdrom, it is user's responsibility not using is as cup stand.

At least, there function can make following source a fairy:
fs/ncpfs/dir.c
fs/smbfs/proc.c
fs/fat/misc.c
fs/udf/udftime.c
fs/cifs/netmisc.c
net/netfilter/xt_time.c
drivers/scsi/ips.c
...

It is different with user-land just because it lakes of large-complex locale
database.

>>>> +/*
>>>> + * 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".

Ok, or int year(after we get conclusion of long/int).

>>>> +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.

So, maybe another function name as unmktime?

>> 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?

I will thanks you for attention and reporting first, then discuss on LKML
about detail of this problem, and get a conclusion and fix.

Now we are in step2: discussing.
We need to get a best way to fix this to make users happy.

Thanks
Zhaolei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists