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

[Lennart Poettering]

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.

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

>          /* 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.

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

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

Otherwise looks good!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.