After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 724604 - Replace mutter-launch with logind integration
Replace mutter-launch with logind integration
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-18 03:09 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2014-12-29 05:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Replace mutter-launch with logind integration (52.58 KB, patch)
2014-02-18 03:09 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2014-02-18 03:09:41 UTC
This uses David Herrmann's new logind sessions interface to retrieve
fds for input devices, rather than using a custom setuid helper to do
the management. This vastly simplifies the interface.

This does require systemd from git, as systemd v209 has not been
released yet.
Comment 1 Jasper St. Pierre (not reading bugmail) 2014-02-18 03:09:44 UTC
Created attachment 269500 [details] [review]
Replace mutter-launch with logind integration
Comment 2 Giovanni Campagna 2014-02-18 03:27:13 UTC
Review of attachment 269500 [details] [review]:

::: src/wayland/meta-login1.c
@@ +61,3 @@
+
+  clutter_evdev_reclaim_devices ();
+}

You're killing all the logic for not running the main loop here, so we keep redrawing and failing because we're not drm master.

@@ +87,3 @@
+  MetaLogin1 *self = user_data;
+  sync_active (self);
+}

Are you sure this is never called twice with the same value? Is it enforced by logind or gdbus?

@@ +111,3 @@
+                                             cancellable,
+                                             error))
+    goto out;

Are we sure fd_list is initialized on error?

@@ +212,3 @@
+                              GError     **error)
+{
+  if (!login1_session_call_activate_sync (self->session_proxy, NULL, error))

What are the semantics for Activate()? Doesn't it require to release all devices before returning, in which case it needs to be async?
(This is from memory, but maybe I'm wrong)
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-02-18 16:52:19 UTC
(In reply to comment #2)
> Review of attachment 269500 [details] [review]:
> 
> You're killing all the logic for not running the main loop here, so we keep
> redrawing and failing because we're not drm master.

I talked to David Herrmann about this, and I think what he wants is to have hooks inside Cogl to pause drawing. This sounds like a reasonable solution, and for now there's nothing wrong with drawing and failing, only an inefficiency.

> Are you sure this is never called twice with the same value? Is it enforced by
> logind or gdbus?

I remember checking and it is enforced by gdbus. You should probably triple-check for me though.

> What are the semantics for Activate()? Doesn't it require to release all
> devices before returning, in which case it needs to be async?
> (This is from memory, but maybe I'm wrong)

Activate(); is if we need to activate our own session, so, no? We don't have any devices to release yet.

The way I'm going to use this is to call meta_activate_session(); right as gnome-shell is ready to do the animation. gdm will fade out the login screen, start the session, and then just wait. When gnome-shell calls meta_activate_session();, we'll switch VTs and do the animation, and gdm will reset its login screen.
Comment 4 Giovanni Campagna 2014-02-18 17:00:18 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 269500 [details] [review] [details]:
> > 
> > You're killing all the logic for not running the main loop here, so we keep
> > redrawing and failing because we're not drm master.
> 
> I talked to David Herrmann about this, and I think what he wants is to have
> hooks inside Cogl to pause drawing. This sounds like a reasonable solution, and
> for now there's nothing wrong with drawing and failing, only an inefficiency.

A pretty bad one: if we fail to draw, we also fail to throttle to vblank, so we busy loop in the background.

(In reply to comment #3)
> > What are the semantics for Activate()? Doesn't it require to release all
> > devices before returning, in which case it needs to be async?
> > (This is from memory, but maybe I'm wrong)
> 
> Activate(); is if we need to activate our own session, so, no? We don't have
> any devices to release yet.

Same question for switch_vt() then.
Comment 5 Jasper St. Pierre (not reading bugmail) 2014-12-29 05:35:44 UTC
This landed a while ago.