[PATCH V2] watchdog add settimeout ioctl

Denys Vlasenko vda.linux at googlemail.com
Tue Aug 26 01:12:43 UTC 2008


Hi Darius,

Sorry for the delay. I know it is frustrating to not have
a response, and I don't even have a good excuse :(

I reviewed past threads about this patch and no-one asked
why it is needed. From the patch it is not really obvious.

Please read on. I afraid the patch will need one more
review iteration.

On Wednesday 20 August 2008 08:49, Darius wrote:
> Fixed definitions.

patch-busybox-watchdog-timeout
  Index: Busybox/include/usage.h
===================================================================
--- Busybox.orig/include/usage.h
+++ Busybox/include/usage.h
@@ -2661,6 +2661,7 @@
        "       [a]sync         Writes are asynchronous / synchronous\n" \
        "       [no]atime       Disable / enable updates to inode access times\n" \
        "       [no]diratime    Disable / enable atime updates to directories\n" \
+       "       [no]relatime    Disable / enable atime updates relative to modification time\n" \

These unrelated edits seem to be not needed now.
In any case, unrelated edits should be in a separate patch.


@@ -4497,11 +4498,12 @@
        "Mon Dec 17 10:31:44 GMT 2000"
 
 #define watchdog_trivial_usage \
-       "[-t N[ms]] [-F] DEV"
+       "[-t N[ms]] [-T] [-F] DEV"

Perhaps "[-T N[ms]]" ?

 #define watchdog_full_usage "\n\n" \
        "Periodically write to watchdog device DEV\n" \
      "\nOptions:" \
-     "\n       -t N    Timer period (default 30)" \
+     "\n       -t N    Userspace timer period (default 30)" \
+     "\n       -T N    Hardware WD timer period (default 60)" \

Let's try to make it more understandable. Think about user
whic sees it after "watchdog --help". What is the difference
between -t and -T? Maybe:

-T N   Set reboot delay to N seconds
-t N   Restart countdown to reboot every N seconds

Is this correct?


-       opts = getopt32(argv, "Ft:", &t_arg);
+       opts = getopt32(argv, "Ft:T:", &st_arg, &ht_arg);
 
-       if (opts & OPT_TIMER) {
+       if (opts & OPT_STIMER) {
                static const struct suffix_mult suffixes[] = {
                        { "ms", 1 },
                        { "", 1000 },
                        { }
                };
-               timer_duration = xatou_sfx(t_arg, suffixes);
+               stimer_duration = xatou_sfx(st_arg, suffixes);
        }

-       if (!(opts & OPT_FOREGROUND)) {
-               bb_daemonize_or_rexec(DAEMON_CHDIR_ROOT, argv);
-       }
+       if (opts & OPT_HTIMER)
+               htimer_duration = xatou16(ht_arg);

Should we allow milliseconds here too?


+       ioctl_or_warn(3, WDIOC_SETTIMEOUT, &htimer_duration);
+       ioctl_or_warn(3, WDIOC_GETTIMEOUT, &htimer_duration);

Aha. Now you are setting the -T duration. Old code was just assuming
it is set correctly. This had to be written in patch description
from the start.

+       printf("watchdog: SW timer is %dms, HW timer is %dms\n",
+               stimer_duration, htimer_duration * 1000);

Printing of this message seems to be an additional change in behavior.
Why? Did you find it very useful? (Again, these things are usually
explained in patch comments)

Looking forward for next iteration of the patch.
--
vda



More information about the busybox mailing list