[systemd-devel] [PATCH v2 3/3] WIP: service: add StartLimitInterval/StartLimitBurst/StartLimitAction

[Michael Olbrich]

On Wed, Feb 08, 2012 at 09:41:10PM +0100, Lennart Poettering wrote:
> On Wed, 08.02.12 10:10, Michael Olbrich (m.olbrich at pengutronix.de) wrote:
> 
> > Signed-off-by: Michael Olbrich <m.olbrich at pengutronix.de>
> > ---
> > changes in v2:
> >  - Directly set ratelimit.interval/ratelimit.burst
> >    There are some issues with that:
> >    The names are now inconsistent: ratelimit vs. start_limit, maybe we
> >    should call the variable ratelimit_action?
> 
> Hmm, ratelimiting is the generic term, so I'd rather not expose that
> name externally, since we might need it later on. For example, it might
> make sense one day to add a logging ratelimiter to the mix, where we'd
> have yet another ratelimiter.
> 
> Hence I'd propose to rename the internal variable to start_limit or so.

ok.

> >    setting StartLimitBurst=0 will trigger an assert. Where would I add
> >    range checking?
> 
> Hmm, I figure if interval or burst is 0 we should implicitly disable the
> ratelimiter, to give the user a nice way to disable the ratelimiting if
> he really wants. I have now changed ratelimit.c accordingly. Please
> check if this does what is needed.

Looks good.

> >          /* Make sure we don't enter a busy loop of some kind. */
> >          if (!ratelimit_test(&s->ratelimit)) {
> > -                log_warning("%s start request repeated too quickly, refusing to start.", u->id);
> > +                switch (s->start_limit_action) {
> > +                case SERVICE_START_LIMIT_NONE:
> > +                        log_warning("%s start request repeated too quickly, refusing to start.", u->id);
> > +                        break;
> > +                case SERVICE_START_LIMIT_REBOOT: {
> > +                        DBusError error;
> > +                        int r;
> > +
> > +                        log_warning("%s start request repeated too quickly, rebooting.", u->id);
> > +                        dbus_error_init(&error);
> > +                        r = manager_add_job_by_name(u->manager, JOB_START, SPECIAL_REBOOT_TARGET, JOB_REPLACE, true, &error, NULL);
> > +                        if (r < 0)
> > +                                log_error("Failed to reboot: %s.", bus_error(&error, r));
> > +                        break;
> > +                }
> 
> I think a fallback on reboot-force would make sense here on
> failure. i.e. don't have the "break" here on failure of
> manager_add_job_by_name(). But please add a comment /* fallthrough */ so
> that this is easy to see.

Hmmm, I considered this. However, if I mask reboot.target (e.g. to prevent
reboots during system update or something like that). Then we fail here
with:
Failed to reboot: Unit reboot.target is masked..

I'm not sure if forcing a reboot anyways is a good idea in this case.

> > +                case SERVICE_START_LIMIT_REBOOT_FORCE:
> > +                        log_warning("%s start request repeated too quickly, force rebooting.", u->id);
> > +                        u->manager->exit_code = MANAGER_REBOOT;
> > +                        break;
> > +                case SERVICE_START_LIMIT_REBOOT_IMMEDIATE:
> > +                        reboot(RB_AUTOBOOT);
> 
> Please add a message here too. People on serial terminals still might
> want to know why their machine is suddenly rebooting and they couzld
> still read the message since serial terminals are not flushed at reboot.

Ah, yes, with log-target == console I can actually see a message there.

> > +                        break;
> > +                default:
> > +                        log_error("start limit action=%i", s->start_limit_action);
> > +                        assert_not_reached("Unknown StartLimitAction.");
> > +                }
> >                  return -ECANCELED;
> 
> Could you move all of this into it's own function please?

Ok.

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 |