Created attachment 204246 [details] [review]
unrelated bug fix

Created attachment 204247 [details] [review]
Drop gsm_consolekit_get_current_session_type

Created attachment 204248 [details] [review]
Drop unused property GsmConsoleKit::is-connected

Created attachment 204249 [details] [review]
Rename GsmConsolekit to GsmLoginTracker

Created attachment 204250 [details] [review]
Add a --enable-systemd configure option

Created attachment 204251 [details] [review]
Add a systemd implementation of GsmLoginTracker

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?

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.

Review of attachment 204249 [details] [review]:

login-tracker is a horrible name given the api.  Maybe GsmSystem or GsmConsole?

Review of attachment 204250 [details] [review]:

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

> 
> 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

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)

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.

Created attachment 204647 [details] [review]
Drop the unused property GsmConsoleKit::is-connected

Nothing has ever used this property.

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.

Created attachment 204649 [details] [review]
Add a --enable-systemd configure option

Created attachment 204650 [details] [review]
Add a systemd implementation of GsmSystem

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.

Created attachment 204652 [details] [review]
Add a --enable-systemd configure option

Created attachment 204653 [details] [review]
Add a systemd implementation of GsmSystem

Comment on attachment 204246 [details] [review]
unrelated bug fix

Thanks, pushed.

Comment on attachment 204646 [details] [review]
Don't export gsm_consolekit_get_current_session_type

Thanks, pushed.

Comment on attachment 204647 [details] [review]
Drop the unused property GsmConsoleKit::is-connected

Thanks, pushed after also removing the override of get_property.

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.

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...

(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...

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...

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.

Created attachment 205746 [details] [review]
Add a GsmSystem interface

Created attachment 205747 [details] [review]
Use GsmSystem instead of GsmConsolekit

Created attachment 205748 [details] [review]
Implement GsmSystem in GsmConsolekit

Created attachment 205749 [details] [review]
Add a systemd implementation of GsmSystem

There you go

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.

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.

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.

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.

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.

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.

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.

Created attachment 206041 [details] [review]
Add a GsmSystem interface

Created attachment 206042 [details] [review]
Use GsmSystem instead of GsmConsolekit

Created attachment 206043 [details] [review]
Implement GsmSystem in GsmConsolekit

Created attachment 206044 [details] [review]
Add a systemd implementation of GsmSystem

Another round, hopefully taking care of all comments.

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.

Ok, I've pushed them now. Happy to do more tweaks as feedback come in.

I just checked everything, and it's all good. Thanks a lot for reworking the patches!