Hello,
first of all, thank you for your comments.
On Thursday 13 of May 2010 15:52:48 you wrote:
> Hi Marek,
>
> On Tue, May 11, 2010 at 12:03:53PM +0200, Marek Otahal wrote:
> > I'd like to send a patch that adds support for pretty printing of current
> > RTC's alarms.
> >
> > The thing it actually does is convert RTC time to local time.
> > To do this (pseudocode):
> > rtc=get rtc alarm's time from rtc dev;
> > time_diff= (-timezone+daylight*3600); -where timezone and daylight are
> > extern constants from time.h;
> > now just add the time_diff to time_t representation of rtc,
>
> ^^^^^^^^
>
> I think this is not correct. The RTC time could be in local time.
>
> This is reason why rtcwake reads /etc/adjtime, see the read_clock_mode()
> function. (Please, see in the main() how rtcwake modifies 'alarm' (-t
> option) variable to convert from local to rtc time.)
The thing is, on some systems (eg. mine) there's no /etc/adjtime so
read_clock_mode() fails and defaults to CM_UTC.
I've changed the time_diff= (-timezone+daylight*3600); to time_diff = sys_time
- rtc_time as used elswhere in the code. My idea is that rtcwake -m show
should ignore the -l/-u params as the user might not know whether rtcwake -m
no -s 600 has actually been called with -l or -u, so I think it'd rather
always return alarm converted to localtime.
I have tested with both HARDWARECLOCK set to localtime and UTC these:
./rtcwake -m no -s 600 -u && ./rtcwake -m show
and
./rtcwake -m no -s 600 -l && ./rtcwake -m show
..and it worked well in all cases.
>
> > int main(int argc, char **argv)
> > {
> >
> > char *devname = DEFAULT_DEVICE;
> >
> > @@ -345,6 +411,7 @@
> >
> > || strcmp(optarg, "no") == 0
> > || strcmp(optarg, "off") == 0
> > || strcmp(optarg, "disable") == 0
> >
> > + || strcmp(optarg, "show") == 0
> >
> > ) {
> >
> > suspend = strdup(optarg);
> > break;
> >
> > @@ -404,6 +471,9 @@
> >
> > }
> >
> > }
> >
> > + if (strcmp(suspend, "show") == 0) {
> > + clock_mode = CM_LOCAL;
> > + }
>
> It would be better to not modify clock_mode and call read_clock_mode()
> and get_basetimes() to get sys_time and rtc_time...
As explained above, I think we should use clock_mode=CM_LOCAL; I just wanted
to move this code to the print_alarm function to keep the code cleaner, but
for some reason, if I add this :
clock_mode = CM_LOCAL;
if (get_basetimes(fd) < 0)
return -1;
to the top of print_alarm function, it doesn't work but does, when the
snip is in main(), can you see why?
>
> > if (clock_mode == CM_AUTO) {
> >
> > if (read_clock_mode() < 0) {
> >
> > printf(_("%s: assuming RTC uses UTC ...\n"),
> >
> > progname);
> > @@ -414,7 +484,7 @@
> >
> > printf(clock_mode == CM_UTC ? _("Using UTC time.\n") :
> > _("Using local time.\n"));
>
> [...]
>
> > +
> > + } else if(strcmp(suspend,"show") == 0) {
> > + if (verbose)
> > + printf(_("suspend mode: show; printing alarm
> > info\n"));
> > + if(print_alarm_info(fd) != 0)
>
> ... and read alarm time (by RTC_WKALM_RD / RTC_ALM_READ) to 'alarm'
> variable and then make the correction (according to diff between
> sys_time and rtc_time) and print the final time (by ctime() or so..).
>
Ok, so should I rename wake to alarm?
Thanks to what you've said, I use the alarm+=sys_time-rtc_time now.
Only a question about ctime(), it would save us a few lines of code, but for
possible parsing of the output by other programs, I thing output of
hh:mm:ss yyyy-mm-dd is better than May 14 2010 13:21:00.
What do you think?
Thank you for your review and I'll send a refactored patch based on your
recommendations soon.
Have a nice weekend,
Marek
> Karel
--
Marek Otahal :o)
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
