Bug 147790 - RFE: ConsoleKit support in KDM (patch provided)
Summary: RFE: ConsoleKit support in KDM (patch provided)
Status: RESOLVED FIXED
Alias: None
Product: kdm
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR wishlist with 60 votes (vote)
Target Milestone: ---
Assignee: kdm bugs tracker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-11 18:37 UTC by Kevin Kofler
Modified: 2008-11-18 14:52 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
The current patch (KDE 4 version) (21.85 KB, patch)
2007-07-11 18:39 UTC, Kevin Kofler
Details
Updated consolekit-kdm patch (22.55 KB, patch)
2007-11-29 19:42 UTC, Kevin Kofler
Details
kdm consolekit support patch for 3.x series (23.00 KB, patch)
2008-01-14 16:36 UTC, Fathi Boudra
Details
kdm consolekit support patch for 3.5.8 (with Mandriva fixes) (23.79 KB, patch)
2008-01-14 17:07 UTC, Kevin Kofler
Details
kdm consolekit support patch for 4.0.0 (with Mandriva fixes) (22.58 KB, patch)
2008-01-14 17:17 UTC, Kevin Kofler
Details
KDM ConsoleKit support patch using libck-connector (against 4.0.98) (11.47 KB, patch)
2008-07-15 18:08 UTC, Kevin Kofler
Details
KDM ConsoleKit support patch using libck-connector (against 4.0.98, actually tested) (11.50 KB, patch)
2008-07-16 16:24 UTC, Kevin Kofler
Details
KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2) (10.49 KB, patch)
2008-11-07 01:42 UTC, Kevin Kofler
Details
KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2) (10.49 KB, patch)
2008-11-07 02:06 UTC, Kevin Kofler
Details
KDM ConsoleKit support patch using libck-connector (against trunk/4.1.2, updated) (11.12 KB, patch)
2008-11-08 18:03 UTC, Kevin Kofler
Details
fixed partial patch (3.70 KB, patch)
2008-11-13 21:05 UTC, Oswald Buddenhagen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Kofler 2007-07-11 18:37:44 UTC
Version:           KDE 3.91.0 (using KDE Devel)
Installed from:    Compiled sources
Compiler:          gcc-c++-4.1.2-12 Fedora 7
OS:                Linux

Recent GNU/Linux distributions, such as Fedora 7, are using a tool called ConsoleKit to keep track of logged-in users. When built with ConsoleKit support, HAL grants or rejects device access privileges based on the presence or absence of a valid ConsoleKit cookie for a local session. Therefore, KDM needs to register itself with ConsoleKit for things like USB automounting to work in current distributions.

See also: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228111

Fedora currently ships a patch I did against KDE 3.5.6/3.5.7 which adds this support. (Thanks also go to S.Çağlar Onur who fixed a segfault in my original patch.) I now ported the patch to 3.91.0 (I'll rediff against the trunk if it doesn't apply cleanly) and am seeking for upstream inclusion into 4.0.

What the patch contains:
* functions to communicate with ConsoleKit using the low-level D-Bus interface lifted from GDM (under the GPL, by William Jon McCann) and ported to work with KDM,
* added CMake logic to detect the low-level D-Bus interface (KDE currently only checks for QtDBus) - note that the patch only needs dbus-devel to build, not ConsoleKit-devel or hal-devel,
* integration with KDM code, conditionalized under #ifdef WITH_CONSOLE_KIT.

What is currently missing is a way to turn this feature on or off with a CMake option, currently the #define WITH_CONSOLE_KIT is hardcoded in dm.h.

The KDE 3 version of the patch is being used successfully in production in Fedora 7. The KDE 4 version isn't much different and has been verified to compile, but some testing won't hurt.
Comment 1 Kevin Kofler 2007-07-11 18:39:43 UTC
Created attachment 21120 [details]
The current patch (KDE 4 version)

And here's the patch I mentioned.

