On Fri, Apr 11, 2014 at 03:34:42AM +0200, Lennart Poettering wrote:
> 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.
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 |
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
