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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.



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