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

[Zhaolei]

OGAWA Hirofumi wrote:
> Zhaolei <zhaolei@xxxxxxxxxxxxxx> writes:
> 
>> +/*
>> + * Similar to the struct tm in userspace <time.h>, but it needs to be here so
>> + * that the kernel source is self contained.
>> + */
>> +struct tm {
>> +	/*
>> +	 * the number of seconds after the minute, normally in the range
>> +	 * 0 to 59, but can be up to 60 to allow for leap seconds
>> +	 */
>> +	int tm_sec;
>> +	/* the number of minutes after the hour, in the range 0 to 59*/
>> +	int tm_min;
>> +	/* the number of hours past midnight, in the range 0 to 23 */
>> +	int tm_hour;
>> +	/* the day of the month, in the range 1 to 31 */
>> +	int tm_mday;
>> +	/* the number of months since January, in the range 0 to 11 */
>> +	int tm_mon;
>> +	/* the number of years since 1900 */
>> +	int tm_year;
> 
> Why isn't this "long"? "int" can overflow.

Hello, OGAWA-san:

Thanks for your review.

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.

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

>> +
>> +extern struct tm *__offtime(__kernel_time_t totalsecs, int offset,
>> +			    struct tm *result);
> 
> Why isn't time_t simply, not __kernel_time_t?

Yes, thanks.
It should be time_t.
I will fix it.

>> +/**
>> + * gmtime_r - converts the calendar time to UTC broken-down time
>> + *
>> + * @totalsecs	the number of seconds elapsed since 00:00:00 on January 1, 1970,
>> + *		Coordinated Universal Time (UTC).
>> + * @result	pointer to struct tm variable to receive broken-down time
>> + *
>> + * Return a pointer to the broken-down time result on success,
>> + * and NULL on when the year does not fit into an integer.
>> + *
>> + * Similar to the gmtime_r() in glibc, broken-down time is expressed in
>> + * Coordinated Universal Time (UTC).
>> + */
>> +static inline struct tm *gmtime_r(__kernel_time_t totalsecs, struct tm *result)
>> +{
>> +	return __offtime(totalsecs, 0, result);
>> +}
>> +
>> +/**
>> + * localtime_r - converts the calendar time to local broken-down time
>> + *
>> + * @totalsecs	the number of seconds elapsed since 00:00:00 on January 1, 1970,
>> + *		Coordinated Universal Time (UTC).
>> + * @result	pointer to struct tm variable to receive broken-down time
>> + *
>> + * Return a pointer to the broken-down time result on success,
>> + * and NULL on when the year does not fit into an integer.
>> + *
>> + * Similar to the localtime_r() in glibc, broken-down time is expressed
>> + * relative to sys_tz.
>> + */
>> +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...

>> +/*
>> + * 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.
> 
>> +{
>> +	return (year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0);
>> +}
> 
>> +/**
>> + * __offtime - converts the calendar time to local broken-down time
>> + *
>> + * @totalsecs	the number of seconds elapsed since 00:00:00 on January 1, 1970,
>> + *		Coordinated Universal Time (UTC).
>> + * @offset	offset seconds adding to totalsecs.
>> + * @result	pointer to struct tm variable to receive broken-down time
>> + *
>> + * Return a pointer to the broken-down time result on success,
>> + * and NULL on when the year does not fit into an integer.
>> + *
>> + * This function is for internal use, call gmtime_r() and localtime_r() instead.
>> + */ 
>> +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)?

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.

Hi, Andrew
What is your opinion?

Thanks
Zhaolei

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