(Garr. Meant gdm instead - moving.)

This is now done in the wip/gdbus-port branch, and it's ready for review.

Great!  Can you post them here with git-bz ?

Actually, upon testing I already found a few bugs, so it may be better to wait until I get it to work before reviewing...

(Btw, I used a branch because git-bz doesn't work well with many patches in sequence)

Created attachment 214320 [details] [review]
Add DBus utilities to create private servers

Consolidates code that creates a private DBus server, including
generating the address and setting up the required message bus
interfaces.

Created attachment 214321 [details] [review]
Port GdmGreeterServer to GDBus

GdmGreeterServer will soon see changes to allow multiple interactions
at the same time, and porting to GDBus is a prerequisite to make
the code more manageable.

Created attachment 214322 [details] [review]
Use GDBus for GdmSettings

Second patch in the GDBus port, this is primarily to ensure that
everything the slave expose is in what GIO thinks is the system bus.

Created attachment 214324 [details] [review]
Port GdmFactorySlave to GDBus

Last commit moved global bus initialization to load GDBus instead
of dbus-glib, so this commit ensures that all traffic on the system
bus is through GIO.

Created attachment 214325 [details] [review]
Continue porting GdmFactorySlave / GdmProductSlave to GDBus

This commit tackles the GdmSession infrastructure, trying to
consolidate GdmSessionRelay and GdmSessionDirect into a single
generated class. For now, only GdmSessionRelay is ported (both
client and server side).

Created attachment 214326 [details] [review]
Refactor GdmSessionDirect to use GDBus and generated bindings

Since GdmSessionRelay went away in the last commit, GdmSessionDirect
was the only class implementing GdmSession, so drop the latter
and use GdmSessionDirect in slaves. Also, refactor it to be an
abstraction over multiple GdmDBusSession instead of talking DBus
directly.

Created attachment 214327 [details] [review]
Port CkConnector to GDBus

Port the CkConnector glue code to GLib and GDBus.

Created attachment 214328 [details] [review]
Port GdmSessionWorker to GDBus

This finishes the GdmSession saga, bringing the last piece inline
with the generated code.

Created attachment 214329 [details] [review]
Port GdmDisplay, GdmXdmcpSlave and friends to GDBus

This is one big commit because it uses generated code both
in the daemon and in the slaves, so we need to port both at the
same time.

Created attachment 214330 [details] [review]
Port the gdm-binary daemon to GDBus

Use GDBusObjectManager for enumerating displays, instead of a
hand rolled interface. Also, fix a bunch of present but not
really working interfaces (slave bus name watching, display-added/removed
signals).

Created attachment 214331 [details] [review]
Finish porting the daemon to GDBus

The daemon, all the slaves, and the session worker now all use
GDBus exclusively. Dependency on dbus-glib has been removed (but
it still remains for the GUI parts)

Created attachment 214332 [details] [review]
Port gdmflexiserver to GDBus

One less dependency on dbus-glib, yay!

Created attachment 214333 [details] [review]
Fix distcheck

Add missing xml files to EXTRA_DIST and generated glue code to
BUILT_SOURCES.
Update .gitignore

Review of attachment 214321 [details] [review]:

::: daemon/Makefile.am
@@ +279,2 @@
 	$(NULL)
 

You need to add gdm-greeter-server.xml to EXTRA_DIST

Review of attachment 214322 [details] [review]:

::: common/Makefile.am
@@ +101,3 @@
 libgdmcommon_la_CPPFLAGS = 		\
 	$(AM_CPPFLAGS)			\
 	$(DISABLE_DEPRECATED_CFLAGS)	\

You need to add gdm-settings.xml to EXTRA_DIST

Review of attachment 214325 [details] [review]:

::: daemon/Makefile.am
@@ +267,1 @@
 gdm_product_slave_LDFLAGS =		\

You need to add gdm-session.xml to EXTRA_DIST

Review of attachment 214321 [details] [review]:

.

Review of attachment 214322 [details] [review]:

.

Review of attachment 214322 [details] [review]:

.

Review of attachment 214327 [details] [review]:

::: daemon/ck-connector.h
@@ +58,2 @@
+const char   *ck_connector_get_cookie                (CkConnector  *ckc);
+gboolean   ck_connector_close_session                (CkConnector  *ckc,

Lots of whitespace movement here, it seems ?

Review of attachment 214333 [details] [review]:

Ah, this should probably be folded into the earlier patches, where these files are introduced

Review of attachment 214333 [details] [review]:

Ah, this should probably be folded into the earlier patches, where these files are introduced

(In reply to comment #19)
> Review of attachment 214322 [details] [review]:
> 
> ::: common/Makefile.am
> @@ +101,3 @@
>  libgdmcommon_la_CPPFLAGS =         \
>      $(AM_CPPFLAGS)            \
>      $(DISABLE_DEPRECATED_CFLAGS)    \
> 
> You need to add gdm-settings.xml to EXTRA_DIST

It's already there (it was used by dbus-glib)

Ok, rebased and killed "fix distcheck"
Updated branch at https://github.com/gcampax/gdm/tree/gdbus-port
(and https://github.com/gcampax/gdm/tree/slave-connection for the related bug/patch)

Hey, so as you know, I've been churning through your patches and reorganzing them a bit on the wip/slave-connection branch.

I'd like to see this land for the release that was supposed to happen today, so I postponed the release and got Florian to postpone the gnome-shell release.

I'd like to do a more thorough write up fo your patchset, but I probably won't have time tonight.  In general, let me just say, these patches are great! GDM can be a daunting codebase to get a handle over, but your patches show an indepth understanding of the code base.

I was also amazed at how fast you got the patches put together after starting.  The main change I made on my branch was to change how clients get the address of their peer-to-peer communication channels. I changed things so clients have to go through the main GDM daemon, and it decides what slave the client should get in touch with.  Also, I took your rewrite as an opportunity to drop GdmGreeterServer, since it doesn't really have much of a purpose.

I'll post my reorganization/assorted fixes here and then push them and do the GDM release.

The following commits have been pushed:
45594c4 chooser: port to libgdm
1ec0665 greeter: port to libgdm
c11bbed libgdmgreeter: rename to libgdm
6ce1c7d libgdmgreeter: generate implementation
982e196 configure: Drop dbus-glib from daemon code
00c37b0 daemon: add greeter test program
2853ce5 worker: add reauthentication support
4f86de3 worker: propagate 'is local' to session worker
e0a3c56 daemon: Add an interface for communicating with GDM via D-Bus
5d31961 worker: Port to GDBus
53e24b4 daemon: Port GdmSession to use GDBus
3eeec6e daemon: Port GdmChooserServer to GDBus
15f3f5d daemon: Finish display if its slave dies
3658827 daemon: Port GdmManager to GDBus
133e18e daemon: Remove transient displays from display store when finished
4e392e2 daemon: Fix added/remove signal emission in display code
dddeab1 daemon: Port display handling to GDBus
0478d42 daemon: NUL terminate X11 cookie
c417ebc daemon: Port GdmGreeterServer to GDBus
3f6eecf daemon: Add utilities to create private DBus servers
82ea382 configure: drop dbus-glib from common libs
f1ddf82 gdmflexiserver: Port to GDBus
20cf9b2 common: Use GDBus for GdmSettings
560e530 common: plug small memory leak
2b55fed simple-greeter: only hide Other item if we can skip it

Created attachment 218971 [details] [review]
chooser: port to libgdm

The chooser is the UI showns to XDMCP clients connecting
via indirect queries.  It shows a list of login screens
on the network and lets the user pick that login screen
to jump to.

Right now it uses its own hardcoded D-Bus calls
to interact with the GDM daemon.

The D-Bus APIs have changed, though, and so now it
no longer works.

This commit changes simple-chooser to use libgdm
instead.

Created attachment 218972 [details] [review]
greeter: port to libgdm

The simple-greeter is the fallback greeter shown if
gnome-shell is unavailable.

Right now it uses its own hardcoded D-Bus calls
to interact with the GDM daemon.

The D-Bus APIs have changed, though, and so now it
no longer works.

This commit changes simple-greeter to use libgdm
instead.

Created attachment 218973 [details] [review]
libgdmgreeter: rename to libgdm

libgdmgreeter is useful for clients that aren't greeters.
This will be even more true in the future as it gets updated
to be useful for screensavers.

Given it offers APIs that apply to non-greeter applications,
it's silly to have greeter in the library name.

This commit renames libgdmgreeter to libgdm.

https://bugzilla.gnome.org/show_bug.cgi?id=676381

Created attachment 218974 [details] [review]
libgdmgreeter: generate implementation

GdmGreeterClient is the interface greeters use to communicate
with their respective slaves. It will eventually also be
useful as an interface for screensavers to do authentication.

The actual GdmGreeterClient code is a just a thin wrapper around
some libdbus calls. Something very similar can be automatically
generated using gdbus-codegen.

This commit:

    - updates the library to use the most up to date dbus
      interfaces provided by the daemon
    - replaces the hand rolled dbus code with generated code,
      but leaving the client interface to get at the generated
      objects.

Based on work by Giovanni Campagna <gcampagna@src.gnome.org>

https://bugzilla.gnome.org/show_bug.cgi?id=676381

Created attachment 218975 [details] [review]
configure: Drop dbus-glib from daemon code

Now that the GDM daemon code is using GDBus,
we don't need to pull in dbus-glib anymore.

This commit removes the pkgconfig check for dbus-glib.

Note, the fallback greeter and chooser still use dbus-glib.

Created attachment 218976 [details] [review]
daemon: add greeter test program

This commit adds a small test program that excerises
the new interface for connecting to GDM.

Created attachment 218977 [details] [review]
worker: add reauthentication support

This commit adds reauthentication support for
screensavers and user switching to use.

1) It adds a "verification mode" argument to the GdmSession constructor
that tweaks the behavior of how the session worker acts to fit login or
unlock scenarios better.

2) It adds a way for programs to open a communication channel for user
verification to already runnings sessions (so reauthentication happens
in the context of the session).

Created attachment 218978 [details] [review]
worker: propagate 'is local' to session worker

It's needed by ConsoleKit and it will be needed for
starting reauthentication sessions.

Created attachment 218979 [details] [review]
daemon: Add an interface for communicating with GDM via D-Bus

One goal for GNOME 3.6, is to replace the screen locking functionality
provided by gnome-screensaver with redesigned functionality provided
by gnome-shell.

At the same time, it makes sense to consolidate the yucky PAM
authentication code to one place (GDM).

Right now only greeters can talk to GDM.  At the time the greeter is
started, the slave sets up a private communication channel which the
greeter then connects to for initiating communication.

This commit adds a new method to the org.gnome.DisplayManager.Manager
interface that allows opening a private connection to the slave that
is associated with the currently running session.  That slave exports
the session object over the bus that greeters can interact with the
session as appropriate.  This interface replaces the
GDM_GREETER_DBUS_ADDRESS environment variable that used to to be used
for connecting the greeter to the slave.

This commit also drops gdm-greeter-server and gdm-chooser-server which
don't fit the new model, and are really just thin middle men that don't do
anything important.

Furthermore, this commit splits GdmSession interfaces 3 orthogonal parts
up into 3 separate interfaces on the session object.

A future commit will make this interface work for screensavers/reauthentication.

Based on work by Giovanni Campagna <gcampagna@src.gnome.org>

https://bugzilla.gnome.org/show_bug.cgi?id=676381

Created attachment 218980 [details] [review]
worker: Port to GDBus

The gdm-session-worker is a process used for managing interaction
with PAM. PAM modules can do weird things to the process they run
in, so GDM segregrates all PAM conversations in their own independent
"worker" subprocesses.

This commit moves gdm-session-worker away from using dbus-glib
to using gdbus instead.

Created attachment 218981 [details] [review]
daemon: Port GdmSession to use GDBus

GdmSession is an object in the slave that manages
the various session worker processes.  Each session
worker process talks to PAM to perform authentication
for the user. For instance, if the user has a fingerprint
reader, then there will normally be two worker processes,
one for handling fingerprint auth, and one for handling
password auth.  GdmSession is the interface layer in the
slave to talking to those running worker processes.

This commit ports GdmSession over to GDBus from dbus-glib.

Created attachment 218982 [details] [review]
daemon: Port GdmChooserServer to GDBus

GdmChooserServer is the slave-side object that handles communication
with choosers. The chooser talks over a private peer-to-peer dbus
connection to its slave via GdmChooserServer.

A chooser is like a greeter, but instead of presenting a login screen
it presents a list of hosts on the network that will offer login screens
if asked.  The user picks one of these hosts and their display is
redirected to it.

This commit makes GdmChooserServer use GDBus.

This gets us one step closer to GDM running without dbus-glib.

https://bugzilla.gnome.org/show_bug.cgi?id=6228

Created attachment 218983 [details] [review]
daemon: Finish display if its slave dies

If a slave goes away for whatever reason, we need to
make sure we clean up the display associated with it.

This commit changes the display code to watch its associated
slave on the bus and automatically finish if the slave goes away.

Created attachment 218984 [details] [review]
daemon: Port GdmManager to GDBus

The GdmManager object controls the GdmLocalDisplayFactory and
GdmXdmcpDisplayFactory singleton objects, which manage displays on local
VTs and displays on remote machines respectively.

Another role of the GdmManager object is to aggregate and export the
displays currently being managed by those display factories over the
system bus.

This commit moves GdmManager over to using GDBus and the
GDBusObjectManager interface for display enumeration.

Created attachment 218985 [details] [review]
daemon: Remove transient displays from display store when finished

When a display finishes (because it failed to start, or the users
session ended) the GDM display handling code needs to remove the
display from its display store.

For static (logind or the first automatic) displays this happens in the

on_static_display_status_changed

function.  For transient (user-switch initiated) displays, though,
it never happens.

This commit reworks on_static_display_status_changed into a more
generally applicable on_display_status_changed function, so the proper
bookkeeping can happen for transient displays, too.

Created attachment 218986 [details] [review]
daemon: Fix added/remove signal emission in display code

The display store is a small container object meant to track
currently known about displays.

It has two signals, "display-added" and "display-removed" that
are supposed to get emitted any time a display gets added or
removed from the store.

Likewise, the GdmManager object has two similar signals that
are supposed to be emitted under similar circumstances.

These signals in GdmDisplayStore and GdmManager were never
actually hooked up to fire at the appropriate times.

This commit changes GdmDisplayStore and GdmManager to properly
fire these signals.

Created attachment 218987 [details] [review]
daemon: Port display handling to GDBus

This is one big commit because it uses generated code both
in the daemon and in the slaves, so we need to port both at the
same time.

Created attachment 218988 [details] [review]
daemon: NUL terminate X11 cookie

The X11 cookie is used as a shared secret between X clients
and the X server.  This cookie is written into an Xauthority
file pointed to by the XAUTHORITY environment variable.  In the
event standard peer-credential based access isn't available, clients
read this file and gain authorization to use the X server by
presenting the cookie to the X server.

This cookie is passed from the gdm daemon to the slave over the bus
as an array of bytes. GDBus expects bytes strings to be NUL terminated.

This commit NUL terminates the cookie, so that it will be compatible
with GDBus's generated code.

Created attachment 218989 [details] [review]
daemon: Port GdmGreeterServer to GDBus

GdmGreeterServer is the slave-side object that handles communication
with greeters. The greeter talks over a private peer-to-peer dbus
connection to its slave via GdmGreeterServer.

This commit makes GdmGreeterServer use GDBus.

This gets us one step closer to GDM running without dbus-glib.

Created attachment 218990 [details] [review]
daemon: Add utilities to create private DBus servers

GDM creates private off-the-bus dbus servers for facilitating
communication between its various processes.

This commit adds a new function:

gdm_dbus_setup_private_server

that performs the work necessary to create a private server,
including code for generating the socket address, and code for
handling new connections.

The ultimate goal is to drop our dependency on dbus-glib, and
instead use GDBus. gdm_dbus_setup_private_server uses GDBus, so
this is one step toward that goal.

Created attachment 218991 [details] [review]
configure: drop dbus-glib from common libs

While the daemon still uses dbus-glib, none of the bits
that rely on the common libs exclusively do anymore.

This commit drops the requirement from configure.

Created attachment 218992 [details] [review]
gdmflexiserver: Port to GDBus

gdmflexiserver is the program that handles initiating
user switch requests.

Ultimately, we don't want any part of GDM using dbus-glib,
including gdmflexiserver.

This commit moves gdmflexiserver over to GDBus.

Created attachment 218993 [details] [review]
common: Use GDBus for GdmSettings

GdmSettings is a system bus service provided by
GDM so that greeters can read custom.conf without
parsing the file themselves.

This commit changes GdmSettings to use gdbus instead
of dbus-glib.

Created attachment 218994 [details] [review]
common: plug small memory leak

In the event GDM fails to be parse its settings
schema file, settings clients will leak the proxy
connection.

This commit fixes that.

Created attachment 218995 [details] [review]
simple-greeter: only hide Other item if we can skip it

Normally if "Other" is the only thing we'll show, we forgo it
and jump straight to the password prompt.

There is a bug, though, if multiple authentication methods are
enabled.  In that case, we hide the user chooser, but don't jump
to the Password prompt.

This commit makes sure we show the user chooser / Other in that case.

Nothing depends on dbus-glib now, so this is fixed, right?

Already found one problem with this patchset:
GdmSession emits ::verification-complete (the GObject signal) before emitting VerificationComplete (the DBus signal). GdmSessionWorker listens to ::verification-complete and resets the session, causing a Reset to be emitted.
Therefore clients would first get Reset, clear the UserVerifier proxy and all associated resources, and never get VerificationComplete.
Inverting lines 403-405 of gdm-session.c fixes it.

Good eye.  I don't think it actually matters for current users, since
the current greeters don't listen for verification-complete, but its
definitely a bug.

I've pushed this fix.

3c0c145 daemon: emit verification-complete over bus before sending to worker

Let's leave the bug open for a bit to catch any other lingering things that pop up.

Created attachment 219040 [details] [review]
daemon: emit verification-complete over bus before sending to worker

When performing reauthentication, the worker listens for the PAM
conversation to finish and then resets the session object and bubbles
it up to its parent as a 'reauthenticated' signal.

This "reset the session" part means that after verification-complete
has been emitted the greeter will get a Reset signal.  This signal
gets sent before VerificationComplete is sent over the bus meaning
the VerificationComplete signal is effectively neutered.

This commit makes VerificationComplete to get sent over the private
bus connection before Reset.

Spotted by Giovanni Campagna <gcampagna@src.gnome.org>

Uhm, is there anything else here?

No, I think this is done -closing it