On Thu, Jan 16, 2014 at 11:19:08AM +0100, Djalal Harouni wrote: > On Thu, Jan 16, 2014 at 06:01:58AM +0100, Zbigniew Jędrzejewski-Szmek wrote: > > Indeed, in a container (without your patches), sessions remain in > > "closing" state. But with your patches, systemd --user instance is > > started and killed immediately during login. Not good either :) > Ok, ouch! It seems that the systemd --user instance is killed without my patches!
Systemd git will confirm it. Ok, after some thoughts and source code analysis, I came to the conclusion that in order to correctly clean session and user state files, we need: a) Improve session_check_gc() and user_check_gc() by using the session and user states. This implies b) b) Make sure that the state of the session and the user is always correctly set. I'll discuss some notes about a) and follow with patches that will try to honore b). I'm sending this to have some feedback before spending more time on it, ahh yes debugging this beast is hard! Please note that I did not take a look at seats logic, only sessions and users using getty and ssh logins. Please feel free to correct me! a) Improve session_check_gc() and user_check_gc() * user_check_gc() has an "if (u->sessions)" check! this should be updated to check if there are sessions that are not in the "closing" state. If *all* the sessions are in the "closing" state then check each session to see if there are still running processes in the background: screen,... If so then abort the user_check_gc() or depend on a per-session function that will perhaps do the "kill-session-processes". * session_check_gc() should check if the session is in the "closing" state and if there are still running processes in the background, then decide to abort or force the "kill"session-processes" But it should not do the following check: "if (s->scope && manager_unit_is_active(s->manager, s->scope))" Since the scope will always be around, the same was applied in the commit 63966da86 for user_check_gc(), do not check the user manager! b) Make sure that the state of the session and the user is always correctly set. Not to mention that some logind clients depend on it. So to make a) we must honor that b) is stable, and after an extensive source code analysis, it seems that the state of users and sessions is not always correct. I'll list some notes/bugs here: * It seems that in systemd v204 after a logout from a session where processes are still running like "screen" the state files will show: systemd 204 session state file: user state file: Active = 1 State = active State = closing ACTIVE_SESSIONS = X Where for systemd git: session state file: user state file: Active = 0 State = closing State = closing ACTIVE_SESSIONS = Not to mention that for systemd git, the session and user state files will show the same even if there are no background processes and they will not be cleaned (using getty or ssh logins) which is is the object of this thread! This is a bug, not fixed yet. * To track the state of sessions the fifo_fd should be used since the event manager will call session_dispatch_fifo() to remove it on eof. Using the session_is_active() *before* the fifo_fd check is not stable since it will return true before the fifo_fd was even created! and also before the session scope and user serivce are created, the session->seat->active will be set earlier before the fifo_fd creation and will be the last one to be unset after fifo_fd removal. And currently this affects src/login/logind-user.c:user_get_state() logic since it checks session_is_active() before calling session_get_state() which performs the fifo_fd check before the session_is_active() So user_get_state() and session_get_state() results might differ! This is a bug, not fixed yet, user_get_state() should count on session_get_state(). * It seems there are other problems with the session and user states, different variables are used to check the state and later these variables are updated at different places! The following patches will describe the problem and try to make the state more stable. The design was do not add extra flags to 'User' and 'Session' structs unless this is the only way. The logic is already there, and it needs a littel synchronization. Thanks! -- Djalal Harouni http://opendz.org _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
