On Wed, 26.03.14 10:02, Michael Olbrich (m.olbr...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
