[systemd-devel] [PATCH] logind: just call user_stop() if user_check_gc() returns false

[Lennart Poettering]

On Sat, 08.02.14 19:20, Djalal Harouni (tixxdz at opendz.org) wrote:

> Currently if the user logs out, the GC may never call user_stop(),
> this will not terminate the systemd user and (sd-pam) of that user.
> 
> To fix this, remove the USER_CLOSING state check that is blocking the
> GC from calling user_stop(). We do not need it since with the current
> logic we have:
> 
> 1) if user_check_gc() returns false this means that all the sessions of
> the user were removed.
> 
> 2) if all the sessions were removed and if linger is not set and if it
> is not open state then user_get_state() will return always USER_CLOSING.
> 
> So that check will never be satisfied for normal cases, and for linger
> user_check_gc() will return true which prevents the GC from collecting
> the user.

Hmm, this doesn't look right... We really shouldn't finalize the user
object as long as the stop job for the user at .service unit is still
running. This is why we have this weird two-step GC logic here: 

First we check whether the user is still needed. If it is, then we'll
issue the stop job. However, this action means that the user is needed
again, as long as the stop job is not finished. Hence we then wait for
that, and then recheck the thing in the GC. This time we are completely
dead, and can remove the thing entirely. Your patch would break that,
no?

> ---
>  src/login/logind.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/src/login/logind.c b/src/login/logind.c
> index a6f84e8..84c5b7d 100644
> --- a/src/login/logind.c
> +++ b/src/login/logind.c
> @@ -889,11 +889,8 @@ void manager_gc(Manager *m, bool drop_not_started) {
>                  LIST_REMOVE(gc_queue, m->user_gc_queue, user);
>                  user->in_gc_queue = false;
>  
> -                if (!user_check_gc(user, drop_not_started) &&
> -                    user_get_state(user) != USER_CLOSING)
> -                        user_stop(user);
> -
>                  if (!user_check_gc(user, drop_not_started)) {
> +                        user_stop(user);
>                          user_finalize(user);
>                          user_free(user);
>                  }


Lennart

-- 
Lennart Poettering, Red Hat