Created attachment 11486 [details] [review]
proposed patch

Newer CK info here:

http://www.freedesktop.org/wiki/Software/ConsoleKit

Created attachment 11489 [details] [review]
Updated patch against current git with some autotools polish

(In reply to comment #3)
> Created an attachment (id=11489) [details]
> Updated patch against current git with some autotools polish
> 
shouldn't you also check for dbus-1 and include <dbus/dbus.h>, since you're using dbus functions?

@jcristau: indeed, makes sense; it will work as is because ck-connector requires dbus in the .pc.

Using this patch, it seems that the session isn't made the active one; after discussion with other people, one thing which might help is that ck_connector_open_session_with_parameters should be used instead of ck_connector_open_session with some parameters from:
display-device
x11-display-device
x11-display

(probably session-type, is-local, and unix-user can be set easily as well)

(In reply to comment #0)
> (not sure I've filed this against the right component, please advise)
> 
> ConsoleKit [1] is used to track user desktop sessions so to work with
> ConsoleKit any process used to launch these needs to poke ConsoleKit so the
> session can be registered. Will attach a patch to xinit.c that does this.
> 
> [1] : 
> http://gitweb.freedesktop.org/?p=ConsoleKit.git
> http://people.freedesktop.org/~david/ConsoleKit.html (a bit outdated)
> 
> Btw, this patch originated from this RH bug
> https://bugzilla.redhat.com/show_bug.cgi?id=233183
> 

(In reply to comment #5)
> @jcristau: indeed, makes sense; it will work as is because ck-connector
> requires dbus in the .pc.
> 
> Using this patch, it seems that the session isn't made the active one; after
> discussion with other people, one thing which might help is that
> ck_connector_open_session_with_parameters should be used instead of
> ck_connector_open_session with some parameters from:
> display-device
> x11-display-device
> x11-display
> 
> (probably session-type, is-local, and unix-user can be set easily as well)
> 

Having done more research on this and actually tried to update this patch to use the above mentioned call (open_session_with_parameters), this will not work.

Typically xinit is not run as a root, so the user does not have enough permissions to create a session with parameters. 

I question weather or not we should try to move this call into X?  X is setuid root.
 

Launching the session from xinit wont work and is actually harmful; instead we should start the ck session as an encapsulating client for the lifetime of the session; for example if you run your X11 client within ck-launch-session, CK should look at the DISPLAY of the calling DBus process and will provision the DISPLAY and the VT in the new CK session.

If however we were to merge this patch, then we wouldn't have any way to tell CK to update its info.

We've merged the ck-launch-session in a Xsession snippet approach in Ubuntu, and it works fine.

I think it's clean enough and doesn't add a dbus dependency to xinit; also, the current patch is insufficient (as it doesn't set x11-display etc.).

The Xsession snippet might make more sense in consolekit itself or distro packaging but I guess it should be shipped whereever the dbus-launch or ssh-agent snippets are shipped.

configure.ac needs a bit more work.  For example, if I pass --with-consolekit 
and I don't actually have it, configure should error out, but it just silently 
continues without consolekit.  Other than that, it looks good.  Mind updating 
the patch?  Feel free to email it to xorg-devel for review, or attach here and 
I'll review it again.  Bonus points if you use 'git format-patch' with a commit 
message that I can use rather than just 'git diff'

> --- Comment #9 from Jeremy Huddleston <jeremyhu@freedesktop.org> 2011-10-03 00:31:49 PDT ---
> configure.ac needs a bit more work.  For example, if I pass --with-consolekit 
> and I don't actually have it, configure should error out, but it just silently 
> continues without consolekit.  Other than that, it looks good.  Mind updating 
> the patch?  Feel free to email it to xorg-devel for review, or attach here and 
> I'll review it again.  Bonus points if you use 'git format-patch' with a commit 
> message that I can use rather than just 'git diff'
> 
Hrm.  What about the comments from Loic?

(In reply to comment #10)
> Hrm.  What about the comments from Loic?

I don't think our comments conflict.  Loic was concerned about adding dependencies, and I was suggesting making them soft rather than hard dependencies.

The only real concern that I see is Michael's:

> Typically xinit is not run as a root, so the user does not have enough
> permissions to create a session with parameters. 
> I question weather or not we should try to move this call into X?  X is setuid
> root.

I admit that I did not read that comment the first time through as it was collapsed in my view of the bug.  On darwin, we trigger a LaunchDaemon in startx to do some privileged actions for xinit (like creating /tmp/.X11-unix).  How is this currently handled on other OSs?  Still as an init script?

> --- Comment #11 from Jeremy Huddleston <jeremyhu@freedesktop.org> 2011-10-04 11:53:02 PDT ---
> (In reply to comment #10)
> > Hrm.  What about the comments from Loic?
> 
> I don't think our comments conflict.  Loic was concerned about adding
> dependencies, and I was suggesting making them soft rather than hard
> dependencies.
> 
Comment #7 and comment #9 sound conflicting to me, but ok.

> The only real concern that I see is Michael's:
> 
> > Typically xinit is not run as a root, so the user does not have enough
> > permissions to create a session with parameters. 
> > I question weather or not we should try to move this call into X?  X is setuid
> > root.
> 
> I admit that I did not read that comment the first time through as it was
> collapsed in my view of the bug.  On darwin, we trigger a LaunchDaemon in
> startx to do some privileged actions for xinit (like creating /tmp/.X11-unix). 
> How is this currently handled on other OSs?  Still as an init script?
> 
At least on Debian, yes, /tmp/.X11-unix is created on boot by an init
script.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/app/xinit/issues/1.