Attachment 116145 Details for Bug 90769 – logind: Fix user_elect_display() to be more stable

[patch] logind: Fix user_elect_display() to be more stable

logind-Fix-userelectdisplay-to-be-more-stable.patch (text/plain), 5.61 KB, created by Philip Withnall on 2015-05-29 10:07:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Philip Withnall

Created: 2015-05-29 10:07:20 UTC

Size: 5.61 KB

patch

obsolete

>From d6721c5fb3a20a0f99f1f6f075a397531b86db4f Mon Sep 17 00:00:00 2001
>From: Philip Withnall <philip.withnall@collabora.co.uk>
>Date: Fri, 29 May 2015 10:49:21 +0100
>Subject: [PATCH] logind: Fix user_elect_display() to be more stable
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>The previous implementation of user_elect_display() could easily end up
>overwriting the userâs valid graphical session with a new TTY session.
>For example, consider the situation where there is one session:
>   c1, type = SESSION_X11, !stopping, class = SESSION_USER
>it is initially elected as the userâs display (i.e. u->display = c1).
>
>If another session is started, on a different VT, the sessions_by_user
>list becomes:
>   c1, type = SESSION_X11, !stopping, class = SESSION_USER
>   c2, type = SESSION_TTY, !stopping, class = SESSION_USER
>
>In the previous code, graphical = c1 and text = c2, as expected.
>However, neither graphical nor text fulfil the conditions for setting
>u->display = graphical (because neither is better than u->display), so
>the code falls through to check the text variable. The conditions for
>this match, as u->display->type != SESSION_TTY (itâs actually
>SESSION_X11). Hence u->display is set to c2, which is incorrect, because
>session c1 is still valid.
>
>Refactor user_elect_display() to use a more explicit filter and
>pre-order comparison over the sessions. This can be demonstrated to be
>stable and only ever âupgradeâ the session to a more graphical one.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=90769
>---
> src/login/logind-user.c | 90 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 54 insertions(+), 36 deletions(-)
>
>diff --git a/src/login/logind-user.c b/src/login/logind-user.c
>index b00e9b1..4e82e9b 100644
>--- a/src/login/logind-user.c
>+++ b/src/login/logind-user.c
>@@ -738,54 +738,72 @@ int user_kill(User *u, int signo) {
>         return manager_kill_unit(u->manager, u->slice, KILL_ALL, signo, NULL);
> }
> 
>+static bool
>+elect_display_filter_cb(Session *s) {
>+        /* Return true if the session is a candidate for the userâs âprimary
>+         * sessionâ or âdisplayâ. */
>+        assert(s);
>+
>+        return (s->class == SESSION_USER && !s->stopping);
>+}
>+
>+static int
>+elect_display_sort_cb(Session *s1, Session *s2) {
>+        /* Indexed by SessionType. Lower numbers mean more preferred. */
>+        const int type_ranks[_SESSION_TYPE_MAX] = {
>+                [SESSION_UNSPECIFIED] = 0,
>+                [SESSION_TTY] = -2,
>+                [SESSION_X11] = -3,
>+                [SESSION_WAYLAND] = -3,
>+                [SESSION_MIR] = -3,
>+                [SESSION_WEB] = -1,
>+        };
>+
>+        /* Calculate the partial order relationship between s1 and s2,
>+         * returning < 0 if s1 is preferred as the userâs âprimary sessionâ,
>+         * 0 if s1 and s2 are equally preferred or incomparable, or > 0 if s2
>+         * is preferred.
>+         *
>+         * s1 or s2 may be NULL. */
>+        if ((s1 == NULL) != (s2 == NULL))
>+                return (s1 == NULL) - (s2 == NULL);
>+
>+        if ((s1->class != SESSION_USER) != (s2->class != SESSION_USER))
>+                return (s1->class != SESSION_USER) - (s2->class != SESSION_USER);
>+
>+        if (s1->stopping != s2->stopping)
>+                return s1->stopping - s2->stopping;
>+
>+        if ((s1->type == _SESSION_TYPE_INVALID) != (s2->type == _SESSION_TYPE_INVALID))
>+                return (s1->type == _SESSION_TYPE_INVALID) - (s2->type == _SESSION_TYPE_INVALID);
>+
>+        if (s1->type != s2->type)
>+                return type_ranks[s1->type] - type_ranks[s2->type];
>+
>+        return 0;
>+}
>+
> void user_elect_display(User *u) {
>-        Session *graphical = NULL, *text = NULL, *other = NULL, *s;
>+        Session *s;
> 
>         assert(u);
> 
>         /* This elects a primary session for each user, which we call
>          * the "display". We try to keep the assignment stable, but we
>          * "upgrade" to better choices. */
>+        log_debug("Electing new display for user %s", u->name);
> 
>         LIST_FOREACH(sessions_by_user, s, u->sessions) {
>-
>-                if (s->class != SESSION_USER)
>-                        continue;
>-
>-                if (s->stopping)
>+                if (!elect_display_filter_cb(s)) {
>+                        log_debug("Ignoring session %s", s->id);
>                         continue;
>+                }
> 
>-                if (SESSION_TYPE_IS_GRAPHICAL(s->type))
>-                        graphical = s;
>-                else if (s->type == SESSION_TTY)
>-                        text = s;
>-                else
>-                        other = s;
>-        }
>-
>-        if (graphical &&
>-            (!u->display ||
>-             u->display->class != SESSION_USER ||
>-             u->display->stopping ||
>-             !SESSION_TYPE_IS_GRAPHICAL(u->display->type))) {
>-                u->display = graphical;
>-                return;
>-        }
>-
>-        if (text &&
>-            (!u->display ||
>-             u->display->class != SESSION_USER ||
>-             u->display->stopping ||
>-             u->display->type != SESSION_TTY)) {
>-                u->display = text;
>-                return;
>+                if (elect_display_sort_cb(s, u->display) < 0) {
>+                        log_debug("Choosing session %s in preference to %s", s->id, u->display ? u->display->id : "-");
>+                        u->display = s;
>+                }
>         }
>-
>-        if (other &&
>-            (!u->display ||
>-             u->display->class != SESSION_USER ||
>-             u->display->stopping))
>-                u->display = other;
> }
> 
> static const char* const user_state_table[_USER_STATE_MAX] = {
>-- 
>2.1.0

Actions: View | Splinter Review

Attachments on bug 90769: 116145

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.