Hi Bernhard,
good work, but I found few problems:
On Fri, Jul 13, 2007 at 11:54:15AM +0200, Bernhard Walle wrote:
> +.SH AVAILABILITY
> +The cal command is part of the util-linux-ng package and is available from
s/cal/rtcwake/ ;-)
> + /* many rtc alarms only support up to 24 hours from 'now' ... */
> + if ((rtc_time + (24 * 60 * 60)) > *wakeup) {
> + if (ioctl(fd, RTC_ALM_SET, &wake.time) < 0) {
> + perror("set rtc alarm");
> + return 0;
> + }
> + if (ioctl(fd, RTC_AIE_ON, 0) < 0) {
> + perror("enable rtc alarm");
> + return 0;
> + }
> +
> + /* ... so use the "more than 24 hours" request only if we must */
> + } else {
> + /* avoid an extra AIE_ON call */
> + wake.enabled = 1;
> +
> + if (ioctl(fd, RTC_WKALM_SET, &wake) < 0) {
> + perror("set rtc wake alarm");
> + return 0;
> + }
> + }
What is different between wake.enabled=1 and RTC_AIE_ON?
> +static void usage(void)
> +{
> + printf("usage: %s [options]\n"
> + "\t-d rtc0|rtc1|...\t(select rtc)\n"
> + "\t-l\t\t\t(RTC uses local timezone)\n"
> + "\t-m standby|mem|...\t(sleep mode)\n"
> + "\t-s seconds\t\t(seconds to sleep)\n"
> + "\t-t time_t\t\t(time to wake)\n"
> + "\t-u\t\t\t(RTC uses UTC)\n"
> + "\t-v\t\t\t(verbose messages)\n"
> + "\t-V\t\t\t(show version)\n", progname);
exit(EXIT_FAILURE);
> +}
This \t mess is really not too much readable...
> +static int read_clock_mode(void)
> +{
> + FILE *fp;
> + char linebuf[MAX_LINE];
> +
> + fp = fopen(ADJTIME_PATH, "r");
> + if (!fp)
> + return 0;
> +
....
fclose(fp);
> + return 1;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + static char *devname = "rtc0";
const char *devname = "/dev/rtc0";
> + static unsigned seconds = 0;
> + static char *suspend = "standby";
You needn't static.
> + int t;
> + int fd;
> + time_t alarm = 0;
> +
> + progname = strrchr(argv[0], '/');
> + if (progname)
> + progname++;
> + else
> + progname = argv[0];
> + if (chdir("/dev/") < 0) {
Please, no.. why we cannot use absolute paths? I think
# rtcwake --device /dev/rtc0
is not so bad.
....
> + printf("%s: unrecognized suspend state '%s'\n",
> + progname, optarg);
> + usage();
> + return 1;
I think better is an exit() call inside usage().
> +
> + /* alarm time, seconds-to-sleep (relative) */
> + case 's':
> + t = atoi(optarg);
> + if (t < 0) {
> + printf("%s: illegal interval %s seconds\n",
> + progname, optarg);
Please, fprintf(stderr, ...) for all error messages.
> + case 'V':
> + printf("%s: version %s\n", progname, VERSION_STRING);
> + return 0;
exit(EXIT_SUCCESS);
> +
> + default:
> + usage();
> + return 1;
> + }
> + }
> +
> + if (clock_mode == CM_AUTO) {
> + if (!read_clock_mode()) {
> + printf("%s: assuming RTC uses UTC ...\n", progname);
> + clock_mode = CM_UTC;
> + }
> + if (verbose)
> + fprintf(stderr, "Using %s time\n",
> + clock_mode == CM_UTC ? "UTC" : "local");
The verbose messages go to stdout or stderr or both? :-) Please, use
stdout.
> + /* this RTC must exist and (if we'll sleep) be wakeup-enabled */
> + fd = open(devname, O_RDONLY);
> + if (fd < 0) {
> + perror(devname);
> + return 1;
> + }
> + if (strcmp(suspend, "on") != 0 && !may_wakeup(devname)) {
> + printf("%s: %s not enabled for wakeup events\n",
fprintf(stderr, ....)
> + progname, devname);
> + return 1;
> + }
Maybe you can open the device after the may_wakeup(devname) check.
I look forward to a new (final) version :-) Thanks for your time.
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
-
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
