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

[Lukáš Nykrýn]

Lennart Poettering píše v Po 13. 08. 2012 v 17:00 +0200:
> On Mon, 13.08.12 14:10, Lukáš Nykrýn (lnykryn at redhat.com) wrote:
> 
> > -        if (UNIT(s)->fragment_path ? is_clean_exit(code, status) : is_clean_exit_lsb(code, status))
> > +        if (UNIT(s)->fragment_path ? is_clean_exit(code, status, &s->successful_status) :
> > +                                     is_clean_exit_lsb(code, status,
> > &s->successful_status))
> 
> I think we should get rid of is_clean_exit_lsb() now probably, and for
> sysv services just implicitly fill the exit code set with the valid LSB
> exit codes. That way we can reduce the SysV specific service handling
> code a bit more to service.c. (but don't bother with this now, this
> should probably be fixed in a separate, other patch)
I still don't like that we have hard-coded exit statuses which are
considered as success for lsb (but I understand why it was done so). I
am not sure if it is a good idea, but can't we pass this problem to a
maintainer by adding possibility to set these in lsb header? (The same
problem can be with hardcoded timeout for lsb).
> 
> >                  f = SERVICE_SUCCESS;
> >          else if (code == CLD_EXITED)
> >                  f = SERVICE_FAILURE_EXIT_CODE;
> > diff --git a/src/core/service.h b/src/core/service.h
> > index c78de79..98b9935 100644
> > --- a/src/core/service.h
> > +++ b/src/core/service.h
> > @@ -28,6 +28,7 @@ typedef struct Service Service;
> >  #include "ratelimit.h"
> >  #include "service.h"
> >  #include "kill.h"
> > +#include "exit-status.h"
> >  
> >  typedef enum ServiceState {
> >          SERVICE_DEAD,
> > @@ -115,6 +116,8 @@ struct Service {
> >  
> >          ServiceType type;
> >          ServiceRestart restart;
> > +        ExitStatuses restart_ignore_status;
> > +        ExitStatuses successful_status;
> 
> I don't like the name of the type. The plural "Statuses" is so
> weird. Maybe "ExitStatusSets" is better? Also, that the type is in
> plural but the variable in singular is weird, no? 
Thanks! I didn't want to call it ExitStatus since there is
exit-status.c, but I wasn't able to find any sane name.
> 
> Also, we never use the adjective "sucessful" anywhere, we always use the
> term "success" instead. Please use "success_status_set" or so as
> variable name here.
> 
> > +        if(!statuses->code){
> > +                statuses->code = set_new(trivial_hash_func, trivial_compare_func);
> > +                if (!statuses->code)
> > +                        return log_oom();
> > +        }
> 
> Wouldn't it be nicer if we only instantiated that array if we actually
> really need it? i.e. when we try to add an entry to this specific set?
Fixed
> 
> > +
> > +        if(!statuses->signal){
> > +                statuses->signal = set_new(trivial_hash_func, trivial_compare_func);
> > +                if (!statuses->signal)
> > +                        return log_oom();
> > +        }
> > +
> > +        FOREACH_WORD(w, l, rvalue, state)
> > +        {
> 
> Please place the { at the end of the FOREACH_WORD line.
Fixed
> 
> > +                int val;
> > +                char *temp = strndup(w, l);
> > +                if (!temp)
> > +                        return log_oom();
> > +
> > +                r = safe_atoi(temp, &val);
> 
> Hmm, we probably should be more careful here with validating the passed
> parameters. Even though the process exit status is of type "int" it
> cannot be negative, and is 8 bit only (i.e. < 256), we should check for
> that and warn the user if it is out of range and skip it.
> 
> > +                return status == 0 ||
> > +                       (successful_status &&
> > +                       set_contains(successful_status->code,
> > INT_TO_PTR(status)));
> You can actually shorten this as set_get()/set_contains() should handle
> a NULL set as the empty set.
I am not quite sure what you meant here. If you were talking about
'successful_status &&' part, it musts stay, because
successful_status->code would result to segfault. 
> 
> >  
> >          /* If a daemon does not implement handlers for some of the
> >           * signals that's not considered an unclean shutdown */
> > @@ -170,14 +174,16 @@ bool is_clean_exit(int code, int status) {
> >                          status == SIGHUP ||
> >                          status == SIGINT ||
> >                          status == SIGTERM ||
> > -                        status == SIGPIPE;
> > +                        status == SIGPIPE ||
> > +                        (successful_status &&
> > +                        set_contains(successful_status->signal, INT_TO_PTR(status)));
> >  
> 
> Same here.
> 
> Otherwise looks good.
> 
> Lennart
> 

I hope that I did not forget anything.

Lukas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-service-add-options-RestartPreventExitStatus-and-Suc.patch
Type: text/x-patch
Size: 19612 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20120814/303d4765/attachment-0001.bin>