Xen devel - Feb 2012 - [PATCH] hvm: Correct RTC time offset update error due to tm->tm_year

Hi

In rtc_set_time, mktime is called to calculate seconds since 1970/01/01, 
input parameters of mktime are required to be in normal date format. 
Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However, 
the current input parameter of mktime is tm->tm_year, and it is the 
number of years since 1900. (For example, if current time is 2012/12/31, 
and tm->tm_year is 112). This is not suitable for requirement of mktime. 
So I think tm->tm_year should be changed to tm->tm_year+1900 when 
calling mktime. Please check the patch attached.

Thanks,
Annie


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of ANNIE LI
> Sent: Monday, February 20, 2012 3:20 PM
> 
> Hi
> 
> In rtc_set_time, mktime is called to calculate seconds since 1970/01/01,
input
> parameters of mktime are required to be in normal date format.
> Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However, the
> current input parameter of mktime is tm->tm_year, and it is the number
of years
> since 1900. (For example, if current time is 2012/12/31, and tm->tm_year
is 112).
> This is not suitable for requirement of mktime.
> So I think tm->tm_year should be changed to tm->tm_year+1900 when
calling
> mktime. Please check the patch attached.
> 
No, tm_year is the number of years since 1900, not the normal date format.

best regards
yang

The mktime which used in xen is different from standard C. I think the
best way is change it same as normal mktime, or else, other people
will make the same mistake too.

2012/2/20, Zhang, Yang Z <yang.z.zhang@intel.com>:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of ANNIE LI
>> Sent: Monday, February 20, 2012 3:20 PM
>>
>> Hi
>>
>> In rtc_set_time, mktime is called to calculate seconds since
1970/01/01,
>> input
>> parameters of mktime are required to be in normal date format.
>> Such as: year=1980, mon=12, day=31, hour=23, min=59, sec=59. However,
the
>> current input parameter of mktime is tm->tm_year, and it is the
number of
>> years
>> since 1900. (For example, if current time is 2012/12/31, and
tm->tm_year
>> is 112).
>> This is not suitable for requirement of mktime.
>> So I think tm->tm_year should be changed to tm->tm_year+1900 when
calling
>> mktime. Please check the patch attached.
>>
> No, tm_year is the number of years since 1900, not the normal date format.
>
> best regards
> yang
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

-- 
young

> -----Original Message-----
> From: young zhang [mailto:young.zhang.free@gmail.com]
> Sent: Monday, February 20, 2012 11:04 PM
> To: Zhang, Yang Z
> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan Magenheimer;
> Konrad Rzeszutek Wilk
> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update error
due
> to tm->tm_year
> 
> The mktime which used in xen is different from standard C. I think the best
way is
> change it same as normal mktime, or else, other people will make the same
> mistake too.
yes. The name will mislead the people who not look into the code, including me.
:)

best regards
yang

On 2012-2-21 7:54, Zhang, Yang Z wrote:
>> -----Original Message-----
>> From: young zhang [mailto:young.zhang.free@gmail.com]
>> Sent: Monday, February 20, 2012 11:04 PM
>> To: Zhang, Yang Z
>> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan
Magenheimer;
>> Konrad Rzeszutek Wilk
>> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset update
error due
>> to tm->tm_year
>>
>> The mktime which used in xen is different from standard C. I think the
best way is
>> change it same as normal mktime, or else, other people will make the
same
>> mistake too.
> yes. The name will mislead the people who not look into the code, including
me. :)
I compared the mktime of xen with mktime of linux. The code is almost 
the same, I do not understand why xen requires the input year is 
tm->tm_year, not tm->tm_year+1900. Am I missing somthing?

See following diff file which is created between mktime of linux and 
mktime of xen, (I did some tab/space format changes for comparison)

diff linux-mktime.c xen-mktime.c
2,4c2,4
< mktime(const unsigned int year0, const unsigned int mon0,
<       const unsigned int day, const unsigned int hour,
<       const unsigned int min, const unsigned int sec)
---
 > mktime (unsigned int year, unsigned int mon,
 >       unsigned int day, unsigned int hour,
 >       unsigned int min, unsigned int sec)
6,8c6
<       unsigned int mon = mon0, year = year0;
<
<       /* 1..12 -> 11,12,1..10 */
---
 >       /* 1..12 -> 11,12,1..10: put Feb last since it has a leap day. */
10c8
<               mon += 12;      /* Puts Feb last since it has leap day */
---
 >               mon += 12;
21d18
<

Thanks
Annie
>
> best regards
> yang
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel

