[systemd-devel] [RFC PATCH] service: add FailureAction= option

[Michael Olbrich]

On Fri, Apr 11, 2014 at 03:34:42AM +0200, Lennart Poettering wrote:
> On Wed, 26.03.14 10:02, Michael Olbrich (m.olbrich at pengutronix.de) wrote:
> 
> > It has the same possible values as StartLimitAction= and is executed
> > immediately if a service fails.
> 
> I think the enum type should probably be renamed to FailureAction, since
> that now sounds like the more generic name. 

Ok.

> > ---
> > 
> > Hi Lennart,
> > 
> > Something like this maybe? I'm not quite sure about the condition in
> > service_enter_dead(). I don't think the action should be executed when the
> > service is explicitly stopped. Maybe it should depend on !forbid_restart?
> > 
> > If you like, I'll add some documentation. An maybe a follow-up patch to
> > rename the StartLimitAction type? To what though?
> 
> > index ae3695a..ab161a5 100644
> > --- a/src/core/service.c
> > +++ b/src/core/service.c
> > @@ -1835,6 +1835,8 @@ static int cgroup_good(Service *s) {
> >          return !r;
> >  }
> >  
> > +static int service_execute_action(Service *s, StartLimitAction action, const char *reason);
> > +
> >  static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) {
> >          int r;
> >          assert(s);
> > @@ -1844,7 +1846,9 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
> >  
> >          service_set_state(s, s->result != SERVICE_SUCCESS ? SERVICE_FAILED : SERVICE_DEAD);
> >  
> > -        if (allow_restart &&
> > +        if (s->result != SERVICE_SUCCESS && s->failure_action != SERVICE_START_LIMIT_NONE)
> > +                service_execute_action(s, s->failure_action,
> > "failed");
> 
> I'd prefer to move the check for SERVICE_START_LIMIT_NONE to the top of service_execute_action().

Hmmm, the check is already there, but for the start limit it produces a
warning. I'll see if I can find a nice way to handle this.

> > +        else if (allow_restart &&
> 
> I would drop the "else" here, I think. Is there a reason not to do the
> restart thing anyway? If it is configured, it should run I think, just
> in case the failure action doesn't work or so...

Are you sure? With Restart=always and FailureAction=reboot this would
try to start the unit while shutting down. Will the Conflicts= from the
default dependencies handle this correctly?

Regards,
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 |