Bug 430388 - patch for libck-connector integration
patch for libck-connector integration
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: kdebase-workspace (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Kevin Kofler
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-27 06:11 EST by Patrice Dumas
Modified: 2008-07-16 10:29 EDT (History)
5 users (show)

See Also:
Fixed In Version: 4.0.98-5.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-14 22:10:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
use ck-connector to start the consolekit session (9.34 KB, patch)
2008-01-27 06:12 EST, Patrice Dumas
no flags Details | Diff
kdebase-workspace-4.0.98-consolekit-kdm.patch (11.48 KB, patch)
2008-07-14 18:57 EDT, Kevin Kofler
no flags Details | Diff

  None (edit)
Description Patrice Dumas 2008-01-27 06:11:22 EST
Description of problem:

I attach a patch for consolekit support using ck-connector.
This should remove the issue of license compatibility and code
duplication. There is a segfault with this patch, but I 
don't know how to obtain a backtrace. I did it by using
code from  
kdebase-workspace-4.0.0-consolekit-kdm.patch and 
inspiration and trivial code from
ConsoleKit-0.2.6/libck-connector/test-connector.c

It should certainly be cleaned by someone more knowledgable
in kdm.

As a side note developping with kdm is a pain since the package
takes very long to compile, install and remove and starting
in-source kdm doesn't work. Splitting kdm would, in my opinion 
be much better (splitting a sub-package may be good too, but 
source splitting is more the issue, in my opinion).

To use it replace kdebase-workspace-4.0.0-consolekit-kdm.patch
with that patch.

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 Patrice Dumas 2008-01-27 06:12:46 EST
Created attachment 293071 [details]
use ck-connector to start the consolekit session
Comment 2 Patrice Dumas 2008-01-27 06:14:04 EST
As a side note, I still wouldn't accept the patch if I was 
the kdm maintainer, since it hardcodes /dev/tty{vt} as device.
Comment 3 Patrice Dumas 2008-03-23 12:32:22 EDT
That patch doesn't interest you?
Comment 4 Kevin Kofler 2008-03-23 14:08:15 EDT
AFAIK, upstream is planning to include my patch which only uses dbus-devel 
into KDE 4.1.
Comment 5 Patrice Dumas 2008-03-23 15:03:15 EDT
Right then.
Comment 6 Kevin Kofler 2008-06-14 21:56:42 EDT
Actually the patch is still not in upstream 4.1, so using libck-connector might 
actually be of interest.
Comment 7 Patrice Dumas 2008-07-12 05:14:14 EDT
You may also want to have a look at Bug 452156, the fix here may allow 
to start kdm in a consolekit session without actually requiring to
patch kdm.
Comment 8 Kevin Kofler 2008-07-12 12:03:05 EDT
I just added a comment there:
> [The solution from bug 452156] looks like the wrong solution to me, Xsession
> doesn't have the information the display manager has, so ConsoleKit will get
> provided with incomplete information if you do it that way.
Comment 9 Kevin Kofler 2008-07-13 19:34:35 EDT
I looked at your patch, also comparing it with mine, and I have a few 
questions:

+string(REGEX REPLACE ";" " " 
CKCONNECTOR_CFLAGS_STRING "${CKCONNECTOR_CFLAGS}")
+set_source_files_properties (session.c PROPERTIES COMPILE_FLAGS
+   "${CKCONNECTOR_CFLAGS_STRING}")

Normally, you aren't supposed to just convert any and all CFLAGS returned by 
pkgconfig that way, what all those other Find*.cmake modules which use 
pkgconfig do is to use the include directories only. For example, 
FindGLIB2.cmake has this call:
pkgconfig(glib-2.0 _LibGLIB2IncDir _LibGLIB2LinkDir _LibGLIB2LinkFlags 
_LibGLIB2Cflags)
and _LibGLIB2Cflags is simply thrown away and never used. Thus my question: Are 
any of the CFLAGS returned by libck-connector really needed?

I'm also not sure ConfigureChecks.cmake is the right place for the 
pkg_check_modules(CKCONNECTOR ck-connector) call, it probably belongs to a 
FindCkConnector.cmake (which would look similar to the FindDBus.cmake I used).

+	if (pwent == NULL) {
+		debug ("NULL user\n");
+		sessionExit( EX_NORMAL );
+	}

This cancels the entire session with an error if pwent is NULL. Are you sure 
that's the right thing to do? What the original code did is that it simply 
skipped the ConsoleKit registration and returned a NULL cookie, and AFAIK 
that's needed to make XDMCP work. (I'm not sure of the details, as that was one 
of the fixes the Mandriva folks did to my code, but I guess pwent is NULL on 
the client if you're logging in to a remote DM, and in that case you don't want 
to do the ConsoleKit registration on the client, only on the server where the 
session is actually located, but you do want to continue the session.)
Comment 10 Patrice Dumas 2008-07-13 19:50:00 EDT
(In reply to comment #9)
> I looked at your patch, also comparing it with mine, and I have a few 
> questions:
> 
> +string(REGEX REPLACE ";" " " 
> CKCONNECTOR_CFLAGS_STRING "${CKCONNECTOR_CFLAGS}")
> +set_source_files_properties (session.c PROPERTIES COMPILE_FLAGS
> +   "${CKCONNECTOR_CFLAGS_STRING}")
> 
> Normally, you aren't supposed to just convert any and all CFLAGS returned by 
> pkgconfig that way, what all those other Find*.cmake modules which use 
> pkgconfig do is to use the include directories only. For example, 
> FindGLIB2.cmake has this call:
> pkgconfig(glib-2.0 _LibGLIB2IncDir _LibGLIB2LinkDir _LibGLIB2LinkFlags 
> _LibGLIB2Cflags)
> and _LibGLIB2Cflags is simply thrown away and never used. 

That looks wrong to me. Why would the LibGLIB2Cflags be thrown away?
Anyway you can completly ignore what I did for cmake, I am a newbie
and I remember that I had much trouble, so I certainly did something 
wrong.

> Thus my question: Are 
> any of the CFLAGS returned by libck-connector really needed?

There is nothing returned besides an include directory.

> I'm also not sure ConfigureChecks.cmake is the right place for the 
> pkg_check_modules(CKCONNECTOR ck-connector) call, it probably belongs to a 
> FindCkConnector.cmake (which would look similar to the FindDBus.cmake I used).

You are certainly right.

> +	if (pwent == NULL) {
> +		debug ("NULL user\n");
> +		sessionExit( EX_NORMAL );
> +	}
> 
> This cancels the entire session with an error if pwent is NULL. Are you sure 
> that's the right thing to do? What the original code did is that it simply 
> skipped the ConsoleKit registration and returned a NULL cookie, and AFAIK 
> that's needed to make XDMCP work. (I'm not sure of the details, as that was one 
> of the fixes the Mandriva folks did to my code, but I guess pwent is NULL on 
> the client if you're logging in to a remote DM, and in that case you don't want 
> to do the ConsoleKit registration on the client, only on the server where the 
> session is actually located, but you do want to continue the session.)

