GNOME Bugzilla – Bug 655380
[PATCH] add automatic multi seat support
Last modified: 2012-10-01 05:21:32 UTC
This patch series adds automatic multi-seat support to gdm. With this in place plugging in hw like the Plugable USB terminal will make gdm immediately show a login prompt on them. This makes sue of the new automatic multi-seat stuff in systemd. Note that this patch series is still pending a few fixes in systemd, hence for now this is only here for review, and should be commited only after the next systemd release out.
Created attachment 192721 [details] [review] 0001-gdm-slave-fix-memory-leak.patch
Created attachment 192722 [details] [review] 0002-gdm-slave-fix-double-dbus_error_init.patch
Created attachment 192723 [details] [review] 0003-build-sys-add-configure-option-with-systemd.patch
Created attachment 192724 [details] [review] 0004-slave-add-native-systemd-implementations-of-locking-.patch
Created attachment 192725 [details] [review] 0005-build-sys-make-CK-support-optional.patch
Created attachment 192726 [details] [review] 0006-slave-pass-seat-id-from-direct-session-to-slaves.patch
Created attachment 192727 [details] [review] 0007-pam-pass-XDG_SEAT-env-var-into-PAM-to-inform-PAM-mod.patch
Created attachment 192728 [details] [review] 0008-local-display-factory-subscribe-to-new-seats-being-c.patch
Created attachment 192729 [details] [review] 0009-server-pass-seat-id-to-server.patch
Created attachment 192730 [details] [review] 0010-gdm-x-hack-introduce-wrapper-for-X-server-to-deal-wi.patch
Review of attachment 192721 [details] [review]: obviously fine
Review of attachment 192722 [details] [review]: looks fine
Review of attachment 192729 [details] [review]: is -seat upstream?
Review of attachment 192730 [details] [review]: oh i see, this patch is out of order with the previous one, no?
(In reply to comment #13) > Review of attachment 192729 [details] [review]: > > is -seat upstream? Yes, and even in f16+rawhide now.
(In reply to comment #14) > Review of attachment 192730 [details] [review]: > > oh i see, this patch is out of order with the previous one, no? No, X understands -seat natively. The gdm-x-seat-hack thing is just there to extend the -seat stuff in X to not only cover input devices but graphics devices, too. X currently does not do any kind of proper enumeration of graphcis hardware (it just bangs PCI registers by means of libpciaccess), so we couldn't teach it seat-awareness for graphics devices. Ajax wants to fix that eventually, and move X to libudev for finding graphics hardware too. When that happens we can remove gdm-x-seat-hack. I kept this wrapper outside of the gdm binaries proper, to make clear that this is only temporary.
Where do we stand on this ?
waiting on me.
Pushing the two a-c-n s
Comment on attachment 192721 [details] [review] 0001-gdm-slave-fix-memory-leak.patch Mark as obsoleted, since it's commited
Comment on attachment 192722 [details] [review] 0002-gdm-slave-fix-double-dbus_error_init.patch Also commited
Created attachment 205384 [details] [review] welcome-session: get rid of register-ck-session property since it is unused daemon/Makefile.am | 6 ------ daemon/gdm-chooser-session.c | 1 - daemon/gdm-greeter-session.c | 1 - daemon/gdm-welcome-session.c | 25 +------------------------ 4 files changed, 1 insertions(+), 32 deletions(-)
Created attachment 205385 [details] [review] build-sys: add configure option --with-systemd
Created attachment 205386 [details] [review] slave: add native systemd implementations of locking/session activation
Created attachment 205387 [details] [review] build-sys: make CK support optional
Created attachment 205388 [details] [review] slave: pass seat id from direct session to slaves
Created attachment 205389 [details] [review] pam: pass XDG_SEAT env var into PAM to inform PAM modules about seat id
Created attachment 205390 [details] [review] local-display-factory: subscribe to new seats being created and removed
Created attachment 205391 [details] [review] server: pass seat id to server
Created attachment 205392 [details] [review] gdm-x-hack: introduce wrapper for X server to deal with USB device enumeration
I have now rebased all outstanding patches to current gdm git. For the rebase I also adde done new patch which gets rid of the last remnants of CK registration code in the welcome session code, since that's already done exclusively with PAM now. This is just a cleanup patch basically, which gets rid of an unused property and all the code that it resulted in. I have also fixed a couple of other things in the patch set (for example, coding style issues which I missed before), and we now handle it properly if we get SeatAdded signals for a seat that already exists from logind (which might happen if we restart logind). I like to believe that all the individual bits are now pretty simple an uncontroversial, and the biggest more controversial issues is already gone since Ray commited the PAM/CK rework.
Review of attachment 205384 [details] [review]: Overall, the clean up makes sense. Two thoughts: 1) You should make sure to point out in the commit message that the property was never used for anything. 2) git-bz makes it a lot easier to attach patches, and makes patches that are more splinter-compatible, so you may want to use it from now on.
Review of attachment 205386 [details] [review]: This seems straightforward enough. Mostly nits below. ::: daemon/gdm-slave.c @@ +548,3 @@ if (XGetWindowProperty (slave->priv->server_display, + DefaultRootWindow (slave->priv->server_display), prop, 0, 1, + False, AnyPropertyType, &actualtype, &actualformat, What did you change here? @@ +1204,3 @@ } +#if WITH_CONSOLE_KIT You use #if a few times when you really need #ifdef I think. I believe config.h leaves these types of variables undefined when not defined to 1. So it's going to cause compiler warnings or so. Actually, the GDM codebase has historically been pretty anti-sprinkled-#ifdefs (see for instance, how we did gdm-session-auditor), but we've been letting it slide somewhat recently, so I wouldn't sweat over it. @@ +1330,3 @@ g_debug ("GdmSlave: getting proxy for seat: %s", slave->priv->display_seat_id); +#ifdef WITH_SYSTEMD I think the guts of this should be in a new function get_primary_session_id_for_user_from_systemd () (and likewise move the consolekit stuff to it's own function). @@ +1331,3 @@ +#ifdef WITH_SYSTEMD + if (sd_booted() > 0) { sd_booted () @@ +1332,3 @@ +#ifdef WITH_SYSTEMD + if (sd_booted() > 0) { + int r; most of the surrounding code uses "res" for temporary result variable. You can't do that here as is, because of the type mismatch, but when this stuff is sitting in its own function then that name will become available again and you can be consistent. @@ +1349,3 @@ + } + + r = sd_seat_get_sessions (slave->priv->display_seat_id, &seats, NULL, NULL); "seats" should be named "sessions" not "seats" @@ +1351,3 @@ + r = sd_seat_get_sessions (slave->priv->display_seat_id, &seats, NULL, NULL); + if (r < 0) { + g_warning ("Failed to get sessions on seat: %s", strerror(-r)); need space between strerror and ( @@ +1355,3 @@ + } + + for (j = seats; *j; j++) { the name "j" should really only be used for integer counter variables (and then only when being nested in a loop such that "i" isn't available). I'd rather see this written: for (i = 0; sessions[i] != NULL; i++) { This way you don't need the ugly looking dereference everytime you want to access the session variable. @@ +1357,3 @@ + for (j = seats; *j; j++) { + + if (!primary_ssid) { primary_ssid == NULL instead of !primary_ssid @@ +1361,3 @@ + + r = sd_session_get_uid (*j, &other); + if (r > 0 && other == uid) need braces around if blocks even when one line in gdm code. @@ +1468,3 @@ dbus_error_init (&local_error); + +#if WITH_SYSTEMD Again, a separate function (say activate_session_id_with_systemd) would be good (and likewise for consolekit) @@ +1469,3 @@ + +#if WITH_SYSTEMD + if (sd_booted() > 0) { sd_booted () @@ +1474,3 @@ + "org.freedesktop.login1.Manager", + "ActivateSession"); + if (message == NULL) braces @@ +1550,3 @@ g_debug ("ConsoleKit: Unlocking session %s", ssid); + +#if WITH_SYSTEMD Same deal, needs to be put in a separate function. @@ +1551,3 @@ + +#if WITH_SYSTEMD + if (sd_booted() > 0) { sd_booted () @@ +1574,3 @@ + dbus_message_unref(message); + if (reply) + dbus_message_unref(reply); braces and spaces, reply != NULL
Review of attachment 205385 [details] [review]: that's fine
Review of attachment 205387 [details] [review]: mostly fine ::: daemon/Makefile.am @@ +363,3 @@ $(XDMCP_LIBS) \ $(LIBWRAP_LIBS) \ + $(SYSTEMD_LIBS) \ This should be in a different commit (maybe the one a couple of patches back?) @@ +368,2 @@ if WITH_CONSOLE_KIT +gdm_binary_SOURCES += \ Why the escaped newline here?
Review of attachment 205388 [details] [review]: This commit seems a little off. First, the commit message doesn't make sense to me and you don't explain any of your motivation for the changes in the commit. Also, this patch seems to pass the seat id from the slave to the worker and from the slave to the session created by the slave, not from the session to the slave like the subject says. But how does the slave have the seat id anyway? Isn't that a property of the display? I think you need a commit first that gets the seat id from the display to the slave started by the display, and after that's taken care of you can follow up with commits that disseminate the seat id to the slave's helpers.
Review of attachment 205389 [details] [review]: This commit does two things 1) conditionalizes consolekit in worker code 2) adds some systemd support. Ideally, those two things would be separate commits, but even if they go in as one commit, the message should reflect what what changes are happening. ::: daemon/gdm-session-worker.c @@ +48,3 @@ #include <X11/Xauth.h> +#if WITH_SYSTEMD need #fidef not #if (and elsewhere) @@ +1154,3 @@ + + if (seat_id != NULL && seat_id[0] != '\0') { + error_code = pam_misc_setenv (worker->priv->pam_handle, "XDG_SEAT", seat_id, 0); Instead of pam_misc_setenv, you can use gdm_session_worker_set_environment_variable and then you shouldn't need pam_misc added to the libs. I don't think in the consolekit case the seat id is XDG_SEAT compatible, right? I mean the seat id format is "seat0" for systemd and "/org/freedesktop/ConsoleKit/Seat1" or something like that for ConsoleKit. I think this needs to be in a systemd specific section, if we don't want "/org/freedesktop/ConsoleKit/Seat1" to end up in people's XDG_SEAT environment.
Review of attachment 205390 [details] [review]: The commit message should define what "subscribe" means specifically. Some comments below; ::: daemon/gdm-local-display-factory.c @@ +343,3 @@ + +static gboolean +display_seat_predicate (const char *id, should call this "lookup_by_seat_id" to match the style of the other gdm_display_store_find() users. @@ +349,3 @@ + const char *looking_for = user_data; + char *current; + gboolean b; should be called "res" to match the surrounding style. @@ +356,3 @@ + current != NULL && + looking_for != NULL && + strcmp (current, looking_for) == 0; this can just be res = g_strcmp0 (current, looking_for) == 0; @@ +373,3 @@ + /* FIXME: don't hardcode seat1? */ + if (seat_id == NULL) + seat_id = CK_SEAT1_PATH; I'd like to see this moved to the caller now, and only used when building with consolekit, unless we can get rid of it entirely. @@ +378,3 @@ + store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY (factory)); + display = gdm_display_store_find (store, display_seat_predicate, (gpointer) seat_id); + if (display != NULL) needs braces. @@ +381,3 @@ + return NULL; + + g_debug ("Adding display on seat %s", seat_id); needs GdmLocalDisplayFactory: prefix @@ +418,3 @@ + GdmDisplayStore *store; + + g_debug ("Removing displays on seat %s", seat_id); GdmLocalDisplayFactory: prefix @@ +421,3 @@ + + store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY (factory)); + gdm_display_store_foreach_remove (store, display_seat_predicate, (gpointer) seat_id); So we're only ever going to get this when the seat is gone, never when we need to kill the seat ourselves? Are there any races where the GDM machinery would detect the seat going away from some other notification? A possible example: say a seat gets forcefully removed by the adminstrator and systemd kills the user's session. GDM notices the session going away and the display dying and begins to process it. At the same time, this delete_display function ges called from SeatRemoved signal sent by systemd. Is that example where two parts of GDM would be racing with each other? If so, is the race harmless? Just trying to avoid bugs down the road that are hard to reproduce. @@ +430,3 @@ + DBusMessageIter iter, sub, sub2; + + dbus_error_init(&error); need space @@ +437,3 @@ + "org.freedesktop.login1.Manager", + "ListSeats"); + if (!message) { == NULL @@ +445,3 @@ + dbus_message_unref (message); + + if (!reply) { == NULL @@ +489,3 @@ + +static DBusHandlerResult +on_seat_signal (DBusConnection *connection, DBusMessage *message, void *userdata) parameters should be on their own lines. userdata should have an underscore @@ +510,3 @@ + } else { + + if (strcmp (dbus_message_get_member (message), "SeatNew") == 0) braces. It also might be cleaner to split SeatNew and SeatRemoved up, but i'm pretty indifferent on that. @@ +713,3 @@ +#if WITH_SYSTEMD + gdm_local_display_factory_stop_monitor(factory); space
Review of attachment 205391 [details] [review]: Again, need a better commit message. Say something about how the X server needs to be told which resources belong to it, and so this commit does some of the prerequiste plumbing to get the seat id over to the code that ultimately starts the X server. ::: daemon/gdm-server.c @@ +304,3 @@ + if (server->priv->display_seat_id != NULL) { + argv[len++] = g_strdup ("-seat"); + argv[len++] = g_strdup (server->priv->display_seat_id); Given no X server supports -seat, I don't think we can have that in this commit (unless this commit also checks somehow if -seat is available (via configure or runtime i guess). Probably easier to just roll these 4 lines into the next patch and leave the seat dangling for now.
(In reply to comment #39) > Given no X server supports -seat, I don't think we can have that in this commit > (unless this commit also checks somehow if -seat is available (via configure or > runtime i guess). I stand corrected. Looking at a copy of the X server source code, I see -seat is supported. What happens if an X server that doesn't support it, encounters it? Error? Ignore?
Review of attachment 205392 [details] [review]: Nice commit message. I realize this is a stopgap, but I'm still not very happy with the overall approach. Part of the issue might just be because it's named "hack", I guess and that's tainting things for me. Still, one alternative idea: Have a static file in xorg.conf.d that defines a specific "gdm-udev-framebuffer-layout" server layout that points to a device section that's like what your hack program writes out, but without the fbdev Option. Then when starting these kinds of seats, we could start the X server with -layout "gdm-udev-framebuffer-layout" and set the FRAMEBUFFER= environment variable to the right framebuffer device. Thoughts?
(In reply to comment #32) > Review of attachment 205384 [details] [review]: > 2) git-bz makes it a lot easier to attach patches, and makes patches that are > more splinter-compatible, so you may want to use it from now on. Well, great idea. I actually uplaoded this first one of the patches with git-send-bugzilla, but that crashed due to some stupid perl failure for the second and subsequent one. And my interest in dealing with Perl is rather low, so I just uploaded them by hand...
(In reply to comment #41) > Review of attachment 205392 [details] [review]: > > Nice commit message. I realize this is a stopgap, but I'm still not very happy > with the overall approach. Part of the issue might just be because it's named > "hack", I guess and that's tainting things for me. Still, one alternative idea: > > Have a static file in xorg.conf.d that defines a specific > "gdm-udev-framebuffer-layout" server layout that points to a device section > that's like what your hack program writes out, but without the fbdev Option. > Then when starting these kinds of seats, we could start the X server with > -layout "gdm-udev-framebuffer-layout" and set the FRAMEBUFFER= environment > variable to the right framebuffer device. Thoughts? Hmm, you are saying X understands env var replacements? If not, I don't see much point in patching X11 for this. I mean, I think it's a good idea not to add temporary cruft to either gdm, nor X11, if it's clear that in the long run we'll need completely different code for this anyway, which is going to be much more complex though (i.e. the full udev enumeration logic in X11). I think what is nice in the solution I put together is that all the glue stuff happens out-of-process in an isolated tool of its own, and doesn't pollute gdm's own source code. Also, during runtime you'll never notice that we used the hack thingy, since it will exec() the real X server, thus replacing its own process.
(In reply to comment #40) > (In reply to comment #39) > > Given no X server supports -seat, I don't think we can have that in this commit > > (unless this commit also checks somehow if -seat is available (via configure or > > runtime i guess). > I stand corrected. Looking at a copy of the X server source code, I see -seat > is supported. Yupp, since 6 month or so, even. The patches for that are in stable F16 and everywhere. > What happens if an X server that doesn't support it, encounters it? Error? > Ignore? Well, I think it should be OK to just rely on a recent X version (i.e. express that in gdm's RPM deps). After all X servers understanding -seat are already out in the wild since many month.
(In reply to comment #43) > (In reply to comment #41) > > Review of attachment 205392 [details] [review] [details]: > > > > Nice commit message. I realize this is a stopgap, but I'm still not very happy > > with the overall approach. Part of the issue might just be because it's named > > "hack", I guess and that's tainting things for me. Still, one alternative idea: > > > > Have a static file in xorg.conf.d that defines a specific > > "gdm-udev-framebuffer-layout" server layout that points to a device section > > that's like what your hack program writes out, but without the fbdev Option. > > Then when starting these kinds of seats, we could start the X server with > > -layout "gdm-udev-framebuffer-layout" and set the FRAMEBUFFER= environment > > variable to the right framebuffer device. Thoughts? > > Hmm, you are saying X understands env var replacements? Yes, I believe so. FRAMEBUFFER is pretty standard. I haven't tried it mind you... But I think you'll be able to get away with a small static text file, since I think it probably already supports the environment variable. > If not, I don't see much point in patching X11 for this. I mean, I think it's a > good idea not to add temporary cruft to either gdm, nor X11, if it's clear that > in the long run we'll need completely different code for this anyway, which is > going to be much more complex though (i.e. the full udev enumeration logic in > X11). I think what is nice in the solution I put together is that all the glue > stuff happens out-of-process in an isolated tool of its own, and doesn't > pollute gdm's own source code. I know it's temporary and you're trying to segregrate the ugly, but I'd rather mix the ugly in so it doesn't stand out so much. It would be one thing if this were a Xfbdev script shipped in the Xorg tree, but if it's shipped in the GDM tree, it should try to integrate with GDM i think. We've had device enumeration code in GDM before...See http://git.gnome.org/browse/gdm/commit/?id=bbad06 for when it was removed. Of course, when X "does the right thing" on its own we can just drop the enumeration, drop the static layout, drop the -layout and the env var. > Also, during runtime you'll never notice that we > used the hack thingy, since it will exec() the real X server, thus replacing > its own process. Yea, I see your angle, I think. You're saying "X doesn't support probing USB graphics devices, but it does support knowing about seats, so bridge the gap for now by wedging between gdm and X, scanning for usb graphics devices on the specified seat and hard code the configuration in a file that X will pick up then transparently let x procede". I'm not 100% saying no, but it definitely turns me off. Out of curiosity, what's the big complication with changing the fbdev driver's probe logic to look at the seat id and pull from udev if applicable? It can't be much more code than your wrapper program can it?
(In reply to comment #44) > Well, I think it should be OK to just rely on a recent X version (i.e. express > that in gdm's RPM deps). After all X servers understanding -seat are already > out in the wild since many month. Sure. Though again, the consolekit idea of a seat id is incompatible with the systemd/udev/Xorg idea of a seat id, so it should only pass it in the systemd case, right?
Looks like here is code for where FRAMEBUFFER is checked: http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/fbdevhw/fbdevhw.c#n304
(In reply to comment #32) > Review of attachment 205384 [details] [review]: > > Overall, the clean up makes sense. Two thoughts: > > 1) You should make sure to point out in the commit message that the property > was never used for anything. Actually the commit message says this: "welcome-session: get rid of register-ck-session property since it is unused" I will now add this to the body of the comment as well.
(In reply to comment #33) > Review of attachment 205386 [details] [review]: > > This seems straightforward enough. Mostly nits below. > > ::: daemon/gdm-slave.c > @@ +548,3 @@ > if (XGetWindowProperty (slave->priv->server_display, > + DefaultRootWindow (slave->priv->server_display), prop, 0, 1, > + False, AnyPropertyType, &actualtype, &actualformat, > > What did you change here? Nothing actually. It's just that this code had trailing whitespace and my editor cleaned that up. Will drop it. > > @@ +1204,3 @@ > } > > +#if WITH_CONSOLE_KIT > > You use #if a few times when you really need #ifdef I think. I believe config.h > leaves these types of variables undefined when not defined to 1. So it's going > to cause compiler warnings or so. Actually, the GDM codebase has historically > been pretty anti-sprinkled-#ifdefs (see for instance, how we did > gdm-session-auditor), but we've been letting it slide somewhat recently, so I > wouldn't sweat over it. Fixed. > > @@ +1330,3 @@ > g_debug ("GdmSlave: getting proxy for seat: %s", > slave->priv->display_seat_id); > > +#ifdef WITH_SYSTEMD > > I think the guts of this should be in a new function > get_primary_session_id_for_user_from_systemd () (and likewise move the > consolekit stuff to it's own function). Done. > > @@ +1331,3 @@ > > +#ifdef WITH_SYSTEMD > + if (sd_booted() > 0) { > > sd_booted () Done. > > @@ +1332,3 @@ > +#ifdef WITH_SYSTEMD > + if (sd_booted() > 0) { > + int r; > > most of the surrounding code uses "res" for temporary result variable. You > can't do that here as is, because of the type mismatch, but when this stuff is > sitting in its own function then that name will become available again and you > can be consistent. Done. > @@ +1349,3 @@ > + } > + > + r = sd_seat_get_sessions (slave->priv->display_seat_id, > &seats, NULL, NULL); > > "seats" should be named "sessions" not "seats" Indeed. Fixed. > @@ +1351,3 @@ > + r = sd_seat_get_sessions (slave->priv->display_seat_id, > &seats, NULL, NULL); > + if (r < 0) { > + g_warning ("Failed to get sessions on seat: %s", > strerror(-r)); > > need space between strerror and ( Done. > > @@ +1355,3 @@ > + } > + > + for (j = seats; *j; j++) { > > the name "j" should really only be used for integer counter variables (and then > only when being nested in a loop such that "i" isn't available). Humpf, i is short for iterator, not integer... > I'd rather see this written: > > for (i = 0; sessions[i] != NULL; i++) { > > This way you don't need the ugly looking dereference everytime you want to > access the session variable. Yeah, but if you do it like this you always end up doing n*2 additions instead of n additions. Anyway, you are the boss, let's hope the compiler is smart enough. Fixed. > @@ +1357,3 @@ > + for (j = seats; *j; j++) { > + > + if (!primary_ssid) { > > primary_ssid == NULL instead of !primary_ssid Fixed. > @@ +1361,3 @@ > + > + r = sd_session_get_uid (*j, &other); > + if (r > 0 && other == uid) > > need braces around if blocks even when one line in gdm code. Urks, how ugly. Man, I love kernel coding style. Anyway, fixed. > @@ +1468,3 @@ > dbus_error_init (&local_error); > + > +#if WITH_SYSTEMD > > Again, a separate function (say activate_session_id_with_systemd) would be good > (and likewise for consolekit) Done. > > @@ +1469,3 @@ > + > +#if WITH_SYSTEMD > + if (sd_booted() > 0) { > > sd_booted () Done. > > @@ +1474,3 @@ > + > "org.freedesktop.login1.Manager", > + "ActivateSession"); > + if (message == NULL) > > braces Done. > @@ +1550,3 @@ > g_debug ("ConsoleKit: Unlocking session %s", ssid); > + > +#if WITH_SYSTEMD > > Same deal, needs to be put in a separate function. Done. > > @@ +1551,3 @@ > + > +#if WITH_SYSTEMD > + if (sd_booted() > 0) { > > sd_booted () Done. > > @@ +1574,3 @@ > + dbus_message_unref(message); > + if (reply) > + dbus_message_unref(reply); > > braces and spaces, reply != NULL Done.
(In reply to comment #35) > Review of attachment 205387 [details] [review]: > > mostly fine > > ::: daemon/Makefile.am > @@ +363,3 @@ > $(XDMCP_LIBS) \ > $(LIBWRAP_LIBS) \ > + $(SYSTEMD_LIBS) \ > > This should be in a different commit (maybe the one a couple of patches back?) Fixed. > > @@ +368,2 @@ > if WITH_CONSOLE_KIT > +gdm_binary_SOURCES += \ > > Why the escaped newline here? Oh, hmm, well, it's the online assignment that isn't newlined. Anyway, dropped it.
(In reply to comment #36) > Review of attachment 205388 [details] [review]: > > This commit seems a little off. First, the commit message doesn't make sense to > me and you don't explain any of your motivation for the changes in the commit. > Also, this patch seems to pass the seat id from the slave to the worker and > from the slave to the session created by the slave, not from the session to the > slave like the subject says. So, what we want to do is passing the seat id from the session to the worker for usage in the PAM chat. We have to go through the slave for that, since the slave is more or less only there to pass information to the worker after all... I didn't split this up give that the slave here is more or less just a proxy for the worker. > But how does the slave have the seat id anyway? > Isn't that a property of the display? Yes, it's a property of the display. And the slave gets the data along with all the other display meta data from the display in gdm_slave_real_start(). > I think you need a commit first that gets > the seat id from the display to the slave started by the display That already exists, simply because Jon's original code already did this for us. > , and after > that's taken care of you can follow up with commits that disseminate the seat > id to the slave's helpers. I have extended the commit message a bit now, explaining all this.
(In reply to comment #37) > Review of attachment 205389 [details] [review]: > > This commit does two things 1) conditionalizes consolekit in worker code 2) > adds some systemd support. Ideally, those two things would be separate > commits, but even if they go in as one commit, the message should reflect what > what changes are happening. Fixed. > ::: daemon/gdm-session-worker.c > @@ +48,3 @@ > #include <X11/Xauth.h> > > +#if WITH_SYSTEMD > > need #fidef not #if (and elsewhere) Fixed. > @@ +1154,3 @@ > + > + if (seat_id != NULL && seat_id[0] != '\0') { > + error_code = pam_misc_setenv (worker->priv->pam_handle, > "XDG_SEAT", seat_id, 0); > > Instead of pam_misc_setenv, you can use > gdm_session_worker_set_environment_variable and then you shouldn't need > pam_misc added to the libs. Fixed. > I don't think in the consolekit case the seat id is XDG_SEAT compatible, right? > I mean the seat id format is "seat0" for systemd and > "/org/freedesktop/ConsoleKit/Seat1" or something like that for ConsoleKit. I > think this needs to be in a systemd specific section, if we don't want > "/org/freedesktop/ConsoleKit/Seat1" to end up in people's XDG_SEAT environment. Fixed.
(In reply to comment #38) > Review of attachment 205390 [details] [review]: > > The commit message should define what "subscribe" means specifically. Some > comments below; Fixed. > ::: daemon/gdm-local-display-factory.c > @@ +343,3 @@ > + > +static gboolean > +display_seat_predicate (const char *id, > > should call this "lookup_by_seat_id" to match the style of the other > gdm_display_store_find() users. Fixed. > > @@ +349,3 @@ > + const char *looking_for = user_data; > + char *current; > + gboolean b; > > should be called "res" to match the surrounding style. Fixed. > @@ +356,3 @@ > + current != NULL && > + looking_for != NULL && > + strcmp (current, looking_for) == 0; > > this can just be res = g_strcmp0 (current, looking_for) == 0; Ah, good to know. Fixed. > > @@ +373,3 @@ > + /* FIXME: don't hardcode seat1? */ > + if (seat_id == NULL) > + seat_id = CK_SEAT1_PATH; > > I'd like to see this moved to the caller now, and only used when building with > consolekit, unless we can get rid of it entirely. OK, moved this down, but I figure we still need it for CK. > > @@ +378,3 @@ > + store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY > (factory)); > + display = gdm_display_store_find (store, display_seat_predicate, > (gpointer) seat_id); > + if (display != NULL) > > needs braces. Fixed. > @@ +381,3 @@ > + return NULL; > + > + g_debug ("Adding display on seat %s", seat_id); > > needs GdmLocalDisplayFactory: prefix > Fixed. > @@ +418,3 @@ > + GdmDisplayStore *store; > + > + g_debug ("Removing displays on seat %s", seat_id); > > GdmLocalDisplayFactory: prefix Fixed. > > @@ +421,3 @@ > + > + store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY > (factory)); > + gdm_display_store_foreach_remove (store, display_seat_predicate, > (gpointer) seat_id); > > So we're only ever going to get this when the seat is gone, never when we need > to kill the seat ourselves? There are two ways a display gets removed: when the display fails on its own. This is handled in on_static_display_status_changed() (which is a signal callback for GdmDisplay's status change signal). This function just calls gdm_display_store_remove() which is simply a NOP if the display is already removed. The other way is when the seat goes away and we informed about this by logind, which is handled in delete_display(). That call uses gdm_display_store_foreach_remove() (since it needs to match by seat id, instead of by display object pointer), and is also idempotent. Hence we should be safe here: if for some reason we get both events we'll both times go through code paths that are safe. > Are there any races where the GDM machinery would > detect the seat going away from some other notification? A possible example: > say a seat gets forcefully removed by the adminstrator and systemd kills the > user's session. GDM notices the session going away and the display dying and > begins to process it. At the same time, this delete_display function ges > called from SeatRemoved signal sent by systemd. Is that example where two parts > of GDM would be racing with each other? If so, is the race harmless? Just > trying to avoid bugs down the road that are hard to reproduce. Yes, they would race, but as mentioned above the two ways to remove a display are idempotent, so it doesn't matter if we remove the displays twice nor in which order. > > @@ +430,3 @@ > + DBusMessageIter iter, sub, sub2; > + > + dbus_error_init(&error); > > need space Fixed. > > @@ +437,3 @@ > + "org.freedesktop.login1.Manager", > + "ListSeats"); > + if (!message) { > > == NULL Fixed. > > @@ +445,3 @@ > + dbus_message_unref (message); > + > + if (!reply) { > > == NULL Fixed. > > @@ +489,3 @@ > + > +static DBusHandlerResult > +on_seat_signal (DBusConnection *connection, DBusMessage *message, void > *userdata) > > parameters should be on their own lines. userdata should have an underscore Fxied. and Fixed. > > @@ +510,3 @@ > + } else { > + > + if (strcmp (dbus_message_get_member (message), > "SeatNew") == 0) > > braces. It also might be cleaner to split SeatNew and SeatRemoved up, but i'm > pretty indifferent on that. I like short code ;-). So if you don't mind, I'll leave it like this... > @@ +713,3 @@ > > +#if WITH_SYSTEMD > + gdm_local_display_factory_stop_monitor(factory); > > space Fixed.
(In reply to comment #39) > Review of attachment 205391 [details] [review]: > > Again, need a better commit message. Say something about how the X server > needs to be told which resources belong to it, and so this commit does some of > the prerequiste plumbing to get the seat id over to the code that ultimately > starts the X server. Fixed.
(In reply to comment #41) > Review of attachment 205392 [details] [review]: > > Nice commit message. I realize this is a stopgap, but I'm still not very happy > with the overall approach. Part of the issue might just be because it's named > "hack", I guess and that's tainting things for me. Still, one alternative idea: > > Have a static file in xorg.conf.d that defines a specific > "gdm-udev-framebuffer-layout" server layout that points to a device section > that's like what your hack program writes out, but without the fbdev Option. > Then when starting these kinds of seats, we could start the X server with > -layout "gdm-udev-framebuffer-layout" and set the FRAMEBUFFER= environment > variable to the right framebuffer device. Thoughts? As discussed on IRC I have now moved this part into systemd, and we just invoke the systemd-supplied wrapper here instead of the real X if it exists and makes sense to. This also has the benefit that other DM implementations can just make use of this wrapper if they want to, instead having to recode that bit.
Created attachment 205941 [details] [review] slave: whitespace cleanup
Created attachment 205942 [details] [review] welcome-session: get rid of register-ck-session property since it is unused
Created attachment 205943 [details] [review] build-sys: add configure option --with-systemd
Created attachment 205944 [details] [review] slave: add native systemd implementations of locking/session activation
Created attachment 205945 [details] [review] build-sys: make CK support optional
Created attachment 205946 [details] [review] slave: pass seat id from product/simple slave to worker, via the direct session
Created attachment 205947 [details] [review] pam: pass XDG_SEAT env var into PAM to inform PAM modules about seat id
Created attachment 205948 [details] [review] local-display-factory: subscribe to new seats being created and removed
Created attachment 205949 [details] [review] server: pass seat id to server
Created attachment 205950 [details] [review] server: invoke X with the systemd multi seat X wrapper if necessary
OK, all patches are up to date now, hope I didn't forget anything you pointed out. Expanded many of the commit messages. And changed the last patch to invoke systemd's wrapper instead of doing its own wrapping.
Review of attachment 205941 [details] [review]: fine
Review of attachment 205942 [details] [review]: i still think it might make sense to point out the property was never used for anything from its inception, but not a big deal.
Review of attachment 205943 [details] [review]: fine
Review of attachment 205944 [details] [review]: optional suggestions below. ::: daemon/gdm-slave.c @@ +1467,3 @@ +#ifdef WITH_SYSTEMD + if (sd_booted () > 0) { + return gdm_slave_get_primary_session_id_for_user_systemd (slave, username); for_user_from_systemd or get_systemd_primary_session_id @@ +1472,3 @@ + +#ifdef WITH_CONSOLE_KIT + return gdm_slave_get_primary_session_id_for_user_ckit (slave, username); for_user_from_ck or get_ck_primary_session_id (ConsoleKit is either spelled out console_kit or abbreviated ck elsewhere in the code, so better to be consistent) @@ +1481,2 @@ static gboolean +activate_session_id_systemd (GdmSlave *slave, activate_systemd_session_id or activate_session_from_systemd @@ +1536,3 @@ +activate_session_id_ckit (GdmSlave *slave, + const char *seat_id, + const char *session_id) same deal @@ +1608,2 @@ static gboolean +session_unlock_systemd (GdmSlave *slave, unlock_systemd_session or session_unlock_with_systemd (same below)
Review of attachment 205945 [details] [review]: fine
Review of attachment 205946 [details] [review]: perfect, thanks.
Review of attachment 205947 [details] [review]: needs one style fix but then you can commit. ::: daemon/gdm-session-worker.c @@ +1158,3 @@ + /* set seat ID */ + if (seat_id != NULL && seat_id[0] != '\0' && sd_booted() > 0) { + gdm_session_worker_set_environment_variable (worker, "XDG_SEAT", seat_id); out of curiosity, have you thought about getting this added to PAM in the same way PAM_XDISPLAY and PAM_XAUTH was recently added? @@ +1597,3 @@ +#ifdef WITH_SYSTEMD + if (sd_booted() > 0) + return; need braces { }
Review of attachment 205948 [details] [review]: looks okay modulo minor style nit below. ::: daemon/gdm-local-display-factory.c @@ +432,3 @@ + "ListSeats"); + if (message == NULL) { + g_warning ("Failed to allocate message"); needs GdmLocalDisplayFactory: prefix (same with all the other g_warnings) @@ +498,3 @@ + dbus_message_get_args (message, + &error, + DBUS_TYPE_STRING, &seat, i've seen some some versions of GCC screw up if you pass a const char * instead of a char * to dbus_message_get_args. A bunch of code does it though, and I haven't seen it cause problems lately, so probably was a gcc bug that's since been fixed.
Review of attachment 205949 [details] [review]: You can commit after the fix up below. ::: daemon/gdm-factory-slave.c @@ +691,3 @@ gboolean res; + slave->priv->server = gdm_server_new (display_name, seat_id, auth_file); You need to pass NULL here if seat_id is from consolekit i think. The X server can't do anything with a consolekit seat right? (Likewise for the other two slaves)
Review of attachment 205950 [details] [review]: you need to change configure.ac to check for systemd >= 39 (or do that in the initial patch that introduces the systemd optional dependency) ::: daemon/Makefile.am @@ +20,3 @@ -DGDM_CACHE_DIR=\""$(localstatedir)/cache/gdm"\" \ -DGDM_SESSION_DEFAULT_PATH=\"$(GDM_SESSION_DEFAULT_PATH)\" \ + -DSYSTEMD_X_SERVER=\"/lib/systemd/systemd-multi-seat-x\" \ can you put this in configure.ac ? We do that for X_SERVER and other systemd stuff, so make sense to put this there too. ::: daemon/gdm-server.c @@ +267,3 @@ + if (server->priv->command != NULL) + return; + braces (and everywhere below) @@ +272,3 @@ + * currently lacks support for multi-seat hotplugging for + * display devices. This bit should be removed as soon as XOrg + * gains native support for display hotplugging. */ s/display hotplugging/automatically enumerating usb based graphics adapters at start up/
(In reply to comment #68) > Review of attachment 205942 [details] [review]: > > i still think it might make sense to point out the property was never used for > anything from its inception, but not a big deal. Oh, it actually used to be used for something IIRC, i.e. before your PAM work got merged, and the thing did CK registration on its own. Which is why I just say in the commit message that it isn't used before that patch.
(In reply to comment #75) > Review of attachment 205949 [details] [review]: > > You can commit after the fix up below. > > ::: daemon/gdm-factory-slave.c > @@ +691,3 @@ > gboolean res; > > + slave->priv->server = gdm_server_new (display_name, seat_id, > auth_file); > > You need to pass NULL here if seat_id is from consolekit i think. The X server > can't do anything with a consolekit seat right? (Likewise for the other two > slaves) Good point! I have now modified the bits that generate the command line to drop the -seat argument unless systemd is used. I think it's a good idea to pass the seat id as far as possible, in order to make it simple for people to grok this, and maybe even for folks who want to port the multi-seat stuff to other backends.
(In reply to comment #77) > (In reply to comment #68) > > Review of attachment 205942 [details] [review] [details]: > > > > i still think it might make sense to point out the property was never used for > > anything from its inception, but not a big deal. > > Oh, it actually used to be used for something IIRC, i.e. before your PAM work > got merged, and the thing did CK registration on its own. Which is why I just > say in the commit message that it isn't used before that patch. If i type 'git log -p' and then hit / and then type register.ck.session i see it getting set, but never ... oh uhh nevermind. I just screwed up the first time I looked. Never mind. You might want to point out the property became vestigial in commit 647cad5bf59a4ff3776ba1ae5cda6b1aaaa1cfb2
(In reply to comment #78) > (In reply to comment #75) > > Review of attachment 205949 [details] [review] [details]: > > > > You can commit after the fix up below. > > > > ::: daemon/gdm-factory-slave.c > > @@ +691,3 @@ > > gboolean res; > > > > + slave->priv->server = gdm_server_new (display_name, seat_id, > > auth_file); > > > > You need to pass NULL here if seat_id is from consolekit i think. The X server > > can't do anything with a consolekit seat right? (Likewise for the other two > > slaves) > > Good point! I have now modified the bits that generate the command line to drop > the -seat argument unless systemd is used. I think it's a good idea to pass the > seat id as far as possible, in order to make it simple for people to grok this, > and maybe even for folks who want to port the multi-seat stuff to other > backends. okay, can you post your updated patch?
Created attachment 206231 [details] [review] slave: add native systemd implementations of locking/session activation
Created attachment 206232 [details] [review] build-sys: make CK support optional
Created attachment 206233 [details] [review] slave: pass seat id from product/simple slave to worker, via the direct session
Created attachment 206234 [details] [review] pam: pass XDG_SEAT env var into PAM to inform PAM modules about seat id
Created attachment 206235 [details] [review] local-display-factory: subscribe to new seats being created and removed
Created attachment 206236 [details] [review] server: pass seat id to server
Created attachment 206237 [details] [review] server: invoke X with the systemd multi seat X wrapper if necessary
OK, hope this time I didn't forget anything. Made all the requested changes (including the "optional" ones). Hope this is OK.
Review of attachment 206237 [details] [review]: Are you planning on changing configure.ac to use this: http://cgit.freedesktop.org/systemd/systemd/commit/?id=ab8da6c266d2a72f13182be1fa2d3301197d33c8 ?
(In reply to comment #89) > Review of attachment 206237 [details] [review]: > > Are you planning on changing configure.ac to use this: > > http://cgit.freedesktop.org/systemd/systemd/commit/?id=ab8da6c266d2a72f13182be1fa2d3301197d33c8 > > ? Eventually yes (which is why I added this to system...), but I am not planning to do a release of systemd in the next days, so if you don't mind I'd like to commit this like this this to gdm for now and update the configure.ac bit in gdm as soon as we have released the next systemd. (the thing is also, that we have renamed the .pc variable since I commited it, sicne we aren't clear yet on how it will be named in the end...).
So I found another big issue with this, playing around with it... If the primary system is switched to a kernel virtual console then everything typed in a seats X session is sent to the tty. This includes the user's password, and for instance, all this text going into this text entry box now. Before this is useful, we need a way to tell the kernel to ignore this input device on tty's and only export the device via /dev/input .
If i cat /proc/input/devices i see this for my keyboard: N: Name="Unicomp Endura Pro Keyboard " P: Phys=usb-0000:00:1d.0-1.3.6/input1 S: Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.3/2-1.3.6/2-1.3.6:1.1/input/input18 U: Uniq= H: Handlers=kbd mouse2 event18 B: PROP=0 B: EV=1f B: KEY=4837fff072ff32d bf54444600000000 70001 20c100b17c000 267bfad941dfed 9e168000004400 10000002 B: REL=143 B: ABS=100000000 B: MSC=10 I'm assuming we only want event18 there and not kbd or mouse2.
Lennart and I talked about this on IRC and I was able to get this working with some changes to the Xorg config file his mult-seat-x program writes out. Namely, it needs the DontVTSwitch option in serverflags, and the GrabDevice option in a new InputClass section.
(In reply to comment #91) > So I found another big issue with this, playing around with it... If the > primary system is switched to a kernel virtual console then everything typed in > a seats X session is sent to the tty. This includes the user's password, and > for instance, all this text going into this text entry box now. Before this is > useful, we need a way to tell the kernel to ignore this input device on tty's > and only export the device via /dev/input . OK, fixed now in systemd git, with your patch (Thanks!). Will upload a new version to rawhide soon.
BTW, after the discussions on IRC what's missing seem to be two things, right? - gdmflexiserver needs porting - And when we activate an existing session we should do so only if its on the same seat. Anything else I forgot?
that's right. Also, docs, but I can add that afterward unless you're feeling creative.
Created attachment 207016 [details] [review] slave: use new ActivateSessionOnSeat call Here's an (incremental) patch that should fix the issue that we try to actviate a session on a different seat if the user logs in twice. I made this incremental for now (sometimes makes sense not to hide the history of this...), but I can merge this into patch 206231 if needed.
merge it! I care more about clarity for people looking through the history, so they are able to understand why specific changes went in, than being "true to reality"
Created attachment 207027 [details] [review] gdmflexiserver: port gdmflexiserver to libsystemd-logind The last missing bit.
Created attachment 207028 [details] [review] slave: add native systemd implementations of locking/session activation Merged patch 207016 and patch 206231 (and fixed one buglet)
Created attachment 207030 [details] [review] gdmflexiserver: port gdmflexiserver to libsystemd-logind Updated patch that explicitly checks whether the seat gdmflexiserver is run on is multi-seat capable.
Created attachment 207033 [details] [review] gdmflexiserver: port gdmflexiserver to libsystemd-logind Another iteration of the gdmflexiserver work, this time clearing the GError properly.
Created attachment 207034 [details] [review] gdmflexiserver: port gdmflexiserver to libsystemd-logind Another iteration: don't fall back to CK if we know that we are on a systemd system.
I've been talking with lennart about these patches on IRC. I think they're ready go in now, although, I assume there's going to be need to be some follow up work on gnome-screensaver etc. Also, I want to write brief docs before doing a release.
OK, committed. Thanks for the review and everything!
This should be closed, I think ?
Nah, still need to do docs, and I won't remember if this doesn't linger.
Also, gdm-server.c still calls a binary shipped with consolekit, so we need to copy that binary into systemd too I guess, or do what it does in some other way.
Also we need to change the default pam configs to be reasonable for systemd/ck-connector as appropriate.
(In reply to comment #108) > Also, gdm-server.c still calls a binary shipped with consolekit, so we need to > copy that binary into systemd too I guess, or do what it does in some other > way. Hmm, which binary?
daemon/gdm-server.c: command = g_strdup_printf (LIBEXECDIR "/ck-get-x11-display-device --display %s",
Also, in gdm_local_display_factory_create_transient_display there's this: /* FIXME: don't hardcode seat1? */ g_object_set (display, "seat-id", CK_SEAT1_PATH, NULL) (and elsewhere) So at a minimum that needs to say "seat0" in the systemd code path. Though better, would be if user switching sessions went through systemd too. See: http://lists.freedesktop.org/archives/systemd-devel/2012-February/004510.html
Just cleaning up the attachment status of already commited stuff...
Created attachment 210096 [details] [review] user-switching: fix seat id confusion There's a few places in the code that was hardcoding consolekit paths still. This commit does some small changes to make that more systemd compatible.
Created attachment 210109 [details] [review] user-switching: don't set PAM_TTY when using systemd It does it for us, and we don't know it anyway, so don't bother trying.
Created attachment 210110 [details] [review] user-switching: don't bail if login screen isn't running It's okay if no login screen is running, we can just launch one.
Created attachment 210111 [details] [review] user-switching: fix sd_session_get_uid check It returns 0 on success.
Created attachment 210112 [details] [review] user-switching: fix session migration It wasn't properly jumping to the correct vt.
Attachment 210109 [details] pushed as 932c65c - user-switching: don't set PAM_TTY when using systemd Attachment 210110 [details] pushed as 7ae2c9c - user-switching: don't bail if login screen isn't running Attachment 210111 [details] pushed as ad7b185 - user-switching: fix sd_session_get_uid check Attachment 210112 [details] pushed as fb6a3cf - user-switching: fix session migration I'm going to close this now. I'll file a separate bug for the missing docs.