GNOME Bugzilla – Bug 744764
use wayland on login screen by default
Last modified: 2017-04-13 20:33:53 UTC
This is a placeholder bug for changing the login screen over to use wayland by default. There are certain situations where wayland won't be an option (non-kms systems, remote XDMCP server, legacy ConsoleKit systems) but we can get it running in the lion's share of cases, so we should try and fallback if things don't work out. I've got a branch that implements this a long with a bunch of clean ups, which post here, so we have a place to deal with the inevitable fall out.
Created attachment 297157 [details] [review] Revert "GdmLaunchEnvironment: allow a session bus address to be passed in" This reverts commit 5872d0738cf0bddd66d8aeae35bacadb04baa4c6. I pushed this patch prematurely. https://bugzilla.gnome.org/show_bug.cgi?id=693668
Created attachment 297158 [details] [review] session: get rid of __previous handling It's vestigial.
Created attachment 297159 [details] [review] manager: clean up some wonky error handling in OpenSession We're leaking errors without sending them back to the caller.
Created attachment 297160 [details] [review] manager: always call gdm_display_unmanage before finish We need to clean up various resources before finishing the display.
Created attachment 297161 [details] [review] manager: handle autologin directly Right now we call a "set_up_greeter_session" method on the display object to to find out the autologin username. This is weird because there's no greeter session when performing an autologin. Also, the display delegates to the slave, and we want to get rid of the slave. Furthermore, the manager handles most of the autologin implementation already, it's just the "get the username" part (which is always "root"), that it delegates. This commit changes the manager to just handle the autologin process completely by itself.
Created attachment 297162 [details] [review] manager: drop wait-for-go GDM used to have this concept called "wait-for-go" where it would stall at boot up and wait for a go signal before proceeding. This feature never got fully reimplemented after the rewrite in '22 and and it's not used anyway. Drop it.
Created attachment 297163 [details] [review] daemon: move plymouth code from slave to manager In an effort to pare down the slave code, this commit moves the plymouth bits to GdmManager.
Created attachment 297164 [details] [review] chooser: drop display-device argument It's always NULL.
Created attachment 297165 [details] [review] static-display: drop extraneous code We set up vfuncs that just call into the default implementation. This commit strips it.
Created attachment 297166 [details] [review] transient-display: drop extraneous code This commit is like the previous commit but for transient displays instead of static displays. We set up vfuncs that just call into the default implementation. This commit strips it.
Created attachment 297167 [details] [review] xdmcp-display: drop extraneous code This commit is like the previous commit but for xdmcp displays instead of transient displays. We set up vfuncs that just call into the default implementation. This commit strips it.
Created attachment 297168 [details] [review] xdmcp-greeter-display: drop class GdmXdmcpGreeterDisplay doesn't do anything that it's parent class does (except call unmanage from finalize which is strange and probably wrong). This commit drops the file and instead just uses GdmXdmcpDisplay directly, instead.
Created attachment 297169 [details] [review] xdmcp-chooser-display: remove dubious looking finish implementation This commit drops the finish implementation from the xdmcp-chooser-display subclass. It's mostly just chaining up, but it also calls "unmanage" which is strange and probably wrong.
Created attachment 297170 [details] [review] static-display: get rid of finish implementation It's there to make sure the display gets recreated after it's done, but we already handle that in the local-display-factory.
Created attachment 297171 [details] [review] display: drop unsubclassed vfuncs gdm-display has a whole host of vfuncs that are now completely unused. This commit drops thems.
Created attachment 297172 [details] [review] display: drop error arg from gdm_display_get_timed_login_details It turns out it never fails, and all callers always pass NULL for it's error argument, so drop that argument and the gboolean return type.
Created attachment 297173 [details] [review] display: get rid of get_timed_login_details vfunc Instead introduce an "allow-timed-login" property that subclasses can set to FALSE to disable timed login.
Created attachment 297174 [details] [review] session: detect when conversation is started more than once and error out It's a bug when it happens but this makes the failure happen right away instead of leading to a dbus timeout.
Created attachment 297175 [details] [review] display: use bitfield for booleans in priv structure We're going to be adding more than a couple, so switch to a bitfield.
Created attachment 297176 [details] [review] display: rename "display" to "self" self is the defacto standard these days.
Created attachment 297177 [details] [review] display: connect to accountsservice from display object The idea is to eventually gut GdmSimpleSlave, so start by copying some of its functionality over to the display. This commit copies accountsservice user listing.
Created attachment 297178 [details] [review] display: add launch-environment property
Created attachment 297179 [details] [review] display: merge GdmStaticDisplay and GdmTransientDisplay They basically do the same thing with very minor differences, so merge them into a new GdmLocalDisplay object.
Created attachment 297180 [details] [review] local-display-factory: rename 'displays' to 'used-display-numbers' GdmLocalDisplayFactory has a hash table called "displays" that doesn't hold displays. It instead holds a list of used display numbers. This commit renames the hash table accordingly.
Created attachment 297181 [details] [review] local-display-factory: drop some superfluous code The store_display function takes a display number as an argument, it then uses that display number to reserve a slot in the "used display numbers" hash table. There's no reason for it to do this though, because the slot is already reserved when the next available display number is looked up in take_next_display_number. This commit drops the superfluous code.
Created attachment 297182 [details] [review] xdmcp-factory: handle display status changes after manager The factory removes the display from the display store, so it should run last. Furthermore the manager, sets up the greeter session, so it should run first.
Created attachment 297183 [details] [review] display: start greeter/chooser session from display object We're trying to get rid of the slave code, so we need to move responibility for launching the greeter to the display objects. This commit changes the display classes to set up a launch environment that the base class runs.
Created attachment 297184 [details] [review] slave: remove accountsservice stuff This is handled on the display object now.
Created attachment 297185 [details] [review] slave: drop some dead code
Created attachment 297186 [details] [review] simple-slave: get rid of finalize method We don't do anything in it.
Created attachment 297187 [details] [review] slave: drop some dead code There is a random smattering of vestigial code in gdm-simple-slave.c. This commit expunges it.
Created attachment 297188 [details] [review] display: move x11 connection from slave to display It's more logical here, and we're trying to get rid of the slave object eventually, anyway.
Created attachment 297189 [details] [review] display: add back manage vfunc This function will be overridden by GdmLocalDisplay to start the X server.
Created attachment 297190 [details] [review] display: get rid of more simple slave code. This commit moves GdmServer to GdmLocalDisplay, and XDMCP connection retries to GdmXdmcpDisplay.
Created attachment 297191 [details] [review] slave: drop the slaves At this point the slaves do nothing useful, so we can get rid of them.
Created attachment 297192 [details] [review] display: drop base class implementation of manage vfunc All it does is set the managed state, which the subclasses can do just as easily, so cut out some code.
Created attachment 297193 [details] [review] session: track login session display mode separate from user sessions In the future X based user sessions will get their own VT just like wayland sessions do. For the time being, the login screen still needs to use the VT and X server set up by the slave code though. This commit makes sure that the two cases are handled distinctly, so down the line they can be given different values.
Created attachment 297194 [details] [review] session: add #ifdef ENABLE_WAYLAND_SUPPORT guard I just noticed there's a forward declaration that's not protected by the enable wayland support definition. This probably fixes a compiler warning in some configurations.
Created attachment 297195 [details] [review] manager: add way for displays to register themselves The plan is to start the X server implicitly as part of starting the session. Once we do that, we'll need some way to communicate that the X server started successfully. This commit adds a RegisterX11Display method to GdmManager to handle that.
Created attachment 297196 [details] [review] session: allow display to be set after object construction When we support starting the X server in the session, we're not going to know the display name up front. This commit makes it possible to set the display name later.
Created attachment 297197 [details] [review] session: add session pid getter We're going to move wtmp recording to GdmManager, and for that we'll need the pid of the session.
Created attachment 297198 [details] [review] session: rename on_session_{started,opened} to on_user_session_{started,opened} All the other handlers dealing with user sessions have user in the name, so this commits brings the two outliers in line.
Created attachment 297199 [details] [review] session: add authentication-failed signal This signal records when a user tried to log in but typed the wrong password (or whatever). We'll need this to move session recording up to GdmManager.
Created attachment 297200 [details] [review] session: move wtmp/btmp logging to GdmManager These record needs to know the display name, and that isn't always available to GdmSession, so this commit moves recording to GdmManager.
Created attachment 297201 [details] [review] manager: rename seed session to embryonic user session Previously the seed session was called the "user session". This was confusing because it exists before the user logs in, so it got renamed to "seed session". This is confusing because it's not immediately clear what a seed session is. It's a lot of typing, but I'm going to go with "embryonic user session" now. Then it's clear it's destined to be a user session.
Created attachment 297202 [details] [review] daemon: add wrapper for launching wayland session This wrapper makes sure the wayland session is registered with the display manager, and that the wayland session gets a dbus-daemon that's automatically cleaned up when the session goes away.
Created attachment 297203 [details] [review] daemon: add wrapper for launching X session This commit adds a utility for launching an X server and session together. This utility works a lot like xinit, but makes use of modern X features (like -displayfd) and has GDM specific integration (like reading from gdm configuration, and optionally running through /etc/gdm/Xsession) The eventual idea is to get the main GDM code out of the X server launching business and instead farm the logic off to an unprivileged helper that gets run within the logind session of the user.
Created attachment 297204 [details] [review] launch-environment: don't start dbus-daemon Either the wrappers will start one, or gnome-session will start one, or libdbus will start one.
Created attachment 297205 [details] [review] display: don't make d-bus object path require display number Going forward we aren't always going to know the display number ahead of time, so don't use it for encoding the display id.
Created attachment 297206 [details] [review] display: don't export so much stuff over system bus When the slave was in a different process we needed to interact with the display in the manager process remotely. Now it's all one process and we don't, so clean up what gets exported over the bus.
Created attachment 297207 [details] [review] session: run logind managed displays through session wrappers Now that we have a utility for starting a wayland session (and registering it with GDM) we should use the tool. Now that we have a utility for starting an X server from within the user session, we can support the LOGIND_MANAGED display mode for X sessions, in a similar way to how we support wayland sessions on a new vt. This commit adds the plumbing necessary to use the new tools.
Created attachment 297208 [details] [review] worker: don't require --enable-wayland to support logind display mode Now that the logind display mode is supported for X servers, we shouldn't cull the code when --disable-wayland is passed to configure.
Created attachment 297209 [details] [review] session: start user X sessions on a new VT We now have all the plumbing in place to start user X sessions on new VTs, so we should do it. This commit does that.
Created attachment 297210 [details] [review] local-display-factory: don't assume display has display number logind managed displays don't have (known) display numbers, so we shouldn't assert that there's going to be one on display status changes.
Created attachment 297211 [details] [review] session: start login screen using session wrappers Since we have the wrappers, let's use them for the login screen, too, when we can. This commit removes GdmServer from GdmLocalDisplay, since starting the X server should be handled implicitly by gdm-x-session, now. All the old logic is now in a new GdmLegacyDisplay object, that will remain for cases where gdm-x-session doesn't work. (ConsoleKit systems, non-seat0 displays, etc)
Created attachment 297212 [details] [review] local-display: don't manage automatically Now that local displays register themselves and registration leads to the displays getting managed, we don't need to manage explicitly (and prematurely).
Created attachment 297213 [details] [review] local-display-factory: only set is-initial on first seat0 display is-initial is for hardcoding vt1 on the X server command line. VTs only make sense on seat0, so GdmServer as ignoring is-initial on auxilliary seats. This commit makes sure that is-initial never gets set in the first place for auxilliary seats, and deletes the code the overrides its value.
Created attachment 297214 [details] [review] session: change bitfield type from gboolean to guint32 gboolean is signed, so it's not a good idea to make it a bitfield.
Created attachment 297215 [details] [review] session: forward is-initial from display to worker The worker needs to know if a display should be forced on vt1 or not when deciding which vt to allocate for the logind session.
Created attachment 297216 [details] [review] worker: force vt 1 for initial display If the session is logind managed, then we currently give it the next available VT. VT 1 will never show up as available, though, since it's allocated and in the foreground. This commit makes sure that, if the initial display is logind managed, then it will get put on VT 1.
Created attachment 297217 [details] [review] data: drop gdm-shell.session It's identical to gnome.session these days. All GDMification comes from the overriden gnome-shell.desktop file.
Created attachment 297218 [details] [review] data: add a gnome-shell-wayland.desktop file This is like gnome-shell.desktop but for wayland. It adds the --mode=gdm argument.
Created attachment 297219 [details] [review] session: plug memory leak gdm_session_get_session_id erroneously strdup's it's return value.
Created attachment 297220 [details] [review] session: add session-type property This property tells us whether or not the display will be wayland. We currently try to autodetect the situation, but fail to for program sessions. This provides a way to override detection.
Created attachment 297221 [details] [review] display: add new session-class property The session-class property is analagous to the sd_login session class. It can be either "greeter" or "user". This helps us decide whether or not to add a launch environment to the display. We need this because some displays go straight to a user session.
Created attachment 297222 [details] [review] manager: keep track of implicitly created displays If a display is created implicitly using gdm-x-session or gdm-wayland-session then we need to add it to the display store so we can track it.
Created attachment 297223 [details] [review] display: only create Xauth file for legacy code paths GdmLocalDisplay doesn't need it, so don't bother doing it then.
Created attachment 297224 [details] [review] display: only add user authorization if connected to display If we aren't connected to the display then we can't give the user access to it,(and we don't need to anyway) This commit adds a new is-connected property to GdmDisplay and changes the code to never give a user authorization if we aren't connected.
Created attachment 297225 [details] [review] session: add way to get session id for specific conversation Right now, the only way to find out the session id of an opened (but not started) session is a parameter in the session-opened handler. This commit adds a new function gdm_session_get_conversation_session_id that returns the session-id for a specified opened conversation.
Created attachment 297226 [details] [review] launch-environment: add session-type property The session-type property is analagous to the sd_login session type. It can be either "x11" or "wayland". This helps us decide whether to start a wayland session or an X session.
Created attachment 297227 [details] [review] display: add session-type property The session-type property is analagous to the sd_login session type. It can be either "x11" or "wayland". This helps us decide whether to start a wayland session or an X session.
Created attachment 297228 [details] [review] local-display-factory: use wayland by default for greeter, fallback to X11 This commit flips the big red switch, now we use wayland by default on the greeter and only fallback to X / legacy code paths in exceptional situations.
Created attachment 297229 [details] [review] local-display-factory: provide override for disabling wayland Since this wayland code is still pretty new, we need to provide an exit tactic for users. This commit introduces daemon/WaylandEnable which can be set to false, to disable wayland support.
Pushing, but leaving bug open for time being.
Attachment 297157 [details] pushed as 45717ba - Revert "GdmLaunchEnvironment: allow a session bus address to be passed in" Attachment 297158 [details] pushed as ecb7f14 - session: get rid of __previous handling Attachment 297159 [details] pushed as be31185 - manager: clean up some wonky error handling in OpenSession Attachment 297160 [details] pushed as 972e242 - manager: always call gdm_display_unmanage before finish Attachment 297161 [details] pushed as 998af1e - manager: handle autologin directly Attachment 297162 [details] pushed as e1edfea - manager: drop wait-for-go Attachment 297163 [details] pushed as c4a1bf5 - daemon: move plymouth code from slave to manager Attachment 297164 [details] pushed as 7f1546e - chooser: drop display-device argument Attachment 297165 [details] pushed as fe350ef - static-display: drop extraneous code Attachment 297166 [details] pushed as 6e7c1da - transient-display: drop extraneous code Attachment 297167 [details] pushed as c5fd695 - xdmcp-display: drop extraneous code Attachment 297168 [details] pushed as 33bb9cf - xdmcp-greeter-display: drop class Attachment 297169 [details] pushed as 9bca76f - xdmcp-chooser-display: remove dubious looking finish implementation Attachment 297170 [details] pushed as f196d21 - static-display: get rid of finish implementation Attachment 297171 [details] pushed as 8d3d988 - display: drop unsubclassed vfuncs Attachment 297172 [details] pushed as 859ed52 - display: drop error arg from gdm_display_get_timed_login_details Attachment 297173 [details] pushed as 185db2f - display: get rid of get_timed_login_details vfunc Attachment 297174 [details] pushed as 153b150 - session: detect when conversation is started more than once and error out Attachment 297175 [details] pushed as 1b4c2c9 - display: use bitfield for booleans in priv structure Attachment 297176 [details] pushed as 2bf8e2e - display: rename "display" to "self" Attachment 297177 [details] pushed as c5beec9 - display: connect to accountsservice from display object Attachment 297178 [details] pushed as 6b417ef - display: add launch-environment property Attachment 297179 [details] pushed as 2cb9568 - display: merge GdmStaticDisplay and GdmTransientDisplay Attachment 297180 [details] pushed as 8c21038 - local-display-factory: rename 'displays' to 'used-display-numbers' Attachment 297181 [details] pushed as 7d9da46 - local-display-factory: drop some superfluous code Attachment 297182 [details] pushed as 0cc8b02 - xdmcp-factory: handle display status changes after manager Attachment 297183 [details] pushed as 50f32ce - display: start greeter/chooser session from display object Attachment 297184 [details] pushed as f7c4c6a - slave: remove accountsservice stuff Attachment 297185 [details] pushed as 7b9dfd7 - slave: drop some dead code Attachment 297186 [details] pushed as 1e10c15 - simple-slave: get rid of finalize method Attachment 297187 [details] pushed as 7b9dfd7 - slave: drop some dead code Attachment 297188 [details] pushed as 08eee2a - display: move x11 connection from slave to display Attachment 297189 [details] pushed as 8b2ac7e - display: add back manage vfunc Attachment 297190 [details] pushed as ca5fccc - display: get rid of more simple slave code. Attachment 297191 [details] pushed as 48f3521 - slave: drop the slaves Attachment 297192 [details] pushed as 804f778 - display: drop base class implementation of manage vfunc Attachment 297193 [details] pushed as e29372c - session: track login session display mode separate from user sessions Attachment 297194 [details] pushed as 911495f - session: add #ifdef ENABLE_WAYLAND_SUPPORT guard Attachment 297195 [details] pushed as ad9b9ff - manager: add way for displays to register themselves Attachment 297196 [details] pushed as 317cae7 - session: allow display to be set after object construction Attachment 297197 [details] pushed as 0192083 - session: add session pid getter Attachment 297198 [details] pushed as fab6fe9 - session: rename on_session_{started,opened} to on_user_session_{started,opened} Attachment 297199 [details] pushed as 020601e - session: add authentication-failed signal Attachment 297200 [details] pushed as e137e14 - session: move wtmp/btmp logging to GdmManager Attachment 297201 [details] pushed as 1a13e58 - manager: rename seed session to embryonic user session Attachment 297202 [details] pushed as a3c197e - daemon: add wrapper for launching wayland session Attachment 297203 [details] pushed as d01169e - daemon: add wrapper for launching X session Attachment 297204 [details] pushed as ee9d691 - launch-environment: don't start dbus-daemon Attachment 297205 [details] pushed as d1058c3 - display: don't make d-bus object path require display number Attachment 297206 [details] pushed as 4aa775b - display: don't export so much stuff over system bus Attachment 297207 [details] pushed as 529d1aa - session: run logind managed displays through session wrappers Attachment 297208 [details] pushed as db48470 - worker: don't require --enable-wayland to support logind display mode Attachment 297209 [details] pushed as 553c74a - session: start user X sessions on a new VT Attachment 297210 [details] pushed as 2ba0912 - local-display-factory: don't assume display has display number Attachment 297211 [details] pushed as 05ac216 - session: start login screen using session wrappers Attachment 297212 [details] pushed as cf31b8f - local-display: don't manage automatically Attachment 297213 [details] pushed as 8a3b7cf - local-display-factory: only set is-initial on first seat0 display Attachment 297214 [details] pushed as b9ecaa2 - session: change bitfield type from gboolean to guint32 Attachment 297215 [details] pushed as 7a4e340 - session: forward is-initial from display to worker Attachment 297216 [details] pushed as 22cab0b - worker: force vt 1 for initial display Attachment 297217 [details] pushed as f66cdfc - data: drop gdm-shell.session Attachment 297218 [details] pushed as 56c9837 - data: add a gnome-shell-wayland.desktop file Attachment 297219 [details] pushed as b7dbef1 - session: plug memory leak Attachment 297220 [details] pushed as 3402c71 - session: add session-type property Attachment 297221 [details] pushed as 35719a7 - display: add new session-class property Attachment 297222 [details] pushed as 7fc253e - manager: keep track of implicitly created displays Attachment 297223 [details] pushed as ec37418 - display: only create Xauth file for legacy code paths Attachment 297224 [details] pushed as 3b66952 - display: only add user authorization if connected to display Attachment 297225 [details] pushed as c2769f0 - session: add way to get session id for specific conversation Attachment 297226 [details] pushed as cafb4c4 - launch-environment: add session-type property Attachment 297227 [details] pushed as 4dc2e07 - display: add session-type property Attachment 297228 [details] pushed as ab90bd3 - local-display-factory: use wayland by default for greeter, fallback to X11 Attachment 297229 [details] pushed as 0c30572 - local-display-factory: provide override for disabling wayland
Created attachment 297418 [details] [review] gdm-wayland-session: initialize local pointers to NULL We follow the "goto out" idiom for error handling, and so it's important that pointers cleaned up at out time, are initialized. This commit does a sweep and nullifies them across the board. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297419 [details] [review] gdm-x-session: initialize local pointers to NULL We follow the "goto out" idiom for error handling, and so it's important that pointers cleaned up at out time, are initialized. This commit does a sweep and nullifies them across the board. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297420 [details] [review] gdm-wayland-session: fix debug message It says "could not start X server" when failing to start the dbus-daemon. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297421 [details] [review] gdm-x-session: fix debug message It says "could not start X server" when failing to start the dbus-daemon. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297422 [details] [review] gdm-wayland-session: don't hardcode path to dbus-daemon It's not always in BINDIR, but it is always in the PATH, so just let the shell/execvp figure it out. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297423 [details] [review] gdm-x-session: don't hardcode path to dbus-daemon It's not always in BINDIR, but it is always in the PATH, so just let the shell/execvp figure it out. https://bugzilla.gnome.org/show_bug.cgi?id=744787
Created attachment 297456 [details] [review] launch-environment: only pass on XAUTHORITY if set It's not set in a number cases now, so trying to send it along, leads to log spew.
Created attachment 297461 [details] [review] display: contact accountsservice up front We need to know if there are any user accounts before the display is prepared, since we use that information to figure out how to prepare the display. Fixes gnome-shell tanking on start up with wayland.
Attachment 297461 [details] pushed as f783a6e - display: contact accountsservice up front
let's close this now.
Created attachment 349664 [details] [review] Drop locked down /org/gnome/desktop/session/session-name setting While rebasing on top of GDM 3.22 for Endless, I noticed this key was still being locked down after having been removed from the defaults settings file in [1] and, for some reason I still don't quite fully understand, that caused GDM fail to start on Endless until I removed it from the locks file. With my fairly limited understanding of how all this works, I believe that removing this key from the locks file probably makes sense, as it's not being pre-set anymore since [1], so I'm applying that on Endless and proposing the same patch upstream. [1] https://git.gnome.org/browse/gdm/commit/?id=f66cdfcb29482bb3267f19de7c57394fe8189f39
Review of attachment 349664 [details] [review]: i don't understand why this could be causing problems for you, but it's a legitimate clean up so push away!
Thanks, pushed: https://git.gnome.org/browse/gdm/commit/?id=9fb36b5bef44bfe4aa1dda52196e08480638ce35