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

[Lennart Poettering]

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.

> +                                <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.

>          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.

Otherwise looks good.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.