On Tue, 2012-02-21 at 02:31 +0000, ANNIE LI wrote:
> 
> On 2012-2-21 7:54, Zhang, Yang Z wrote:
> >> -----Original Message-----
> >> From: young zhang [mailto:young.zhang.free@gmail.com]
> >> Sent: Monday, February 20, 2012 11:04 PM
> >> To: Zhang, Yang Z
> >> Cc: ANNIE LI; xen-devel@lists.xensource.com; Kurt Hackel; Dan
Magenheimer;
> >> Konrad Rzeszutek Wilk
> >> Subject: Re: [Xen-devel] [PATCH] hvm: Correct RTC time offset
update error due
> >> to tm->tm_year
> >>
> >> The mktime which used in xen is different from standard C. I think
the best way is
> >> change it same as normal mktime, or else, other people will make
the same
> >> mistake too.
> > yes. The name will mislead the people who not look into the code,
including me. :)
> 
> I compared the mktime of xen with mktime of linux. The code is almost 
> the same, I do not understand why xen requires the input year is 
> tm->tm_year, not tm->tm_year+1900. Am I missing somthing?
The Linux kernel''s version of mktime and Xen''s version both
differ from
standard C/POSIX defined function (in fact I expect Xen''s is derived
from Linux''s). Not least because they take a bunch of values instead of
a struct tm as arguments (i.e. they take unsigned int year, not
tm->tm_year).

If you wanted to compare Xen vs. a POSIX compliant mktime you''d
probably
want the libc version. The Xen and Linux mktime()s certainly differ
substantially from the eglibc one.

I don''t think you can expect that the in-kernel mktime necessarily has
the same interface as POSIX documents, there is certainly no particular
reason why they should or must (a kernel is not a POSIX environment).
The comment preceding Xen''s mktime makes no mention of offsetting
anything by 1900 (or anything else) and I believe its implementation is
consistent with its defined interface.

Ian.
> See following diff file which is created between mktime of linux and 
> mktime of xen, (I did some tab/space format changes for comparison)
> 
> diff linux-mktime.c xen-mktime.c
> 2,4c2,4
> < mktime(const unsigned int year0, const unsigned int mon0,
> <       const unsigned int day, const unsigned int hour,
> <       const unsigned int min, const unsigned int sec)
> ---
>  > mktime (unsigned int year, unsigned int mon,
>  >       unsigned int day, unsigned int hour,
>  >       unsigned int min, unsigned int sec)
> 6,8c6
> <       unsigned int mon = mon0, year = year0;
> <
> <       /* 1..12 -> 11,12,1..10 */
> ---
>  >       /* 1..12 -> 11,12,1..10: put Feb last since it has a leap
day. */
> 10c8
> <               mon += 12;      /* Puts Feb last since it has leap day
*/
> ---
>  >               mon += 12;
> 21d18
> <
> 
> Thanks
> Annie
> >
> > best regards
> > yang
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xensource.com/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel

Thanks a lot for your reply, Ian.
I guess there is misunderstanding here, see following,
> The Linux kernel''s version of mktime and Xen''s version
both differ from
> standard C/POSIX defined function (in fact I expect Xen''s is
derived
> from Linux''s). Not least because they take a bunch of values
instead of
> a struct tm as arguments (i.e. they take unsigned int year, not
> tm->tm_year).
>   
Yes, Xen''s is same as Linux''s.
> If you wanted to compare Xen vs. a POSIX compliant mktime you''d
probably
> want the libc version. The Xen and Linux mktime()s certainly differ
> substantially from the eglibc one.
>   
Thanks, my original aim is to address an issue in rtc_set_time of 
\xen\arch\x86\hvm\rtc.c. (at least I thought it was, need your 
confirmation :-) )
> I don''t think you can expect that the in-kernel mktime necessarily
has
> the same interface as POSIX documents, there is certainly no particular
> reason why they should or must (a kernel is not a POSIX environment).
> The comment preceding Xen''s mktime makes no mention of offsetting
> anything by 1900 (or anything else) and I believe its implementation is
> consistent with its defined interface.
>   
Yes, the comments does not mention of offsetting anything by 1900, and 
this is the reason why I created the patch.
In \xen\arch\x86\hvm\rtc.c, rtc_set_time call mktime to calculate the 
seconds since 1970/01/01, the input parameters of mktime are required to 
be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, 
min=59, sec=59. (Just like Linux)

However, current xen code has some problem when dealing with 
tm->tm_year, see following,
    tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
    after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
                   tm->tm_hour, tm->tm_min, tm->tm_sec);
(For example, if current time is 2012/12/31, tm->tm_year is 112 here)

To meet the requirement of Xen''s mktime, tm->tm_year should be
changed
to tm->tm_year+1900 when calling mktime in Xen. See following,
    after = mktime(tm->tm_year+1900, tm->tm_mon+1, tm->tm_mday,
                   tm->tm_hour, tm->tm_min, tm->tm_sec);

