Currently the user and session states are not stable: 1) session state: To get the session state the function session_get_state() is used.
Opening state: At login the D-Bus CreateSession() method will call session_start() which calls user_start() and session_start_scope() to queue the scope job and set the session->scope_job => session_get_state() == SESSION_OPENING (correct) Then execution will continue from session_send_create_reply() which is called by the D-Bus match_job_removed() signal. math_job_removed() will check if this is the session scope and if this is the previously queued scope job and if so it will clear the session->scope_job => session_get_state() == SESSION_CLOSING (incorrect) (session closing since fifo_fd == -1) Then scope is created successfully, call session_send_create_reply() which will wait for the session scope *and* the user service to be successfully created. /* user service is still pending */ if (s->scope_job || s->user->service_job)) return 0 => session_get_state() == SESSION_CLOSING (incorrect) else session_create_fifo() /* fifo_fd finally created */ => session_get_state() == SESSION_ACTIVE (correct) 2) user state: To get the user state the function user_get_state() is used. I'll add that the user_get_state() and session_get_state() do not have the same logic when it comes to sessions, this will just add ambiguity. user_get_state() calls session_is_active() before checking session_get_state(), and session_is_active() will return true right from the start since the logic is set during D-Bus CreateSession(). Opening state: At login we have session_start() which calls user_start() here we already: => user_get_state() == USER_ACTIVE (incorrect) (not fixed in this patch) At user_start() user_start_slice() queue the slice and set user->slice_job user_start_service() queue the service and set user->service_job => user_get_state() == USER_OPENING (correct) Then execution will continue from session_send_create_reply() which is called by the D-Bus match_job_removed() signal. math_job_removed() will check if these are the user service and slice and if they are the previously queued jobs, if so it will clear the: user->service_job and user->slice_job => user_get_state() == USER_ACTIVE (incorrect) (incorrect since the fifo_fd has not been created) Then service is created successfully, call session_send_create_reply() which will wait for the session scope *and* the user service to be successfully created. If so then session_send_create_reply() will continue and call session_create_fifo() => user_get_state() == USER_ACTIVE (correct) (fifo_fd was created) Fix: To fix the session state and remove the two incorrect SESSION_CLOSING, we do not clear up the "session->scope_job" when we detect that this is the session scope, we setup a temporary variable "scope_job" that will get the current value of "session->scope_job" and update it if necessary. Add a new "opening" variable to check if the session scope and user service were successfully created. Update the session_send_create_reply() function to receive the "opening" variable as a third bool parameter and adapt its logic. Then clear the "session->scope_job" when session_send_create_reply() is finished, this ensures that the state will just go from: "SESSION_OPENING" => "SESSION_ACTIVE" The same goes for the user state, in order to remove the incorrect state when the fifo_fd is not created but the state shows USER_ACTIVE, we do not clear the "user->service_job" and "user->slice_job" right away, we store the state in a temporary variable "service_job" and update it later if we detect that this is the user service. Add a new "opening" variable to check if the session scope and user service were successfully created and pass it to session_send_create_reply(). --- src/login/logind-dbus.c | 56 ++++++++++++++++++++++++++++------------- src/login/logind-session-dbus.c | 8 +++--- src/login/logind-session.h | 2 +- 3 files changed, 45 insertions(+), 21 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 2c86b9f..0123a34 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1937,54 +1937,76 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b session = hashmap_get(m->session_units, unit); if (session) { + bool opening, scope_job = !!session->scope_job; - if (streq_ptr(path, session->scope_job)) { - free(session->scope_job); - session->scope_job = NULL; - } + /* Set to false if the session scope was created */ + if (streq_ptr(path, session->scope_job)) + scope_job = false; + + /* If the session scope and the user service are still + * in the creation process this will be set to true, + * otherwise it will be false */ + opening = scope_job || !!session->user->service_job; if (session->started) { if (streq(result, "done")) - session_send_create_reply(session, NULL); + session_send_create_reply(session, NULL, opening); else { _cleanup_bus_error_free_ sd_bus_error e = SD_BUS_ERROR_NULL; sd_bus_error_setf(&e, BUS_ERROR_JOB_FAILED, "Start job for unit %s failed with '%s'", unit, result); - session_send_create_reply(session, &e); + session_send_create_reply(session, &e, opening); } } else session_save(session); + if (!scope_job) { + /* Clean this up after session_send_create_reply() */ + free(session->scope_job); + session->scope_job = NULL; + } + session_add_to_gc_queue(session); } user = hashmap_get(m->user_units, unit); if (user) { + bool opening, service_job = !!user->service_job; - if (streq_ptr(path, user->service_job)) { - free(user->service_job); - user->service_job = NULL; - } - - if (streq_ptr(path, user->slice_job)) { - free(user->slice_job); - user->slice_job = NULL; - } + /* Set to false if the user service was created */ + if (streq_ptr(path, user->service_job)) + service_job = false; LIST_FOREACH(sessions_by_user, session, user->sessions) { if (!session->started) continue; + /* If the session scope and the user service are + * still in the creation process this will be set + * to true, otherwise it will be false */ + opening = service_job || !!session->scope_job; + if (streq(result, "done")) - session_send_create_reply(session, NULL); + session_send_create_reply(session, NULL, opening); else { _cleanup_bus_error_free_ sd_bus_error e = SD_BUS_ERROR_NULL; sd_bus_error_setf(&e, BUS_ERROR_JOB_FAILED, "Start job for unit %s failed with '%s'", unit, result); - session_send_create_reply(session, &e); + session_send_create_reply(session, &e, opening); } } + if (!service_job) { + /* Clean this up after session_send_create_reply() */ + free(user->service_job); + user->service_job = NULL; + } + + if (streq_ptr(path, user->slice_job)) { + free(user->slice_job); + user->slice_job = NULL; + } + user_save(user); user_add_to_gc_queue(user); } diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 54ad827..72bef4c 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -640,7 +640,7 @@ int session_send_lock_all(Manager *m, bool lock) { return r; } -int session_send_create_reply(Session *s, sd_bus_error *error) { +int session_send_create_reply(Session *s, sd_bus_error *error, bool opening) { _cleanup_bus_message_unref_ sd_bus_message *c = NULL; _cleanup_close_ int fifo_fd = -1; _cleanup_free_ char *p = NULL; @@ -649,12 +649,12 @@ int session_send_create_reply(Session *s, sd_bus_error *error) { /* This is called after the session scope and the user service * were successfully created, and finishes where - * bus_manager_create_session() left off. */ + * method_create_session() left off. */ if (!s->create_message) return 0; - if (!sd_bus_error_is_set(error) && (s->scope_job || s->user->service_job)) + if (!sd_bus_error_is_set(error) && opening) return 0; c = s->create_message; @@ -663,6 +663,8 @@ int session_send_create_reply(Session *s, sd_bus_error *error) { if (error) return sd_bus_reply_method_error(c, error); + /* At this stage the session scope and the user service + * were successfully created */ fifo_fd = session_create_fifo(s); if (fifo_fd < 0) return fifo_fd; diff --git a/src/login/logind-session.h b/src/login/logind-session.h index d724e20..0810def 100644 --- a/src/login/logind-session.h +++ b/src/login/logind-session.h @@ -150,7 +150,7 @@ int session_send_changed(Session *s, const char *properties, ...) _sentinel_; int session_send_lock(Session *s, bool lock); int session_send_lock_all(Manager *m, bool lock); -int session_send_create_reply(Session *s, sd_bus_error *error); +int session_send_create_reply(Session *s, sd_bus_error *error, bool opening); const char* session_state_to_string(SessionState t) _const_; SessionState session_state_from_string(const char *s) _pure_; -- 1.8.3.1 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel