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

[Lukáš Nykrýn]

Lennart Poettering píše v Po 06. 08. 2012 v 16:52 +0200:
> 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
> 
Again thanks for review. Here is modified patch.
If you think that it would be better to add signal stuff before
accepting this patch I will not disagree.

Lukas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-service-allow-service-to-inhibit-respawn-with-specia.patch
Type: text/x-patch
Size: 7413 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20120806/50529911/attachment.bin>