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

[Lennart Poettering]

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. 

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

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

Otherwise looks pretty good.

Lennart

-- 
Lennart Poettering, Red Hat