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

[Lennart Poettering]

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)

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

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?

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

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

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

-- 
Lennart Poettering - Red Hat, Inc.