[systemd-devel] [PATCH] service: don't try to kill the service more than once when the watchdog timeout hits

[Lennart Poettering]

On Wed, 17.07.13 09:20, Michael Olbrich (m.olbrich at pengutronix.de) wrote:

> Hi,
> 
> On Wed, Jul 17, 2013 at 03:53:09AM +0200, Lennart Poettering wrote:
> > On Wed, 12.06.13 01:22, Michael Olbrich (m.olbrich at pengutronix.de) wrote:
> > 
> > > If ExecStopPost= is defined then it is executed after SIGKILL. Otherwise
> > > another round of SIGTERM/SIGSTOP is started which is rather useless when
> > > the watchdog timeout hits.
> > > So go directly to the final SIGKILL if ExecStopPost= is not defined.
> > 
> > Hmm, why not go always directly into SERVICE_FINAL_SIGKILL? Why bother
> > with SERVICE_STOP_SIGKILL at all? What am I missing?
> 
> I think a small summery is necessary here:
> The original watchdog code just called service_enter_dead(). This had the
> problem that no cleanup happened. The process was not killed.
> Then I posted a patch to go irectly into SERVICE_FINAL_SIGKILL to fix that.
> It got applied.
> Then you changed it to SERVICE_STOP_SIGKILL so that ExecStopPost= is still
> called.
> However, this has a annoying side effect. Consider a service that enters
> service_handle_watchdog() with a process that cannot be killed. What
> happens is this:
> 
> service_handle_watchdog()
> STOP_SIGKILL
> wait
> ExecStopPost= if available
> FINAL_SIGTERM
> wait
> FINAL_SIGKILL
> wait
> service_enter_dead()
> 
> Without a ExecStopPost= defined anything after the first wait is just more
> useless waiting.
> I wrote this patch to avoid waiting longer than necessary but still be able
> to run ExecStopPost=.
> I later realized that this problem is not limited to the watchdog usecase.
> So I wrote a second patch (it should be applied instead of this one) that
> changes the sequence to:
> 
> [...]
> STOP_SIGKILL
> wait
> if (ExecStopPost= is available) {
> 	ExecStopPost=
> 	FINAL_SIGTERM
> 	wait
> 	FINAL_SIGKILL
> 	wait
> }
> service_enter_dead()
> 
> I sent it as a reply to the first patch. I've also attached it for
> reference.

Ah, thanks! Sorry for the confusion and the way too long review time for
the patch.

Committed the second patch now. Thanks!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.