[systemd-devel] [PATCH 1/3] logind: make the user and session states more stable
Djalal Harouni
tixxdz at opendz.org
Wed Jan 22 01:25:51 PST 2014
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
More information about the systemd-devel
mailing list