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 ---
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);
