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 666891 - Optionally use systemd for session tracking
Optionally use systemd for session tracking
Status: RESOLVED FIXED
Product: gnome-session
Classification: Core
Component: gnome-session
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Session Maintainers
Session Maintainers
Depends on:
Blocks: systemd
 
 
Reported: 2011-12-27 04:21 UTC by Matthias Clasen
Modified: 2012-02-02 14:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
unrelated bug fix (861 bytes, patch)
2011-12-27 04:22 UTC, Matthias Clasen
committed Details | Review
Drop gsm_consolekit_get_current_session_type (3.33 KB, patch)
2011-12-27 04:22 UTC, Matthias Clasen
reviewed Details | Review
Drop unused property GsmConsoleKit::is-connected (3.03 KB, patch)
2011-12-27 04:23 UTC, Matthias Clasen
reviewed Details | Review
Rename GsmConsolekit to GsmLoginTracker (35.27 KB, patch)
2011-12-27 04:24 UTC, Matthias Clasen
none Details | Review
Add a --enable-systemd configure option (1.77 KB, patch)
2011-12-27 04:24 UTC, Matthias Clasen
none Details | Review
Add a systemd implementation of GsmLoginTracker (14.54 KB, patch)
2011-12-27 04:25 UTC, Matthias Clasen
reviewed Details | Review
Don't export gsm_consolekit_get_current_session_type (3.40 KB, patch)
2012-01-05 01:34 UTC, Matthias Clasen
committed Details | Review
Drop the unused property GsmConsoleKit::is-connected (3.14 KB, patch)
2012-01-05 01:34 UTC, Matthias Clasen
committed Details | Review
Rename GsmConsolekit to GsmSystem (34.48 KB, patch)
2012-01-05 01:34 UTC, Matthias Clasen
none Details | Review
Add a --enable-systemd configure option (2.33 KB, patch)
2012-01-05 01:34 UTC, Matthias Clasen
none Details | Review
Add a systemd implementation of GsmSystem (14.25 KB, patch)
2012-01-05 01:34 UTC, Matthias Clasen
none Details | Review
Rename GsmConsolekit to GsmSystem (37.70 KB, patch)
2012-01-05 01:40 UTC, Matthias Clasen
none Details | Review
Add a --enable-systemd configure option (2.33 KB, patch)
2012-01-05 01:40 UTC, Matthias Clasen
none Details | Review
Add a systemd implementation of GsmSystem (14.25 KB, patch)
2012-01-05 01:40 UTC, Matthias Clasen
none Details | Review
Add an --enable-systemd configure option (2.46 KB, patch)
2012-01-21 04:16 UTC, Matthias Clasen
reviewed Details | Review
Add a GsmSystem interface (7.88 KB, patch)
2012-01-21 04:16 UTC, Matthias Clasen
reviewed Details | Review
Use GsmSystem instead of GsmConsolekit (9.50 KB, patch)
2012-01-21 04:16 UTC, Matthias Clasen
none Details | Review
Implement GsmSystem in GsmConsolekit (11.17 KB, patch)
2012-01-21 04:16 UTC, Matthias Clasen
none Details | Review
Add a systemd implementation of GsmSystem (17.24 KB, patch)
2012-01-21 04:16 UTC, Matthias Clasen
reviewed Details | Review
GsmConsolekit: make api more uniform (4.78 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review
Add a --enable-systemd configure option (2.47 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review
Add a GsmSystem interface (7.58 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review
Use GsmSystem instead of GsmConsolekit (10.42 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review
Implement GsmSystem in GsmConsolekit (10.70 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review
Add a systemd implementation of GsmSystem (17.32 KB, patch)
2012-01-24 23:27 UTC, Matthias Clasen
none Details | Review

Description Matthias Clasen 2011-12-27 04:21:26 UTC
systemds logind is replacing ConsoleKit (on systemd systems). Here is a patch that adds a --enable-systemd configure option and replaces gsm-consolekit by a
systemd implementation if it is specified. Only compile-tested.
Comment 1 Matthias Clasen 2011-12-27 04:22:02 UTC
Created attachment 204246 [details] [review]
unrelated bug fix
Comment 2 Matthias Clasen 2011-12-27 04:22:56 UTC
Created attachment 204247 [details] [review]
Drop gsm_consolekit_get_current_session_type
Comment 3 Matthias Clasen 2011-12-27 04:23:33 UTC
Created attachment 204248 [details] [review]
Drop unused property GsmConsoleKit::is-connected
Comment 4 Matthias Clasen 2011-12-27 04:24:11 UTC
Created attachment 204249 [details] [review]
Rename GsmConsolekit to GsmLoginTracker
Comment 5 Matthias Clasen 2011-12-27 04:24:43 UTC
Created attachment 204250 [details] [review]
Add a --enable-systemd configure option
Comment 6 Matthias Clasen 2011-12-27 04:25:13 UTC
Created attachment 204251 [details] [review]
Add a systemd implementation of GsmLoginTracker
Comment 7 Ray Strode [halfline] 2012-01-04 18:15:23 UTC
Review of attachment 204247 [details] [review]:

This should probably have a slightly different summary since you aren't dropping gsm_consolekit_get_current_session_type but making it an internel implementation detail.

::: gnome-session/gsm-consolekit.c
@@ +895,3 @@
         session_type = gsm_consolekit_get_current_session_type (consolekit);
 
+        ret = (g_strcmp0 (session_type, "LoginWindow") == 0);

maybe keep the #define but move it to the top of the file?

::: gnome-session/gsm-logout-dialog.c
@@ -253,1 @@
 

since we aren't caching the session_type anymore, are all the extra blocking dbus calls going to cause problems?
Comment 8 Ray Strode [halfline] 2012-01-04 18:38:22 UTC
Review of attachment 204248 [details] [review]:

Makes sense to me.  I was going to suggest referencing the commit where its use went away in your commit message, but it looks like it was always just cruft from the get go.
Comment 9 Ray Strode [halfline] 2012-01-04 18:43:05 UTC
Review of attachment 204249 [details] [review]:

login-tracker is a horrible name given the api.  Maybe GsmSystem or GsmConsole?
Comment 10 Ray Strode [halfline] 2012-01-04 18:46:12 UTC
Review of attachment 204250 [details] [review]:

shouldn't --with-systemd default to yes if systemd is noticed as being available?
Comment 11 Matthias Clasen 2012-01-04 19:24:11 UTC
> 
> login-tracker is a horrible name given the api.  Maybe GsmSystem or GsmConsole?

GsmConsole makes me think of something related to ttys, but GsmSystem sounds ok.

> shouldn't --with-systemd default to yes if systemd is noticed as being
> available?

No opinion.
I don't really see that default == auto buys us much, other than some extra configure cruft
Comment 12 Ray Strode [halfline] 2012-01-04 22:21:22 UTC
Review of attachment 204251 [details] [review]:

::: gnome-session/gsm-systemd.c
@@ +154,3 @@
+                                      NULL);
+        g_variant_get (res, "(o)", &manager->priv->session_path);
+        g_variant_unref (res);

Lots of synchronous call here... It's all at init time though, so I assume it's okay?

@@ +348,3 @@
+
+        can_restart = polkit_authorization_result_get_is_authorized (res) ||
+                      polkit_authorization_result_get_is_challenge (res);

so given we return true here when the user can't yet restart, but might be able to after answering some questions, maybe we should call this gsm_login_tracker_is_restart_unavailable.

@@ +402,3 @@
+        login_tracker = gsm_get_login_tracker ();
+        sd_session_get_uid (login_tracker->priv->session_id, &uid);
+        pw = getpwuid (uid);

pw can be NULL in certain cases, should probably not crash in that case.

@@ +405,3 @@
+
+        /* FIXME */
+        ret = (g_strcmp0 (pw->pw_name, "gdm") == 0);

Yea, this is really unfortunate since i don't think all distros use the same name for the gdm user.  I think logind stores the service name the session was started with, so you could probably key off that assuming it exports it (which i can't immediately confirm briefly looking through the logind code)
Comment 13 Matthias Clasen 2012-01-05 01:34:06 UTC
Created attachment 204646 [details] [review]
Don't export gsm_consolekit_get_current_session_type

We already have gsm_consolekit_is_current_session_login to
replace direct session type manipulation, so use it as the
public API, and keep the session type private.
Comment 14 Matthias Clasen 2012-01-05 01:34:09 UTC
Created attachment 204647 [details] [review]
Drop the unused property GsmConsoleKit::is-connected

Nothing has ever used this property.
Comment 15 Matthias Clasen 2012-01-05 01:34:12 UTC
Created attachment 204648 [details] [review]
Rename GsmConsolekit to GsmSystem

The next commit will add a systemd-based implementation of this
api, and the name GsmConsolekit would be misleading then.
Comment 16 Matthias Clasen 2012-01-05 01:34:14 UTC
Created attachment 204649 [details] [review]
Add a --enable-systemd configure option
Comment 17 Matthias Clasen 2012-01-05 01:34:17 UTC
Created attachment 204650 [details] [review]
Add a systemd implementation of GsmSystem
Comment 18 Matthias Clasen 2012-01-05 01:40:39 UTC
Created attachment 204651 [details] [review]
Rename GsmConsolekit to GsmSystem

The next commit will add a systemd-based implementation of this
api, and the name GsmConsolekit would be misleading then.
Comment 19 Matthias Clasen 2012-01-05 01:40:41 UTC
Created attachment 204652 [details] [review]
Add a --enable-systemd configure option
Comment 20 Matthias Clasen 2012-01-05 01:40:43 UTC
Created attachment 204653 [details] [review]
Add a systemd implementation of GsmSystem
Comment 21 Vincent Untz 2012-01-16 12:56:59 UTC
Comment on attachment 204246 [details] [review]
unrelated bug fix

Thanks, pushed.
Comment 22 Vincent Untz 2012-01-16 12:59:34 UTC
Comment on attachment 204646 [details] [review]
Don't export gsm_consolekit_get_current_session_type

Thanks, pushed.
Comment 23 Vincent Untz 2012-01-16 13:03:57 UTC
Comment on attachment 204647 [details] [review]
Drop the unused property GsmConsoleKit::is-connected

Thanks, pushed after also removing the override of get_property.
Comment 24 Vincent Untz 2012-01-16 13:13:41 UTC
For the remaining bits: I'd prefer to have GsmSystem an interface implemented by both gsm-systemd and gsm-consolekit. This would enable a runtime detection of whether the system is running systemd or not.

Arguably, this is useless if you don't build with systemd support. However, if you build with systemd support, you might still be booting on a system using sysvinit -- if systemd is available, but not as default.
Comment 25 Matthias Clasen 2012-01-16 13:22:01 UTC
I think runtime detection is a fringe feature that I really don't want to invest time and bugs in. An OS can really only have one login-tracking system...
Comment 26 Vincent Untz 2012-01-16 13:33:04 UTC
(In reply to comment #25)
> I think runtime detection is a fringe feature that I really don't want to
> invest time and bugs in.

http://0pointer.de/public/systemd-man/sd_booted.html

> An OS can really only have one login-tracking system...

It helps distro who are slowly switching to systemd, though...
Comment 27 Matthias Clasen 2012-01-17 14:30:45 UTC
Even if you get me to do the extra work here, there's other modules which just have an --enable-systemd configure option; you'll have to take care of those yourself...
Comment 28 Matthias Clasen 2012-01-21 04:16:26 UTC
Created attachment 205745 [details] [review]
Add an --enable-systemd configure option

This will be used in the following commits to implement
session tracking using systemd instead of ConsoleKit.
Comment 29 Matthias Clasen 2012-01-21 04:16:30 UTC
Created attachment 205746 [details] [review]
Add a GsmSystem interface
Comment 30 Matthias Clasen 2012-01-21 04:16:33 UTC
Created attachment 205747 [details] [review]
Use GsmSystem instead of GsmConsolekit
Comment 31 Matthias Clasen 2012-01-21 04:16:36 UTC
Created attachment 205748 [details] [review]
Implement GsmSystem in GsmConsolekit
Comment 32 Matthias Clasen 2012-01-21 04:16:39 UTC
Created attachment 205749 [details] [review]
Add a systemd implementation of GsmSystem
Comment 33 Matthias Clasen 2012-01-21 04:18:37 UTC
There you go
Comment 34 Ray Strode [halfline] 2012-01-23 22:28:49 UTC
Review of attachment 205745 [details] [review]:

s/the following/follow up/ or s/the following/subsequent/

::: configure.ac
@@ +79,3 @@
+              AS_HELP_STRING([--enable-systemd], [Use systemd]),
+              [with_systemd=$enableval],
+              [with_systemd=auto])

I think it's more customary to use enable_foo=$enableval for AC_ARG_ENABLE and with_foo=$withval for AC_ARG_WITH.  You're kind of mixing the two.  I guess it doesn't really matter in practice.
Comment 35 Ray Strode [halfline] 2012-01-23 22:35:00 UTC
Review of attachment 205746 [details] [review]:

::: gnome-session/gsm-system.c
@@ +24,3 @@
+#include <glib/gi18n.h>
+
+#include "gsm-marshal.h"

shouldn't be needed, yea?

@@ +101,3 @@
+
+gboolean
+gsm_system_is_current_session_login (void)

Why is this one different than the rest (takes no system object, has a mismatched name for the vfunc) ?

@@ +130,3 @@
+        GsmSystem *system;
+
+        system = NULL;

This is odd. I wouldn't expect to a see a constructor for an interface, especially one that returns NULL always.
Comment 36 Ray Strode [halfline] 2012-01-23 22:54:04 UTC
Review of attachment 205748 [details] [review]:

::: gnome-session/gsm-consolekit.c
@@ +831,3 @@
+gsm_consolekit_new (void)
+{
+        GsmConsolekit *manager;

In some places in the code its called consolekit and in others its called manager (probably for historical cut-and-paste reasons).  Changing all the names to be consistent is maybe a bit much, but it would be good if new code used the better name.

::: gnome-session/gsm-system.c
@@ +131,3 @@
         GsmSystem *system;
 
+        system = (GsmSystem *) gsm_consolekit_new ();

GSM_SYSTEM () not (GsmSystem *).  But i'm still not very fond of this function.  Maybe it could just be folded into the get function above? or at least made static.
Comment 37 Ray Strode [halfline] 2012-01-23 23:04:05 UTC
Review of attachment 205749 [details] [review]:

::: gnome-session/gsm-system.c
@@ +132,3 @@
         GsmSystem *system;
 
+        system = (GsmSystem *) gsm_systemd_new ();

(same comments as in previous patch about GSM_SYSTEM() and potentially folding this function back into its caller.)

@@ +137,3 @@
+                g_debug ("Using ConsoleKit for session tracking");
+        }
+        else {

else on same line as }

::: gnome-session/gsm-systemd.c
@@ +111,3 @@
+                           error->message);
+                g_error_free (error);
+        }

smoosh else on to this line

@@ +135,3 @@
+        manager->priv->subject = polkit_unix_session_new_for_process_sync (getpid(), NULL, NULL);
+
+        sd_pid_get_session (getpid(), &manager->priv->session_id);

space between getpid and () (both occurances)

@@ +297,3 @@
+
+        sd_session_get_seat (manager->priv->session_id, &seat);
+        ret = sd_seat_can_multi_session (seat);

This function counter-intuitive returns more than TRUE and FALSE.  Make sure you treat negative return values as FALSE.
Comment 38 Matthias Clasen 2012-01-24 18:36:57 UTC
Review of attachment 205746 [details] [review]:

::: gnome-session/gsm-system.c
@@ +102,3 @@
+gboolean
+gsm_system_is_current_session_login (void)
+{

I agree it is odd, but thats what the api was before; I could fix it up, but thats kinda orthogonal.
Comment 39 Matthias Clasen 2012-01-24 23:27:44 UTC
Created attachment 206039 [details] [review]
GsmConsolekit: make api more uniform

gsm_consolekit_is_current_session_login was called out as
'not like the others' in patch review, so change it
to gsm_consolekit_is_login_session and give it a GsmConsolekit
argument. Adjust all callers.
Comment 40 Matthias Clasen 2012-01-24 23:27:46 UTC
Created attachment 206040 [details] [review]
Add a --enable-systemd configure option

This will be used in the subsequent commits to implement
session tracking using systemd instead of ConsoleKit.
Comment 41 Matthias Clasen 2012-01-24 23:27:49 UTC
Created attachment 206041 [details] [review]
Add a GsmSystem interface
Comment 42 Matthias Clasen 2012-01-24 23:27:52 UTC
Created attachment 206042 [details] [review]
Use GsmSystem instead of GsmConsolekit
Comment 43 Matthias Clasen 2012-01-24 23:27:55 UTC
Created attachment 206043 [details] [review]
Implement GsmSystem in GsmConsolekit
Comment 44 Matthias Clasen 2012-01-24 23:27:58 UTC
Created attachment 206044 [details] [review]
Add a systemd implementation of GsmSystem
Comment 45 Matthias Clasen 2012-01-24 23:43:38 UTC
Another round, hopefully taking care of all comments.
Comment 46 Ray Strode [halfline] 2012-01-27 22:15:18 UTC
So I talked to vuntz about these on IRC.  He said he wants something like this in before the next release and doesn't mind if this lands now.

When he has time he's going to look at these changes more closely, either before they go in or after, either way.
Comment 47 Matthias Clasen 2012-01-30 17:46:42 UTC
Ok, I've pushed them now. Happy to do more tweaks as feedback come in.
Comment 48 Vincent Untz 2012-02-02 14:49:49 UTC
I just checked everything, and it's all good. Thanks a lot for reworking the patches!