On Fri, Feb 07, 2014 at 04:48:39PM +0100, Lennart Poettering wrote: > On Thu, 06.02.14 21:37, Djalal Harouni (tix...@opendz.org) wrote: > > I think this one I fixed by adding a new "stopping" variable. Could you check? Yes, the stopping variable caught most of the races, I've sent another patch that will close which I think is the last one: http://lists.freedesktop.org/archives/systemd-devel/2014-February/016889.html
Thanks Lennart! > > Currently the user and session states are not stable, they are affected > > by several races during login: > > > > 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() 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. > > If so it will clear the session->scope_job > > > > => session_get_state() == SESSION_CLOSING (incorrect) > > (session closing since fifo_fd == -1) > > > > So scope job has finished and scope was created successfully, later the > > session_send_create_reply() 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) > > > > To sum it up, current state during login: > > "SESSION_OPENING"=>"SESSION_CLOSING"x2=>"SESSION_ACTIVE" > > > > 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 "active" variable to check if the session scope and user > > service jobs are still being created. > > > > Update session_jobs_replay() and session_send_create_reply() function to > > receive the "opening" variable as an argument, so it will still wait for > > the scope and service jobs to finish before creating the session fifo. > > > > The "session->scope_job" will be cleared when session_jobs_reply() > > finishes. This ensures that the state will just go from: > > "SESSION_OPENING" => "SESSION_ACTIVE" > > > > 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(). This will > > we be fixed in the followup patches. > > > > 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) > > > > user_start() calls: > > 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() 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, > > here the state should stay USER_OPENING) > > > > Later when the user service is created successfully, > > session_send_create_reply() will also wait for the session scope to be > > 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) > > > > To fix this, we use the same logic as used to fix session states. 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. > > > > The new "active" variable will be used to check if the session scope and > > user service are still being created. If so we'll wait for the next > > match_job_removed() signal and continue, otherwise we proceed with > > session_jobs_reply() and session_send_create_reply() in order to notify > > clients. > > --- > > src/login/logind-dbus.c | 44 > > ++++++++++++++++++++++++++++++----------- > > src/login/logind-session-dbus.c | 8 +++++--- > > src/login/logind-session.h | 2 +- > > 3 files changed, 39 insertions(+), 15 deletions(-) > > > > diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c > > index 7b050fb..0560707 100644 > > --- a/src/login/logind-dbus.c > > +++ b/src/login/logind-dbus.c > > @@ -1919,7 +1919,9 @@ const sd_bus_vtable manager_vtable[] = { > > SD_BUS_VTABLE_END > > }; > > > > -static int session_jobs_reply(Session *s, const char *unit, const char > > *result) { > > +/* session_jobs_reply() calls session_send_create_reply() to > > + * notify client that they are able to login now. */ > > +static int session_jobs_reply(Session *s, const char *unit, const char > > *result, bool opening) { > > int r = 0; > > > > assert(s); > > @@ -1929,12 +1931,12 @@ static int session_jobs_reply(Session *s, const > > char *unit, const char *result) > > return r; > > > > if (streq(result, "done")) > > - r = session_send_create_reply(s, NULL); > > + r = session_send_create_reply(s, 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); > > - r = session_send_create_reply(s, &e); > > + r = session_send_create_reply(s, &e, opening); > > } > > > > return r; > > @@ -1973,22 +1975,46 @@ 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 active, scope_job = !!session->scope_job; > > > > - if (streq_ptr(path, session->scope_job)) { > > + /* Set to false if the scope job has finished */ > > + if (streq_ptr(path, session->scope_job)) > > + scope_job = false; > > + > > + /* If the session scope and the user service are still > > + * being created this will be set to true, otherwise > > + * it will be false */ > > + active = scope_job || !!session->user->service_job; > > + session_jobs_reply(session, unit, result, active); > > + > > + if (!scope_job) { > > + /* Clean this up after session_jobs_reply() */ > > free(session->scope_job); > > session->scope_job = NULL; > > } > > > > - session_jobs_reply(session, unit, result); > > - > > session_save(session); > > session_add_to_gc_queue(session); > > } > > > > user = hashmap_get(m->user_units, unit); > > if (user) { > > + bool active, service_job = !!user->service_job; > > + > > + /* Set to false if the user service has finished */ > > + if (streq_ptr(path, user->service_job)) > > + service_job = false; > > + > > + LIST_FOREACH(sessions_by_user, session, user->sessions) { > > + /* If the session scope and the user service are > > + * still being created this will be set to true, > > + * otherwise it will be false */ > > + active = service_job || !!session->scope_job; > > + session_jobs_reply(session, unit, result, active); > > + } > > > > - if (streq_ptr(path, user->service_job)) { > > + if (!service_job) { > > + /* Clean this up after session_jobs_reply() */ > > free(user->service_job); > > user->service_job = NULL; > > } > > @@ -1998,10 +2024,6 @@ int match_job_removed(sd_bus *bus, sd_bus_message > > *message, void *userdata, sd_b > > user->slice_job = NULL; > > } > > > > - LIST_FOREACH(sessions_by_user, session, user->sessions) { > > - session_jobs_reply(session, unit, result); > > - } > > - > > 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 7ee4956..54db864 100644 > > --- a/src/login/logind-session-dbus.c > > +++ b/src/login/logind-session-dbus.c > > @@ -641,7 +641,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; > > @@ -650,12 +650,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; > > @@ -664,6 +664,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 7bf1932..ebe3902 100644 > > --- a/src/login/logind-session.h > > +++ b/src/login/logind-session.h > > @@ -152,7 +152,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_; > > > Lennart > > -- > Lennart Poettering, Red Hat -- Djalal Harouni http://opendz.org _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel