[systemd-devel] [RFC] logind: introduce session "positions"
David Herrmann
dh.herrmann at gmail.com
Mon Dec 2 04:51:55 PST 2013
Hi
On Mon, Dec 2, 2013 at 1:09 PM, Tom Gundersen <teg at jklm.no> wrote:
> On Sun, Dec 1, 2013 at 12:43 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>> logind has no concept of session ordering. Sessions have a unique name,
>> some attributes about the capabilities and that's already it. There is
>> currently no stable+total order on sessions. If we use the logind API to
>> switch between sessions, we are faced with an unordered list of sessions
>> we have no clue of.
>>
>> This used to be no problem on seats with VTs or on seats with only a
>> single active session. However, with the introduction of multi-session
>> capability for seats without VTs, we need to find a way to order sessions
>> in a stable way.
>>
>> This patch introduces session "positions". A position is a simple integer
>> assigned to a session which is never changed implicitly (currently, we
>> also don't change it explicitly, but that may be changed someday). For
>> seats with VTs, we force the position to be the same as the VTnr. Without
>> VTs, we simply find the lowest unassigned number and use it as position.
>> If position-assignment fails or if, for any reason, we decide to not
>> assign a position to a session, the position is set to 0 (which is treated
>> as invalid position).
>> During session_load() or if two sessions have the same VTnr, we may end up
>> with two sessions with the same position (this shouldn't happen, but lets
>> be fail-safe in case some other part of the stack fails). This case is
>> dealt with gracefully by ignoring any session but the first session
>> assigned to the position. Thus, session->pos is a hint, seat->positions[i]
>> is the definite position-assignment. Always verify both match in case you
>> need to modify them!
>>
>> Additionally, we introduce SwitchTo(unsigned int) on the seat-dbus-API.
>> You can call it with any integer value != 0 and logind will try to switch
>> to the request position. If you implement a compositor or any other
>> session-controller, you simply watch for ctrl+alt+F1 to F12 and call
>> SwitchTo(Fx). logind will figure a way out deal with this number.
>> For convenience, we also introduce SwitchToNext/Previous(). It should be
>> called on ctrl+alt+Left/Right (like the kernel-console used to support).
>
> So if (somehow) two sessions are assigned to the same position, the
This "somehow" really means someone hand-edited
/run/systemd/sessions/* or you have a corrupted FS and then
systemd-logind crashed. You never end up with two sessions on the same
position under normal conditions. I just mentioned it so people better
understand the error/fallback paths in the code.
This obviously isn't true for seats with VTs. There you can start
multiple sessions on the same VT. However, this is strongly
discouraged as systemd randomly chooses one session to be active then.
> behavior of Next/Previous would be something like: if the 'first'
> session on the given position is active, and you do SwitchToNext
> followed by SwitchToPrevious you get back were you started. However,
> if the 'second' session is the active one and you do Next/Previous you
> end up at the 'first' one?
>
> If we want to ensure that Next/Previous are true inverses of each
> other (which I guess might be more intuitive?) we could simply say
> that the most recent session on a given position is the 'first' one,
> does that make sense? Of course that means that Session.Activate() may
> change the session associated with, say, SwitchTo(3), but reading the
> stuff you wrote below about classes, that sort of makes sense.
>
> Also, what is the semantics of position 0, are these truly unordered,
> so that Next/Previous will fail when called on them, or will you end
> up at the 'first'/'last' position respectively? (Looking at the code,
> it seems that SwitchToNext is defined for position 0, but I may have
> misread).
Position 0 is unused. SwitchTo(0) returns EINVAL and
SwitchToNext/Previous skip this position. This is copied from VTs
where pos==0 is invalid, too.
>> Note that the public API (SwitchTo*()) is *not* bound to the underlying
>> logic that is implemented now. We don't export "session-positions" on the
>> dbus/C API! They are an implementation detail. Instead, the SwitchTo*()
>> API is supposed to be a hint to let logind choose the session-switching
>> logic. Any foreground session-controller is free to enumerate/order
>> existing sessions according to their needs and call Session.Activate()
>> manually. But the SwitchTo*() API provides a uniform behavior across
>> session-controllers.
>>
>> Background: Session-switching keys depend on the active keymap. The XKB
>> specification provides the XKB_KEY_XF86Switch_VT_1-12 key-symbols which
>> have to be mapped by all keymaps to allow session-switching. It is usually
>> bound to ctrl+alt+Fx but may be set differently. A compositor passes any
>> keyboard input to XKB before passing it to clients. In case a key-press
>> invokes the XKB_KEY_XF86Switch_VT_x action, the keypress is *not*
>> forwarded to clients, but instead a session-switch is scheduled.
>>
>> This actually prevents us from handling these keys outside of the session.
>> If an active compositor has a keymap with a different mapping of these
>> keys, and logind itself tries to catch these combinations, we end up with
>> the key-press sent to the compositor's clients *and* handled by logind.
>> This is *bad* and we must avoid this. The only situation where a
>> background process is allowed to handle key-presses is debugging and
>> emergency-keys. In these cases, we don't care for keymap mismatches and
>> accept the double-event. Another exception is unmapped keys like
>> PowerOff/Suspend (even though this one is controversial).
>>
>> Future ideas: As this commit-msg isn't long enough, yet, some notes on
>> future ideas. The current position-assignment is compatible with the
>> legacy VT numbers. However, it is a rather outdated way of addressing
>> sessions. Instead, we can make use of session-classes of logind. We
>> already tag session with one of the classes "greeter", "user",
>> "background", "lock-screen". So one of my ideas is to make
>> "position-assignment" a "per-class" thing. And instead of mapping F1-F12
>> directly to the positions, we map it as follows:
>> - F1: Activate the last-spawned session in the "greeter" class. Usually,
>> only a single greeter should be active, but in case
>> systemd-welcomed runs and gdm is spawned later, this will switch to
>> gdm (actually gdm.service should stop systemd-welcomed.service but
>> lets be overly pedantic here).
>> - F2: Activate the session from the "user" class which has been active
>> last. So if you switch to F1 and back to F2, you're guaranteed to
>> get back to your last active user-session.
>> - F3-F11: Direct mapping to "user" sessions. So F3 maps to the
>> user-session with position 3, F11 to position 11 (or apply a
>> "-2" offset, so F3=>1 and F11=>9..)
>> - F12: Switch to the last-spawned session in the "emergency" class. This
>> doesn't exist, yet, but we could start sessions showing the
>> kernel-log or an emergency-console here.
>>
>> Inside of each group, you can use SwitchToNext/Previous() to switch to the
>> next/previous session. As only the "user" group has a way of directly
>> addressing sessions, this is needed for the "greeter" and "emergency"
>> groups. Note that both groups are expected to only have one session, so
>> this is really just a fallback for odd cases. However, for the "user"
>> group the Next/Previous can come in quite handy.
>>
>> Comments?
>> ---
>> src/login/logind-seat-dbus.c | 56 ++++++++++++++++++++++++++
>> src/login/logind-seat.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
>> src/login/logind-seat.h | 8 ++++
>> src/login/logind-session.c | 13 ++++++
>> src/login/logind-session.h | 1 +
>> 5 files changed, 174 insertions(+)
>>
>> diff --git a/src/login/logind-seat-dbus.c b/src/login/logind-seat-dbus.c
>> index 23f975b..ff9e2e4 100644
>> --- a/src/login/logind-seat-dbus.c
>> +++ b/src/login/logind-seat-dbus.c
>> @@ -235,6 +235,59 @@ static int method_activate_session(sd_bus *bus, sd_bus_message *message, void *u
>> return sd_bus_reply_method_return(message, NULL);
>> }
>>
>> +static int method_switch_to(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
>> + Seat *s = userdata;
>> + unsigned int to;
>> + int r;
>> +
>> + assert(bus);
>> + assert(message);
>> + assert(s);
>> +
>> + r = sd_bus_message_read(message, "u", &to);
>> + if (r < 0)
>> + return r;
>> +
>> + if (!to)
>> + return -EINVAL;
>> +
>> + r = seat_switch_to(s, to);
>> + if (r < 0)
>> + return r;
>> +
>> + return sd_bus_reply_method_return(message, NULL);
>> +}
>> +
>> +static int method_switch_to_next(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
>> + Seat *s = userdata;
>> + int r;
>> +
>> + assert(bus);
>> + assert(message);
>> + assert(s);
>> +
>> + r = seat_switch_to_next(s);
>> + if (r < 0)
>> + return r;
>> +
>> + return sd_bus_reply_method_return(message, NULL);
>> +}
>> +
>> +static int method_switch_to_previous(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
>> + Seat *s = userdata;
>> + int r;
>> +
>> + assert(bus);
>> + assert(message);
>> + assert(s);
>> +
>> + r = seat_switch_to_previous(s);
>> + if (r < 0)
>> + return r;
>> +
>> + return sd_bus_reply_method_return(message, NULL);
>> +}
>> +
>> const sd_bus_vtable seat_vtable[] = {
>> SD_BUS_VTABLE_START(0),
>>
>> @@ -250,6 +303,9 @@ const sd_bus_vtable seat_vtable[] = {
>>
>> SD_BUS_METHOD("Terminate", NULL, NULL, method_terminate, 0),
>> SD_BUS_METHOD("ActivateSession", "s", NULL, method_activate_session, 0),
>> + SD_BUS_METHOD("SwitchTo", "u", NULL, method_switch_to, 0),
>> + SD_BUS_METHOD("SwitchToNext", NULL, NULL, method_switch_to_next, 0),
>> + SD_BUS_METHOD("SwitchToPrevious", NULL, NULL, method_switch_to_previous, 0),
>>
>> SD_BUS_VTABLE_END
>> };
>> diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c
>> index b1a5ec3..928e2c5 100644
>> --- a/src/login/logind-seat.c
>> +++ b/src/login/logind-seat.c
>> @@ -79,6 +79,7 @@ void seat_free(Seat *s) {
>>
>> hashmap_remove(s->manager->seats, s->id);
>>
>> + free(s->positions);
>> free(s->state_file);
>> free(s);
>> }
>> @@ -270,6 +271,60 @@ int seat_set_active(Seat *s, Session *session) {
>> return 0;
>> }
>>
>> +int seat_switch_to(Seat *s, unsigned int num) {
>> + /* Public session positions skip 0 (there is only F1-F12). Maybe it
>> + * will get reassigned in the future, so return error for now. */
>> + if (!num)
>> + return -EINVAL;
>> +
>> + if (num >= s->position_count || !s->positions[num])
>> + return -EINVAL;
>> +
>> + return session_activate(s->positions[num]);
>> +}
>> +
>> +int seat_switch_to_next(Seat *s) {
>> + unsigned int start, i;
>> +
>> + if (!s->position_count)
>> + return -EINVAL;
>> +
>> + start = 1;
>> + if (s->active && s->active->pos > 0)
>> + start = s->active->pos;
>> +
>> + for (i = start + 1; i < s->position_count; ++i)
>> + if (s->positions[i])
>> + return session_activate(s->positions[i]);
>> +
>> + for (i = 1; i < start; ++i)
>> + if (s->positions[i])
>> + return session_activate(s->positions[i]);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +int seat_switch_to_previous(Seat *s) {
>> + unsigned int start, i;
>> +
>> + if (!s->position_count)
>> + return -EINVAL;
>> +
>> + start = 1;
>> + if (s->active && s->active->pos > 0)
>> + start = s->active->pos;
>> +
>> + for (i = start - 1; i > 0; --i)
>> + if (s->positions[i])
>> + return session_activate(s->positions[i]);
>> +
>> + for (i = s->position_count - 1; i > start; --i)
>> + if (s->positions[i])
>> + return session_activate(s->positions[i]);
>> +
>> + return -EINVAL;
>> +}
>> +
>> int seat_active_vt_changed(Seat *s, unsigned int vtnr) {
>> Session *i, *new_active = NULL;
>> int r;
>> @@ -403,6 +458,46 @@ int seat_stop_sessions(Seat *s) {
>> return r;
>> }
>>
>> +void seat_evict_position(Seat *s, Session *session) {
>> + unsigned int pos = session->pos;
>> +
>> + session->pos = 0;
>> +
>> + if (!pos)
>> + return;
>> +
>> + if (pos < s->position_count && s->positions[pos] == session)
>> + s->positions[pos] = NULL;
>
> Shouldn't you restore some other session to this position if there are
> other sessions with s->pos == pos?
As I said above, that shouldn't happen. We would have to scan through
s->sessions to look whether there's another session with this
position. But if someone tinkers with our runtime files, a lot more
can go wrong than just duplicated positions. So I doubt it would help
to do that here.
Worst thing that can happen is that the second session on this
position is unreachable via SwitchTo*(). Seems ok, doesn't it?
Cheers
David
>> +}
>> +void seat_claim_position(Seat *s, Session *session, unsigned int pos) {
>> + /* with VTs, the position is always the same as the VTnr */
>> + if (seat_has_vts(s))
>> + pos = session->vtnr;
>> +
>> + if (!GREEDY_REALLOC0(s->positions, s->position_count, pos + 1))
>> + return;
>> +
>> + seat_evict_position(s, session);
>> +
>> + session->pos = pos;
>> + if (pos > 0 && !s->positions[pos])
>> + s->positions[pos] = session;
>> +}
>> +
>> +static void seat_assign_position(Seat *s, Session *session) {
>> + unsigned int pos;
>> +
>> + if (session->pos > 0)
>> + return;
>> +
>> + for (pos = 1; pos < s->position_count; ++pos)
>> + if (!s->positions[pos])
>> + break;
>> +
>> + seat_claim_position(s, session, pos);
>> +}
>> +
>> int seat_attach_session(Seat *s, Session *session) {
>> assert(s);
>> assert(session);
>> @@ -413,6 +508,7 @@ int seat_attach_session(Seat *s, Session *session) {
>>
>> session->seat = s;
>> LIST_PREPEND(sessions_by_seat, s->sessions, session);
>> + seat_assign_position(s, session);
>>
>> seat_send_changed(s, "Sessions", NULL);
>>
>> diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h
>> index 1254268..9e21e3a 100644
>> --- a/src/login/logind-seat.h
>> +++ b/src/login/logind-seat.h
>> @@ -41,6 +41,9 @@ struct Seat {
>> Session *pending_switch;
>> LIST_HEAD(Session, sessions);
>>
>> + Session **positions;
>> + size_t position_count;
>> +
>> bool in_gc_queue:1;
>> bool started:1;
>>
>> @@ -55,12 +58,17 @@ int seat_load(Seat *s);
>>
>> int seat_apply_acls(Seat *s, Session *old_active);
>> int seat_set_active(Seat *s, Session *session);
>> +int seat_switch_to(Seat *s, unsigned int num);
>> +int seat_switch_to_next(Seat *s);
>> +int seat_switch_to_previous(Seat *s);
>> int seat_active_vt_changed(Seat *s, unsigned int vtnr);
>> int seat_read_active_vt(Seat *s);
>> int seat_preallocate_vts(Seat *s);
>>
>> int seat_attach_session(Seat *s, Session *session);
>> void seat_complete_switch(Seat *s);
>> +void seat_evict_position(Seat *s, Session *session);
>> +void seat_claim_position(Seat *s, Session *session, unsigned int pos);
>>
>> bool seat_has_vts(Seat *s);
>> bool seat_is_seat0(Seat *s);
>> diff --git a/src/login/logind-session.c b/src/login/logind-session.c
>> index beaa601..c24e573 100644
>> --- a/src/login/logind-session.c
>> +++ b/src/login/logind-session.c
>> @@ -125,6 +125,7 @@ void session_free(Session *s) {
>> if (s->seat->pending_switch == s)
>> s->seat->pending_switch = NULL;
>>
>> + seat_evict_position(s->seat, s);
>> LIST_REMOVE(sessions_by_seat, s->seat->sessions, s);
>> }
>>
>> @@ -231,6 +232,9 @@ int session_save(Session *s) {
>> if (s->seat && seat_has_vts(s->seat))
>> fprintf(f, "VTNR=%u\n", s->vtnr);
>>
>> + if (!s->vtnr)
>> + fprintf(f, "POS=%u\n", s->pos);
>> +
>> if (s->leader > 0)
>> fprintf(f, "LEADER=%lu\n", (unsigned long) s->leader);
>>
>> @@ -266,6 +270,7 @@ int session_load(Session *s) {
>> _cleanup_free_ char *remote = NULL,
>> *seat = NULL,
>> *vtnr = NULL,
>> + *pos = NULL,
>> *leader = NULL,
>> *type = NULL,
>> *class = NULL,
>> @@ -290,6 +295,7 @@ int session_load(Session *s) {
>> "REMOTE_USER", &s->remote_user,
>> "SERVICE", &s->service,
>> "VTNR", &vtnr,
>> + "POS", &pos,
>> "LEADER", &leader,
>> "TYPE", &type,
>> "CLASS", &class,
>> @@ -350,6 +356,13 @@ int session_load(Session *s) {
>> if (!s->seat || !seat_has_vts(s->seat))
>> s->vtnr = 0;
>>
>> + if (pos && s->seat) {
>> + unsigned int npos;
>> +
>> + safe_atou(pos, &npos);
>> + seat_claim_position(s->seat, s, npos);
>> + }
>> +
>> if (leader) {
>> k = parse_pid(leader, &s->leader);
>> if (k >= 0)
>> diff --git a/src/login/logind-session.h b/src/login/logind-session.h
>> index ee93101..d724e20 100644
>> --- a/src/login/logind-session.h
>> +++ b/src/login/logind-session.h
>> @@ -69,6 +69,7 @@ struct Session {
>> Manager *manager;
>>
>> char *id;
>> + unsigned int pos;
>> SessionType type;
>> SessionClass class;
>>
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> systemd-devel mailing list
>> systemd-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
More information about the systemd-devel
mailing list