Indeed, looks like it can be ignored for xdmcp, but I don't think
that a getpwent error should be ignored for local login.
Comment 11 Kevin Kofler 2008-07-14 18:57:37 EDT
Created attachment 311784 [details]
kdebase-workspace-4.0.98-consolekit-kdm.patch

Here's the version of the patch I'm going to test:
* cleaned up the CMake machinery to look more like elsewhere in KDE (use things
like macro_optional_find_package and macro_log_feature, use pkg-config the same
way it is used everywhere else)
* don't exit if pwent == NULL, just skip the ConsoleKit registration
* finally apply the fix from Debian as per
http://bugs.kde.org/show_bug.cgi?id=147790#c23 (the equivalent code is in
session.c, not consolekit.c, in the current patch)
* don't exit if both device and remotehost are empty (but I'm not sure whether
this is a good idea, to be honest I don't know if it can happen nor what the
proper way to react is if it does)
Comment 12 Kevin Kofler 2008-07-14 21:44:39 EDT
Comment on attachment 311784 [details]
kdebase-workspace-4.0.98-consolekit-kdm.patch

Current patch here:
http://cvs.fedoraproject.org/viewcvs/rpms/kdebase-workspace/devel/kdebase-works
pace-4.0.98-consolekit-kdm.patch?rev=1.2&view=markup

* fixed build failure in session.c when ck-connector is not detected
* fixed detection of CKCONNECTOR_INCLUDE_DIR
Comment 13 Kevin Kofler 2008-07-14 22:14:05 EDT
Now built in Rawhide, but if this still segfaults, we'll have to untag it.
Comment 14 Kevin Kofler 2008-07-14 22:36:16 EDT
New patch:
http://cvs.fedoraproject.org/viewcvs/rpms/kdebase-workspace/devel/kdebase-workspace-4.0.98-consolekit-kdm.patch?rev=1.3&view=markup

Fixes a bug which almost certainly caused a segfault (maybe the one you 
mentioned in your original post?): is_local must be passed by pointer, not by 
value. I also simplified the code for remote_host_name a bit while I was at it.
Comment 15 Rex Dieter 2008-07-15 02:30:57 EDT
lightly tested this last iteration, and so far so good.  no (visible) crashes, and 
$ ck-list-sessions
Session6:
        uid = '500'
        realname = 'Rex Dieter'
        seat = 'Seat1'
        session-type = ''
        active = TRUE
        x11-display = ':0'
        x11-display-device = '/dev/tty7'
        display-device = ''
        remote-host-name = ''
        is-local = TRUE
        on-since = '2008-07-15T06:21:46Z'
Session2:
        uid = '0'
        realname = 'root'
        seat = 'Seat1'
        session-type = ''
        active = FALSE
        x11-display = ''
        x11-display-device = ''
        display-device = '/dev/tty1'
        remote-host-name = ''
        is-local = TRUE
        on-since = '2008-07-14T16:00:23Z'
        idle-since-hint = '2008-07-14T16:00:58Z'
Comment 16 Kevin Kofler 2008-07-16 10:29:46 EDT
FYI (in case you want to reuse this in XDM or somewhere), I fixed 2 more bugs:
* if you take the address of a char[20], you get a char(*)[20] which is 
represented like a char *, not a char **, 
ck_connector_open_session_with_parameters expects the latter,
* the correct parameter to pass is x11-display-device, not display-device.
Fixed patch here:
http://cvs.fedoraproject.org/viewcvs/rpms/kdebase-workspace/devel/kdebase-workspace-4.0.98-consolekit-kdm.patch?rev=1.5&view=markup

Note You need to log in before you can comment on or make changes to this bug.