Thanks,
Annie
> Ian.
>
>   
>> See following diff file which is created between mktime of linux and 
>> mktime of xen, (I did some tab/space format changes for comparison)
>>
>> diff linux-mktime.c xen-mktime.c
>> 2,4c2,4
>> < mktime(const unsigned int year0, const unsigned int mon0,
>> <       const unsigned int day, const unsigned int hour,
>> <       const unsigned int min, const unsigned int sec)
>> ---
>>  > mktime (unsigned int year, unsigned int mon,
>>  >       unsigned int day, unsigned int hour,
>>  >       unsigned int min, unsigned int sec)
>> 6,8c6
>> <       unsigned int mon = mon0, year = year0;
>> <
>> <       /* 1..12 -> 11,12,1..10 */
>> ---
>>  >       /* 1..12 -> 11,12,1..10: put Feb last since it has a
leap day. */
>> 10c8
>> <               mon += 12;      /* Puts Feb last since it has leap
day */
>> ---
>>  >               mon += 12;
>> 21d18
>> <
>>
>> Thanks
>> Annie
>>     
>>> best regards
>>> yang
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xensource.com/xen-devel
>>>       
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xensource.com/xen-devel
>>     
>
>
>

On Wed, 2012-02-22 at 13:10 +0000, annie li wrote:
> Thanks a lot for your reply, Ian.
> I guess there is misunderstanding here, see following,
> > The Linux kernel''s version of mktime and Xen''s
version both differ from
> > standard C/POSIX defined function (in fact I expect Xen''s is
derived
> > from Linux''s). Not least because they take a bunch of values
instead of
> > a struct tm as arguments (i.e. they take unsigned int year, not
> > tm->tm_year).
> >   
> Yes, Xen''s is same as Linux''s.
> > If you wanted to compare Xen vs. a POSIX compliant mktime
you''d probably
> > want the libc version. The Xen and Linux mktime()s certainly differ
> > substantially from the eglibc one.
> >   
> Thanks, my original aim is to address an issue in rtc_set_time of 
> \xen\arch\x86\hvm\rtc.c. (at least I thought it was, need your 
> confirmation :-) )
> > I don''t think you can expect that the in-kernel mktime
necessarily has
> > the same interface as POSIX documents, there is certainly no
particular
> > reason why they should or must (a kernel is not a POSIX environment).
> > The comment preceding Xen''s mktime makes no mention of
offsetting
> > anything by 1900 (or anything else) and I believe its implementation
is
> > consistent with its defined interface.
> >   
> Yes, the comments does not mention of offsetting anything by 1900, and 
> this is the reason why I created the patch.
Oh -- I see now that the original patch is fixing the caller and it was someone
else who introduced this red-herring about the mktime interface itself, sorry.
> In \xen\arch\x86\hvm\rtc.c, rtc_set_time call mktime to calculate the 
> seconds since 1970/01/01, the input parameters of mktime are required to 
> be in normal date format. Such as: year=1980, mon=12, day=31, hour=23, 
> min=59, sec=59. (Just like Linux)
> 
> However, current xen code has some problem when dealing with 
> tm->tm_year, see following,
>     tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
>     after = mktime(tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>                    tm->tm_hour, tm->tm_min, tm->tm_sec);
> (For example, if current time is 2012/12/31, tm->tm_year is 112 here)
> 
> To meet the requirement of Xen''s mktime, tm->tm_year should be
changed
> to tm->tm_year+1900 when calling mktime in Xen. See following,
>     after = mktime(tm->tm_year+1900, tm->tm_mon+1, tm->tm_mday,
>                    tm->tm_hour, tm->tm_min, tm->tm_sec);
Yes, this looks plausible to me (although I''m no expert on this code).
Something similar happens in rtc_next_second. 

Perhaps it would be better to add a function or macro to do the
conversion, such that it is somewhat self documenting? Or at least make
the 1900 a #define with a suitable name.

Ian.

On 2012-2-22 21:58, Ian Campbell wrote:
>
> Yes, this looks plausible to me (although I''m no expert on this
code).
> Something similar happens in rtc_next_second. 
>
> Perhaps it would be better to add a function or macro to do the
> conversion, such that it is somewhat self documenting? Or at least make
> the 1900 a #define with a suitable name.
>   
Sure.
How about
#define epoch_year   1900
#define get_year(x)    (x + epoch_year)
?

Thanks,
Annie
> Ian.
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On 2012-2-22 21:58, Ian Campbell wrote:
>
> Yes, this looks plausible to me (although I''m no expert on this
code).
> Something similar happens in rtc_next_second. 
>
> Perhaps it would be better to add a function or macro to do the
> conversion, such that it is somewhat self documenting? Or at least make
> the 1900 a #define with a suitable name.
>   
I changed the 1900 to a macro and added a new function get_year. The new 
patch is attached.

Thanks
Annie


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 2012-2-23 22:52, annie li wrote:
>
>
> On 2012-2-22 21:58, Ian Campbell wrote:
>>
>> Yes, this looks plausible to me (although I''m no expert on
this code).
>> Something similar happens in rtc_next_second.
>> Perhaps it would be better to add a function or macro to do the
>> conversion, such that it is somewhat self documenting? Or at least make
>> the 1900 a #define with a suitable name.
>>   
> I changed the 1900 to a macro and added a new function get_year. The 
> new patch is attached.
Sorry, two macro define: epoch_year and get_year.

Thanks
Annie

                {