Hello, forwarding, sorry to push it, I just wasn't sure where the mail ended. Please see mail below.. -- Marek Otahal :o)
--- Begin Message ---
- To: Karel Zak <kzak@xxxxxxxxxx>, util-linux-ng@xxxxxxxxxxxxxxx
- Subject: Fwd: Re: [PATCH] [1/2] util-linux-ng/sys-utils/rtcwake : add print mode for rtc alarm
- From: Marek Otahal <markotahal@xxxxxxxxx>
- Date: Tue, 16 Nov 2010 04:17:41 +0800
- User-agent: KMail/1.13.5 (Linux/2.6.36-ARCH; KDE/4.5.3; i686; ; )
Hello Karel & ML, thank you for reviewing my ideas, coming back after a long time but I managed to rewrite the code according to your suggestions and I find it much cleaner now ;) Change log: Support for 'show' action/mode (rtcwake -m <mode>) * show: print information on current alarms Signed-off-by: Marek Otahal <markotahal@xxxxxxxxx> Regards, Marek Patch: --- util-linux-ng/sys-utils/rtcwake.c 2010-11-15 22:17:40.496666667 +0800 +++ util-linux-ng-mmm/sys-utils/rtcwake.c 2010-11-16 04:07:55.333333334 +0800 @@ -62,8 +62,8 @@ CM_LOCAL }; -static unsigned verbose; -static unsigned dryrun; +static unsigned verbose = 0; +static unsigned dryrun = 0; // 0=no 1=yes enum ClockMode clock_mode = CM_AUTO; static struct option long_options[] = { @@ -298,6 +298,58 @@ return 0; } +/** + * print basic alarm settings + */ +static int print_alarm(int fd) +{ + // get_basetimes() and read_clock_mode() have already been called from main() + + struct rtc_wkalrm wake; + + /* First try the preferred RTC_WKALM_RD */ + if (ioctl(fd, RTC_WKALM_RD, &wake) < 0) { + /* Fall back on the non-preferred way of reading wakeups; only + * works for alarms < 24 hours from now */ + wake.enabled=1; // set this to 1 and determine from value of the year -1 means disabled + if (ioctl(fd, RTC_ALM_READ, &wake.time) < 0) { + perror(_("read rtc alarm\n")); + return -1; + } + } + + if (verbose) + printf(_("alarm: status(on/off) [time(hh:mm:ss) date(yyyy-mm- dd)]\n")); + + if (wake.enabled!=1 || wake.time.tm_year==-1) { // alarm is not set + printf(_("alarm: off\n")); + return 0; + } + + struct rtc_time rtc = wake.time; + struct tm tm; + time_t alarm; + + memset(&tm, 0, sizeof tm); + tm.tm_sec = rtc.tm_sec; + tm.tm_min = rtc.tm_min; + tm.tm_hour = rtc.tm_hour; + tm.tm_mday = rtc.tm_mday; + tm.tm_mon = rtc.tm_mon; + tm.tm_year = rtc.tm_year; + tm.tm_isdst = -1; /* assume the system knows better than the RTC */ + + alarm = mktime(&tm); + if (alarm == (time_t)-1) { + perror(_("convert time failed.\n")); + return -1; + } + + alarm += sys_time - rtc_time; // 0 if both UTC, or expresses diff if RTC in local time + printf(_("alarm: on %s\n"), ctime(&alarm)); + return 0; +} + int main(int argc, char **argv) { char *devname = DEFAULT_DEVICE; @@ -346,6 +398,7 @@ || strcmp(optarg, "no") == 0 || strcmp(optarg, "off") == 0 || strcmp(optarg, "disable") == 0 + || strcmp(optarg, "show") == 0 ) { suspend = strdup(optarg); break; @@ -415,7 +468,7 @@ printf(clock_mode == CM_UTC ? _("Using UTC time.\n") : _("Using local time.\n")); - if (!alarm && !seconds && strcmp(suspend,"disable")) { + if (!alarm && !seconds && strcmp(suspend,"disable") && strcmp(suspend,"show")) { fprintf(stderr, _("%s: must provide wake time\n"), progname); usage(EXIT_FAILURE); } @@ -467,7 +520,7 @@ } else alarm = rtc_time + seconds + 1; - if (setup_alarm(fd, &alarm) < 0) + if (strcmp(suspend,"show")!=0 && setup_alarm(fd, &alarm) < 0) exit(EXIT_FAILURE); printf(_("%s: wakeup from \"%s\" using %s at %s\n"), @@ -479,8 +532,7 @@ if (strcmp(suspend, "no") == 0) { if (verbose) printf(_("suspend mode: no; leaving\n")); - close(fd); - exit(EXIT_SUCCESS); + dryrun=1; // to skip disabling alarm at the end } else if (strcmp(suspend, "off") == 0) { char *arg[4]; @@ -524,6 +576,14 @@ /* just break, alarm gets disabled in the end */ if (verbose) printf(_("suspend mode: disable; disabling alarm\n")); + + } else if(strcmp(suspend,"show") == 0) { + if (verbose) + printf(_("suspend mode: show; printing alarm info\n")); + if(print_alarm(fd) != 0) + rc = EXIT_FAILURE; + dryrun = 1; // don't really disable alarm in the end, just show + } else { if (verbose) printf(_("suspend mode: %s; suspending system\n"), suspend); -- Marek Otahal :o)--- Begin Message ---
- To: Karel Zak <kzak@xxxxxxxxxx>, "util-linux-ng@xxxxxxxxxxxxxxx" <util-linux-ng@xxxxxxxxxxxxxxx>
- Subject: Re: [PATCH] [1/2] util-linux-ng/sys-utils/rtcwake : add print mode for rtc alarm
- From: Marek Otahal <markotahal@xxxxxxxxx>
- Date: Fri, 14 May 2010 16:37:00 +0200
- In-reply-to: <20100513135248.GG2209@xxxxxxxxxxx>
- References: <201005111203.54418.markotahal@xxxxxxxxx> <20100513135248.GG2209@xxxxxxxxxxx>
- User-agent: KMail/1.13.3 (Linux/2.6.32-lts; KDE/4.4.3; i686; ; )
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)
--- End Message ------ util-linux-ng/sys-utils/rtcwake.c 2010-11-15 22:17:40.496666667 +0800 +++ util-linux-ng-mmm/sys-utils/rtcwake.c 2010-11-16 04:07:55.333333334 +0800 @@ -62,8 +62,8 @@ CM_LOCAL }; -static unsigned verbose; -static unsigned dryrun; +static unsigned verbose = 0; +static unsigned dryrun = 0; // 0=no 1=yes enum ClockMode clock_mode = CM_AUTO; static struct option long_options[] = { @@ -298,6 +298,58 @@ return 0; } +/** + * print basic alarm settings + */ +static int print_alarm(int fd) +{ + // get_basetimes() and read_clock_mode() have already been called from main() + + struct rtc_wkalrm wake; + + /* First try the preferred RTC_WKALM_RD */ + if (ioctl(fd, RTC_WKALM_RD, &wake) < 0) { + /* Fall back on the non-preferred way of reading wakeups; only + * works for alarms < 24 hours from now */ + wake.enabled=1; // set this to 1 and determine from value of the year -1 means disabled + if (ioctl(fd, RTC_ALM_READ, &wake.time) < 0) { + perror(_("read rtc alarm\n")); + return -1; + } + } + + if (verbose) + printf(_("alarm: status(on/off) [time(hh:mm:ss) date(yyyy-mm-dd)]\n")); + + if (wake.enabled!=1 || wake.time.tm_year==-1) { // alarm is not set + printf(_("alarm: off\n")); + return 0; + } + + struct rtc_time rtc = wake.time; + struct tm tm; + time_t alarm; + + memset(&tm, 0, sizeof tm); + tm.tm_sec = rtc.tm_sec; + tm.tm_min = rtc.tm_min; + tm.tm_hour = rtc.tm_hour; + tm.tm_mday = rtc.tm_mday; + tm.tm_mon = rtc.tm_mon; + tm.tm_year = rtc.tm_year; + tm.tm_isdst = -1; /* assume the system knows better than the RTC */ + + alarm = mktime(&tm); + if (alarm == (time_t)-1) { + perror(_("convert time failed.\n")); + return -1; + } + + alarm += sys_time - rtc_time; // 0 if both UTC, or expresses diff if RTC in local time + printf(_("alarm: on %s\n"), ctime(&alarm)); + return 0; +} + int main(int argc, char **argv) { char *devname = DEFAULT_DEVICE; @@ -346,6 +398,7 @@ || strcmp(optarg, "no") == 0 || strcmp(optarg, "off") == 0 || strcmp(optarg, "disable") == 0 + || strcmp(optarg, "show") == 0 ) { suspend = strdup(optarg); break; @@ -415,7 +468,7 @@ printf(clock_mode == CM_UTC ? _("Using UTC time.\n") : _("Using local time.\n")); - if (!alarm && !seconds && strcmp(suspend,"disable")) { + if (!alarm && !seconds && strcmp(suspend,"disable") && strcmp(suspend,"show")) { fprintf(stderr, _("%s: must provide wake time\n"), progname); usage(EXIT_FAILURE); } @@ -467,7 +520,7 @@ } else alarm = rtc_time + seconds + 1; - if (setup_alarm(fd, &alarm) < 0) + if (strcmp(suspend,"show")!=0 && setup_alarm(fd, &alarm) < 0) exit(EXIT_FAILURE); printf(_("%s: wakeup from \"%s\" using %s at %s\n"), @@ -479,8 +532,7 @@ if (strcmp(suspend, "no") == 0) { if (verbose) printf(_("suspend mode: no; leaving\n")); - close(fd); - exit(EXIT_SUCCESS); + dryrun=1; // to skip disabling alarm at the end } else if (strcmp(suspend, "off") == 0) { char *arg[4]; @@ -524,6 +576,14 @@ /* just break, alarm gets disabled in the end */ if (verbose) printf(_("suspend mode: disable; disabling alarm\n")); + + } else if(strcmp(suspend,"show") == 0) { + if (verbose) + printf(_("suspend mode: show; printing alarm info\n")); + if(print_alarm(fd) != 0) + rc = EXIT_FAILURE; + dryrun = 1; // don't really disable alarm in the end, just show + } else { if (verbose) printf(_("suspend mode: %s; suspending system\n"), suspend);
--- End Message ---
Attachment:
signature.asc
Description: This is a digitally signed message part.
