[systemd-devel] [PATCH] service: allow service to inhibit respawn with special return code

[Lennart Poettering]

On Mon, 06.08.12 16:45, Lukáš Nykrýn (lnykryn at redhat.com) wrote:

> +        if (!*set) {
> +                set_free(*set);
> +                *set = NULL;
> +        }

You probably mean if (*set) here, not (!*set). But honestly I think this
bit should just go entirely as for most other settings we actually do
allow adding in additional values later on if it makes sense (example:
Environment=). 

> +
> +        *set = set_new(trivial_hash_func, trivial_compare_func);
> +        if (!*set)
> +                return -ENOMEM;

We probably should start using log_oom() for things like this now that
it is available.

> +
> +        FOREACH_WORD(w, l, rvalue, state)
> +        {
> +                char *temp = new0(char, l + 1);
> +                if (!temp)
> +                        return -ENOMEM;

Please use strndup() here! (And also log_oom() here please).

> +
> +                strncpy(temp, w, l);
> +                r = safe_atoi(temp, &val);
> +                free(temp);
> +                if (r < 0) {
> +                        log_error("[%s:%u] Failed to parse numeric value: %s", filename, line, w);
> +                        set_free(*set);
> +                        *set = NULL;

I'd leave the hashmap set, as it is freed anyway when the service goes
away, and makes simpler and easier to read.

> +                        return r;
> +                }
> +                r = set_put(*set, INT_TO_PTR(val));
> +                if (r < 0) {
> +                        log_error("[%s:%u] Unable to store: %s", filename, line, w);
> +                        set_free(*set);
> +                        *set = NULL;

Same here.

Otherwise looks good.

(We want the signal stuff eventually I guess, but we can easily add this later).

Lennart

-- 
Lennart Poettering - Red Hat, Inc.