I'm attaching only the KDE 4 version because KDE 3 is now in bugfix-only mode
as I understand it.
Comment 2 Kevin Kofler 2007-07-14 20:14:09 UTC
Could we have some feedback on:
* whether the feature is considered desirable for KDE in the first place and
* the current implementation?

Should I send this to a mailing list for discussion? kde-devel? kde-core-devel? Somewhere else?
Comment 3 Oswald Buddenhagen 2007-07-14 22:30:16 UTC
the integration into the existing structure sucks. ;)

anyway, the first question that comes to mind is: why not a PAM module?
if you need some information that is not available in the currently provided context, we can try to work this out.
Comment 4 Kevin Kofler 2007-07-14 22:34:28 UTC
The ConsoleKit folks say a PAM module is the wrong approach for a graphical login manager.

See for example this thread:
http://lists.freedesktop.org/archives/hal/2007-February/007363.html
In particular the post from David Zeuthen:
[snip]
> Plus, you really want your login manager to have native ConsolKit
> support (rather than a PAM module) so you can feed in more hints.. this
> PAM module is more of a stop-gap solution because the RH util-linux
> maintainer wanted it that way for /bin/login. Sure, this PAM module
> might be useful too for wdm, kdm  but I think the position of the
> ConsoleKit team (or me at least; can't speak for Jon) is that you want
> to call into ConsoleKit from your app... heck, you already have to patch
> e.g. wdm to set/unset these environment variables.
Comment 5 Oswald Buddenhagen 2007-07-14 22:49:29 UTC
i'm sort of with the util-linux guy.
it's right that some env vars need to be set (well, actually, nothing except xauthority is missing, afaict - the vt can be queried from the x server), but that might have wider use anyway. oh, right - it is said in that thread, too. the "PAM sucks too much" argumentation doesn't make sense - get in touch with the novell guys who are the (only?) maintainers and fix it. so far for stop-gap solutions ...
Comment 6 Kevin Kofler 2007-07-14 23:17:03 UTC
But what do we have to gain by setting more environment variables and writing a PAM module or patching the existing one to handle the extra variables over just calling ConsoleKit directly using D-Bus? The code to do the latter is already written, it's how GDM does it, it's how KDM in Fedora 7 does it, and I don't see what the extra indirection through a PAM module would bring us.
Comment 7 Oswald Buddenhagen 2007-07-14 23:40:47 UTC
you don't really grok the concepts of abstraction and modularity, huh? ;)
and argumentum ad populum does not work for me.
Comment 8 Kevin Kofler 2007-07-18 13:26:11 UTC
William Jon McCann has explained the rationale for the ConsoleKit design at GUADEC, here are his slides:
http://people.freedesktop.org/~hughsient/temp/guadec-multiuser.pdf
Comment 9 Rex Dieter 2007-07-18 13:51:22 UTC
Regardless, a solution is needed, and it looks like more discussion is warranted... so taking this to (something like) kde-core-devel is probably a good idea at this point.
Comment 10 Oswald Buddenhagen 2007-07-18 14:06:14 UTC
in this context, deep integration makes a lot of sense, indeed.
your patch isn't anything like that, though. it's just a poor hack that should be better done via PAM. ;)

this wish depends on bug #89023, bug #59353, bug #103046 ... sort of all the same.
Comment 11 Kevin Kofler 2007-07-18 21:57:25 UTC
The goal of my patch wasn't to implement GDM-style fast user switching in KDM, but to make KDM with its existing behavior "play nice" with the ConsoleKit infrastructure.

I did, however, port all of the ConsoleKit integration code from GDM, so the code needed to communicate with ConsoleKit for fast user switching if/when it gets implemented is already in consolekit.c (see unlock_ck_session), just not used.

Note that fast user switching probably also needs support from KScreensaver, not just KDM. (gnome-screensaver communicates with ConsoleKit to support the session locking and unlocking for fast user switching.)
Comment 12 Oswald Buddenhagen 2007-07-19 12:58:13 UTC
and this mere "playing nice" is better served via PAM. period.

if you want this patch to be a basis for anything, you have to a) fix the license and b) fix the coding style. but don't expect integration any time soon anyway.
Comment 13 Kevin Kofler 2007-07-19 13:01:58 UTC
The GPL is compatible with the X11 license and the KDM frontend is already under the GPL, so what's the problem with GPL code in the backend?
Comment 14 Oswald Buddenhagen 2007-07-19 13:09:55 UTC
the backend is supposed to stay x11 for free exchange with xdm (even though this hasn't happened for some years now).
Comment 16 Kevin Kofler 2007-07-26 21:28:11 UTC
It shall be noted that this is the next step (which is going to be taken for Fedora 8 if everything goes as planned), which probably isn't of direct relevance to KDM (it's just a move from a mix of the old pam_console setup and the new ConsoleKit+HAL setup to only the new one).

What went into Fedora 7 already (and what we're patching KDM for) is this:
http://fedoraproject.org/wiki/Releases/FeatureFastUserSwitching
(As already discussed above, we don't have GDM-style fast user switching really working in KDM yet, but we did implement the parts needed to keep HAL working in KDM sessions.)
Comment 17 Kevin Kofler 2007-11-29 19:42:54 UTC
Created attachment 22250 [details]
Updated consolekit-kdm patch

This is an updated version of the patch:
* applies against kdebase-workspace 3.96.1
* includes Mandriva's fixes for xdmcp not working, see:
https://bugzilla.redhat.com/show_bug.cgi?id=243560
http://qa.mandriva.com/show_bug.cgi?id=35502
http://www.mandriva.com/security/advisories?name=MDKA-2007:110
Comment 18 Kevin Kofler 2007-11-29 19:45:22 UTC
Also http://qa.mandriva.com/show_bug.cgi?id=34786 (35502 is just the update tracker, not the actual bug report).
Comment 19 Fathi Boudra 2008-01-14 16:36:21 UTC
Created attachment 23030 [details]
kdm consolekit support patch for 3.x series
Comment 20 Fathi Boudra 2008-01-14 16:37:24 UTC
Also http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=450603

--------

the pkg-utopia team is going to upload a hal version to experimental,
which has compiled in support for ConsoleKit/PolicyKit and we plan to
upload this version to unstable later if nothing goes awfully wrong.

This means, that hal clients in the desktop session have to be active,
in order to call hal methods ( at least for stuff like
Suspend()/Hibernate()/Mount() ).
For this to work, the login manager has to register the session on login
with ConsoleKit. Then ConsoleKit allows to track, if the desktop session
is active or not. This allows to fix many longstanding issues with
regard to fast user switching.
gdm already has support for ConsoleKit (upstream) and the attached patch
also adds ConsoleKit support to kdm. It only adds a dependency on D-Bus.
Running kdm without ConsoleKit being installed is still possible, so
it's perfectly safe to include this patch now, even if the current hal
version in unstable doesn't require it yet.
This is a heads up, to give you enough time to upload an updated kdm
version, before the CK/PK enabled hal version is uploaded to unstable.

So please consider to add this patch to your next upload.

--------

We have included the patch to our Debian packages.
It could be nice to have it included upstream too.

Comment 21 Kevin Kofler 2008-01-14 17:07:38 UTC
Created attachment 23031 [details]
kdm consolekit support patch for 3.5.8 (with Mandriva fixes)

WARNING: The patch you uploaded is outdated, in particular you're missing the
fix by Nicolas Lécureuil from Mandriva to make XDMCP work.

I'm attaching the latest version I know about, which is the current version in
Fedora CVS and includes the Mandriva fixes.
Comment 22 Kevin Kofler 2008-01-14 17:17:52 UTC
Created attachment 23032 [details]
kdm consolekit support patch for 4.0.0 (with Mandriva fixes)

And here's a version which applies against 4.0.0.
Comment 23 Fathi Boudra 2008-03-12 11:53:29 UTC
Another update related to http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=457487

--- a/kdm/backend/consolekit.c
+++ b/kdm/backend/consolekit.c
@@ -461,7 +461,7 @@
 	add_param_string (&iter_struct, "x11-display", d->name);
 	add_param_boolean (&iter_struct, "is-local", ((d->displayType & d_location) == dLocal));
 #ifdef XDMCP
-	if ((d->displayType & d_location) != dLocal) {
+	if ((d->displayType & d_location) != dLocal && d->remoteHost) {
 		add_param_string (&iter_struct, "remote-host-name", d->remoteHost);
 	}
 #endif
Comment 24 Kevin Kofler 2008-03-12 11:55:04 UTC
That change is redundant if the fixes from Mandriva have been applied.
Comment 25 Kevin Kofler 2008-03-12 11:56:40 UTC
To be more explicit, the fix from Mandriva is the proper fix, as it actually sets d->remoteHost as it's supposed to, see the xdmcp.c hunk:
+			d->remoteHost = networkAddressToHostname (pdpy->connectionType,
+			                             &pdpy->connectionAddress);
Comment 26 Modestas Vainius 2008-03-12 12:09:40 UTC
Please read Debian bug report. Debian was already using "kdm consolekit support patch for 3.5.8 (with Mandriva fixes)" and d->remoteHost could still be NULL under certain circumstances (see bug report).
Comment 27 Fathi Boudra 2008-03-12 13:45:17 UTC
we applied the patch with mandriva fixes, it seems not sufficient and resulted in the updated patch.

from the bug report:
[...]
When logging in on a remote xterminal via XDMCP kdm segfaults, causing the
session to die and the greeter to be redisplayed.

A backtrace of the crash (below) shows that we crash trying to create a
bogus DBUS entry.  Further investigation indicates the d->remoteHost is
NULL.  It looks like there are two different types of nonlocal session:
Localy manged X servers that connect to a remote XDMCP server, and remote X
terminals logging into this host via XDMCP.  remoteHost is only set in
the former case.

The patch below fixes the crash by only adding the dbus attribute if
remoteHost is known.
[...]
Comment 28 Rex Dieter 2008-05-22 21:26:23 UTC
srsly, about every major linux distro is including some variant of this patch, and that's just plain silly.

Are we still blocking on just license and coding style (Re comment #12) ?
Comment 29 Joseph Tate 2008-05-22 22:00:04 UTC
I applied a version of this patch in foresight's not yet released KDE version.  I updated it for 3.5.9.
Comment 30 Shawn Starr 2008-06-15 03:34:39 UTC
Please merge this into trunk. KDE Developers in Fedora will have serious problems because ConsoleKit is defacto now (it uses PAM) and thus No sound
Comment 31 Kevin Kofler 2008-06-15 03:37:15 UTC
s/it uses PAM/PulseAudio needs it to get access to sound devices and everything which outputs sound uses PulseAudio/
Comment 32 Kevin Kofler 2008-06-15 03:41:20 UTC
(Display managers are still expected to talk to ConsoleKit directly, not through PAM. And PAM is completely irrelevant for the sound issue, maybe you mixed up PAM with PA (PulseAudio)?)
Comment 33 Kevin Kofler 2008-06-15 04:00:15 UTC
There's also a version of the patch by Patrice Dumas which uses libck-connector instead of the consolekit.c I adapted from GDM: https://bugzilla.redhat.com/show_bug.cgi?id=430388

This is probably cleaner, and easier to properly autodetect (use ConsoleKit if and only if libck-connector is there), the drawback is that the extra dependency is used instead of just D-Bus, but that's probably not a real issue. The real issue is that that version of the patch hasn't had much testing yet.
Comment 34 Kevin Kofler 2008-07-03 02:09:06 UTC
One advantage of using libck-connector is that there's no longer the GPLed consolekit.c, and libck-connector is licensed under the MIT X11 license, so Ossi's licensing concerns should be dealt with by that solution.
Comment 35 Kevin Kofler 2008-07-15 18:08:09 UTC
Created attachment 26151 [details]
KDM ConsoleKit support patch using libck-connector (against 4.0.98)

I have cleaned up the version of the patch using libck-connector, and Rex
Dieter has tested it. I also applied the fix by the Debian folks (from
comment #23).

This patch:
* automatically detects whether to enable ConsoleKit support: it is enabled if
and only if libck-connector is detected (it can be overridden with
-DWITH_CkConnector=off, as I'm using macro_optional_find_package).
* uses macro_log_feature to report on the above.
* contains no GPL code, it's all X11-licensed.
* doesn't duplicate ConsoleKit interfacing code, reuses the (X11-licensed)
ck-connector library instead.

Therefore, I think this patch should be acceptable for upstream merging. Please
take into account that most, if not all, major distributions are using some
variant of this patch (or the older revision which used the low-level D-Bus
interface only and had the ConsoleKit interfacing code copied from GDM) and
that vanilla KDE no longer works properly on all those distributions, because
ConsoleKit registration is required for several core mechanisms (e.g. HAL,
PulseAudio, ...) to work.
Comment 36 Kevin Kofler 2008-07-15 18:12:33 UTC
(The patch is against 4.0.98, it should apply against both 4.1 branch and 4.2 trunk, probably 4.0 too. But by the freeze policies, I think only 4.2 can be considered for upstream merging.)
Comment 37 Kevin Kofler 2008-07-15 18:58:08 UTC
And I almost forgot: Thanks to Patrice Dumas for the original revision of the libck-connector-based version of the patch!
Comment 38 Oswald Buddenhagen 2008-07-15 23:18:32 UTC
my very first sentence still applies. ;)
look at the way pam is integrated; anything else is just wrong.

the coding style does not match. i will fix that myself if you insist.
Comment 39 Kevin Kofler 2008-07-16 16:24:15 UTC
Created attachment 26177 [details]
KDM ConsoleKit support patch using libck-connector (against 4.0.98, actually tested)

Oops, we accidentally tested an old version. (Sorry, we really thought we had
tested the new one successfully, but it turns out we didn't, oops.) This
version is actually tested (I swear!), I fixed 2 more bugs (a segfault and an
incorrect parameter name which caused ConsoleKit not to give out the device
permissions), now it's working fine.

> the coding style does not match. i will fix that myself if you insist

That would be very helpful indeed, because I'm not sure I can find all the
coding style mismatches. :-(

And yes, I insist that the patch is needed. :-)
Comment 40 Oswald Buddenhagen 2008-07-16 17:07:21 UTC
> And yes, I insist that the patch is needed. :-) 
>
i don't doubt that. ;)

you're not listening properly, though. the bottom line is, if you are touching session.c, you are doing something wrong.

btw, the WIN32 test in FindCKConnector seems pretty pointless ...
Comment 41 Kevin Kofler 2008-11-07 01:42:55 UTC
Created attachment 28377 [details]
KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2)

So here's a version of the patch which does not touch session.c at all, but only client.c (and a one-line fix to xdmcp.c), as requested. I also made some minor adjustments to match the KDM backend's coding style. The behavior should be strictly equivalent to the previous version of the patch.

The diff was made against 4.1.2, but I verified that it also applies cleanly (no rejects, no fuzz, even no offsets) against the current trunk.

I have tested it in a Fedora kdebase-workspace-4.1.2 RPM (I replaced the previous ConsoleKit patch by this one), it works (builds and runs) fine (as confirmed by ConsoleKit's ck-list-sessions tool).
Comment 42 Kevin Kofler 2008-11-07 02:06:59 UTC
Created attachment 28378 [details]
KDM ConsoleKit support patch using libck-connector (against trunk or 4.1.2)

Same as above, but without the -Wwrite-strings warning (just a s/char \*remote_host_name/const char *remote_host_name/). ;-)

The patch now introduces no warnings.
Comment 43 Oswald Buddenhagen 2008-11-07 16:10:53 UTC
now we are getting somewhere ... ;)

- the dbus check should use pkgconfig, too
- extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code in verify()
- the if(p) in hunk 1216+ makes no sense. basically for the same reason as above.
- !is_local && !td->remoteHost is most likely insatisfiable, too. pls check.
- afaik, the generic error messages should not mention dbus-induced failures, as those are guaranteed to return a proper error code. btw, doesn't the connector provide some function to stringify its own error messages? can there be non-dbus errors at all?
- camelCase is preferred to underscore_madness ;)
  there are some other minor stylistic issues which are harder to explain than to fix, so i'll do that myself prior to checkin.
Comment 44 Kevin Kofler 2008-11-07 17:22:15 UTC
> - the dbus check should use pkgconfig, too

IIRC, I didn't use it because the 2 include dirs (DBUS_INCLUDE_DIR and DBUS_ARCH_INCLUDE_DIR) made the default cmake pkgconfig macros fail and so I followed the path of least resistance, but there must be a way to use pkgconfig anyway, I'm rechecking, I'll change it if I can.

> - extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code
in verify()

Uh, I don't understand, the pwent is needed by both the PAM and AIX code and the ConsoleKit code so why should I be computing the exact same pwent twice?

I can easily change it, but I don't understand what's wrong with the way I implemented it.

> - the if(p) in hunk 1216+ makes no sense. basically for the same reason as
above.

See above.

> - !is_local && !td->remoteHost is most likely insatisfiable, too. pls check.

That's what I thought too, but the Debian folks assured me otherwise, see comment #23 and comment #27.

> - afaik, the generic error messages should not mention dbus-induced failures,
> as those are guaranteed to return a proper error code.

So should I just change it to a generic "Cannot close CK session\n"?

> btw, doesn't the connector provide some function to stringify its own error
> messages?

Unfortunately no, it doesn't.

> can there be non-dbus errors at all?

I don't know, so I have to assume the worst.

> - camelCase is preferred to underscore_madness ;)

Uh, sorry, I tried to match the surrounding code, which uses underscores in a lot of places. :-) But I'm going to change my variable names to camelCase.
Comment 45 Kevin Kofler 2008-11-07 17:25:34 UTC
Oops, I understand now what you mean by those:
> - extending the #ifdef in hunk 1207+ is wrong. it must be complementary to code
> in verify()
> - the if(p) in hunk 1216+ makes no sense. basically for the same reason as
> above.

(If not PAM nor AIX, p is already computed by verify, so I'm actually computing it twice, and if (p) is always satisfied, right?)

/me needs more sleep, sorry. :-)

I'll fix this.
Comment 46 Oswald Buddenhagen 2008-11-07 18:26:02 UTC
> (If not PAM nor AIX, p is already computed by verify, so I'm actually computing
> it twice, and if (p) is always satisfied, right?)
> 
yup :)

> the Debian folks assured me otherwise, see comment #23 and comment #27.
> 
hmm. i wonder whether any of these cases is actually legitimate. "foreign" and "local feedback" (Xnest -query localhost) displays should be investigated, and the findings documented or the other code fixed.

> So should I just change it to a generic "Cannot close CK session\n"?
> 
sounds kind of reasonable.

> I don't know, so I have to assume the worst.
> 
i'd seek some definite clarification on that.

> I tried to match the surrounding code, which uses underscores in a
> lot of places. :-)
> 
yeah, the naming is a mess. i'm trying to standardize incrementally.
Comment 47 Kevin Kofler 2008-11-08 18:03:00 UTC
Created attachment 28418 [details]
KDM ConsoleKit support patch using libck-connector (against trunk/4.1.2, updated)

Here's a version of the patch addressing your comments:
* FindDBus.cmake now uses pkg-config, through the new FindPkgConfig. This also solves the problem with D-Bus's 2 include directories.
* I also changed FindCkConnector.cmake to use FindPkgConfig instead of the old UsePkgConfig (also avoids having to hardcode the "/ConsoleKit/ck-connector" part of the includedir).
* I removed the #ifdef extension (which was computing p a second time when verify() already did it) and the useless if (p) check.
* I added a comment for the seemingly-redundant !is_local && td->remoteHost check pointing to the relevant Debian bug report.
* The fallback error messages for when dbus_error_is_set is false for some reason are now less verbose.
* I renamed all the variable names I added to camelCase. (Of course I can't change the names used by D-Bus and libck-connector nor pwent's pw_uid.)

Tested, builds, runs, registers CK session successfully.
Comment 48 Kevin Kofler 2008-11-11 15:09:31 UTC
Ping? The 4.2 hard feature freeze is approaching, we need this merged before. (And yes, it's in the 4.2 feature plan, Rex Dieter added it.)
Comment 49 Oswald Buddenhagen 2008-11-13 21:05:08 UTC
Created attachment 28549 [details]
fixed partial patch

i knew what i'm getting into, so i procrastinated. ;)
i knew that the remoteHost stuff is fishy, just not how much. :}
this patch compiles, but is otherwise untested. have fun with it.
the case that would legitimately use remoteHost (the local chooser mode, "remote login" in the greeter) needs to be handled completely separately (yes, outside client.c this time ;).
Comment 50 Kevin Kofler 2008-11-13 21:17:36 UTC
So can we please commit it now (with your changes)? I really don't want to miss the freeze and get told it now has to wait until 4.3!

And it would be helpful if you explained what exactly was wrong with the way remoteHost was handled. Because "fishy" is a bit vague. ;-)
Comment 51 Oswald Buddenhagen 2008-11-13 21:34:39 UTC
stop panicking. this is clearly a bugfix, right? ;)

everything is wrong with the remoteHost usage. grep for it, get a grip of what it is meant to do (you'll need that to implement the second part anyway).
Comment 52 Kevin Kofler 2008-11-14 03:18:09 UTC
But can we merge the current code now? The fact that this:
> the case that would legitimately use remoteHost (the local chooser mode,
> "remote login" in the greeter) needs to be handled completely separately (yes,
> outside client.c this time ;).
is missing isn't really a blocker bug, is it?
Comment 53 Kevin Kofler 2008-11-14 03:43:36 UTC
Oh, and for sessions where KDM is local and the session is remote (is this what remoteHost is used for? Looking at the code and your comment #49, I think it is, but I'm not 100% sure I understand), ConsoleKit probably doesn't really need to know about that session anyway. ConsoleKit is used to enumerate sessions on the local machine to give out permissions to those sessions (e.g. device permissions and PolicyKit-controlled application permissions), so as far as I know sessions which are actually on the remote machine aren't really interesting to ConsoleKit anyway.
Comment 54 Oswald Buddenhagen 2008-11-17 00:45:25 UTC
> ConsoleKit probably doesn't really need to know about that session anyway.
>
maybe. it could be useful for fast user switching ... if there was a way to switch back to a local session. maybe some day.

if you want me to commit, you have to say whether my patch works. ;)
Comment 55 Kevin Kofler 2008-11-18 06:00:45 UTC
Hmmm, to really test this, we'll need some XDMCP setup. I can have a try later today or tomorrow with my 2 machines (I have stuff for the university to do right now), hopefully that'll allow some testing. But I won't complain if somebody with more experience with XDMCP helps test this. ;-)

(And I'm worried that the release team will kill us if we sneak this into 4.2 now, we're already late. ;-( )
Comment 56 Oswald Buddenhagen 2008-11-18 14:52:32 UTC
SVN commit 886022 by ossi:

add rudimentary support for ConsoleKit.
this is quite a crutch - it is so poorly integrated that one could just
as well do it with PAM (if the respective module wasn't that braindead).
BUG: 147790


 A             cmake/modules/FindCkConnector.cmake  
 A             cmake/modules/FindDBus.cmake  
 M  +11 -0     kdm/CMakeLists.txt  
 M  +4 -0      kdm/backend/CMakeLists.txt  
 M  +96 -0     kdm/backend/client.c  
 M  +3 -0      kdm/config-kdm.h.cmake  


WebSVN link: http://websvn.kde.org/?view=rev&revision=886022