[systemd-devel] [PATCH 2/4] WIP: service: add watchdog restart/reboot timeouts

[Michael Olbrich]

On Wed, Nov 02, 2011 at 02:42:12AM +0100, Lennart Poettering wrote:
> On Mon, 24.10.11 18:04, Michael Olbrich (m.olbrich at pengutronix.de) wrote:
> 
> >                          <varlistentry>
> > +
> > <term><varname>WatchdogRestartUSec=</varname></term>
> 
> Hmm, so there's a bit of an asymmetry in systemd here. While the bus
> properties usually use "USec" as unit, since they are just 64bit
> integers, the configuration settings in the files are actually all
> suffixed "Sec", since they all parse unit-less values as seconds. It's a
> bit weird, but should clarify that WatchdogRestartSec=5 means 5s, while
> still accepting WatchdogRestartSec=50ms -- which will be 50ms, even if
> the switch is a bit surprising there.
> 
> Anyway, this might be a bit surprising in general, but it does make some
> sense. And hence I'd like to ask you to use Sec=, not USec= as suffix
> for your settings, especially since you use config_parse_usec() like the
> other timeouts.

I just messed up the documentation, right? It's sec in the service file and
usec in the D-Bus API. I copied most of it from Timeout[U]Sec.

> > +                                <listitem><para>Configures the time to
> > +                                wait before restarting a service. This
> > +                                is activated with the first
> > +                                <citerefentry><refentrytitle>sd_notify</refentrytitle><manvolnum>3</manvolnum></citerefentry>
> > +                                call with "WATCHDOG=1". If the time
> > +                                between two such calls is larger than
> > +                                the configured time then the service
> > +                                is restarted.</para></listitem>
> > +                        </varlistentry>
> 
> Please mention that this defaults to 0, which means no watchdog enabled.

ok.

> >          dual_timestamp_get(&s->watchdog_timestamp);
> > +        if (s->watchdog_restart_usec) {
> > +                if ((r = unit_watch_timer(UNIT(s), s->watchdog_restart_usec, &s->watchdog_restart_watch)) < 0)
> > +                        log_warning("%s failed to install watchdog restart timer: %s", s->meta.id, strerror(-r));
> > +        }
> > +        if (s->watchdog_restart_usec) {
> > +                if ((r = unit_watch_timer(UNIT(s), s->watchdog_reboot_usec, &s->watchdog_reboot_watch)) < 0)
> > +                        log_warning("%s failed to install watchdog reboot timer: %s", s->meta.id, strerror(-r));
> > +        }
> >  }
> 
> Hmm, please change this syntax:
> 
> if ((r = foo()) < 0) { ...
> 
> to this:
> 
> r = foo();
> if (r < 0) { ...
> 
> I started systemd using the first syntax, but in order to come closer to
> the kernel coding style all new code should use the second syntax.

ok.

> Otherwise looks good.

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |