On Wed, Nov 02, 2011 at 02:42:12AM +0100, Lennart Poettering wrote:
> On Mon, 24.10.11 18:04, Michael Olbrich ([email protected]) 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 |
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
