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

Zhaolei <zhaolei@...fujitsu.com> 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.

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

> +
> +extern struct tm *__offtime(__kernel_time_t totalsecs, int offset,
> +			    struct tm *result);

Why isn't time_t simply, not __kernel_time_t?

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

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

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
--
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