Merge lp:~andrzejtp2010/lightdm/lightdm-trunk-xephyr-multiseat into lp:lightdm

[Andrzej Pietrasiewicz]

Dear All,

There are 2 patches proposed for merging, and they add the following:

- On closing an xserver-local remove the corresponding lock file from the temporary directory; stale files impact the choice of DISPLAY numbers in subsequent runs of lightdm

- Optionally use the same vt number for all seats

These changes are required for xephyr-based multiseat configuration to work. Please see beforeafterx.blogspot.com for more information.

The first patch is a cleanup useful for all use cases. The second patch by default does not change the already existing behaviour so it won't hurt.

I kindly ask for a review,

Andrzej

[Robert Ancell (robert-ancell)]

Hi Andrzej,

The first change of removing the lock file is the wrong thing to do. The lock file is owned by the X server process and should only be removed by the X server in question. What is the reason for this change?

The second change of using a common VT doesn't seem required. As far as I can tell Xephyr ignores the VT option (as it's not required) so I don't see a need for this.

review: Needs Information

[Andrzej Pietrasiewicz (andrzejtp2010)]

Hello Robert,

On 10/01/12 03:57, Robert Ancell wrote:
> Review: Needs Information

Thank you for your review, please see my comments inline.

<snip>

> The first change of removing the lock file is the wrong thing to do. The lock file is owned by the X server process and should only be removed by the X server in question. What is the reason for this change?

After X server crashes the lock file remains there; after restarting the
lightdm the DISPLAY number is picked by it but if the lock file is still
there the number cannot be recycled. E.g. if there are 3 servers (:0,
:1, :2) and :1 crashes, then after restarting lightdm the :1 cannot be
reused and :0, :2, :3 are started instead.

>
> The second change of using a common VT doesn't seem required. As far as I can tell Xephyr ignores the VT option (as it's not required) so I don't see a need for this.

The change is intended for multiseat local configurations. By multiseat
configuration I mean sharing one computer by e.g. 2 people
simultaneously: there are 2 keyboards, 2 mice and 2 monitors, and 2
independent X sessions. In fact using 2 monitors is only for
convenience; technically speaking sharing one monitor to display 2 X
sessions side-by-side is just as good (a nice picture with as many as 4
gdms on one monitor is here:
http://4.bp.blogspot.com/_5K-T2ymLHSo/TU60HMooXEI/AAAAAAAAAAc/SDH27atTA2o/s1600/IMGP5282.JPG).
The idea is that there is one X server covering all available monitors,
and there are a number of Xephyrs running inside it, one per each user.
The Xephyrs are configured so that they use the evdev protocol and read
keyboards/mice directly from /dev/input/eventXX files. And in each
Xephyr a greeter is run (e.g. ligthdm-gtk-greeter). Once we have a
greeter, we can start X session and we are all set.

Here is my lightdm.conf:

[LightDM]
greeter-user=root
user-authority-in-system-dir=false
common-vt=true

[SeatDefaults]
xserver-allow-tcp=false
greeter-hide-users=true
user-session=lightdm-xsession
session-wrapper=/etc/X11/Xsession

[Seat:0]
xserver-command=X -br -dpms -s 0 -novtswitch
greeter-session=xinerama

[Seat:1]
xserver-command=/etc/multiseat/scripts/Xephyr-seat-1
greeter-session=multiseat-greeter-1

[Seat:2]
xserver-command=/etc/multiseat/scripts/Xephyr-seat-2
greeter-session=multiseat-greeter-2

With this setup lightdm dutifully starts the 3 servers. And without the
patch in question the Seat:0 will run on vt7, Seat:1 will run on vt8 and
Seat:2 will run on vt9; it is possible to switch between them with
Ctrl-Alt-F7..9, but only one is visible at a time. And this is exactly
the opposite of what I want! Because I want all of them to be visible at
the same time.

Perhaps you can prove me wrong, which would otherwise be good as the
less features the better if things still can be done, but I haven't
manage to run multiseat without the patch adding common-vt. However,
with the patch applied, multiseat runs like a charm!

Just for your information, I've been using gdm for many (roughly 8)
years for this purpose and it just worked out of the box; and it was THE
display manager of choice for virtually all multiseat users all over the
world. It used to have...

[Robert Ancell (robert-ancell)]

OK, so the two issues are:

1. LightDM doesn't handle stale X server locks. I've fixed this in lp:~robert-ancell/lightdm/stale-xserver-lock

2. I'm still not understanding the requirement to have all three X servers running on the same VT. Could you open a bug with this requirement and attach the logs from /var/log/lightdm? I'm not sure if LightDM can properly handle the case you're describing - you have Seat:0 being the main X server and Seat:1 and Seat:2 running inside it. There's nothing to stop Seat:1 starting before Seat:0 and if Seat:0 and Seat:1 are Xephyr servers then they shouldn't have VTs?!

[Andrzej Pietrasiewicz (andrzejtp2010)]

Hello Robert,

thank you for addressing the issue,

W dniu 16.04.2013 04:26, Robert Ancell pisze:
> OK, so the two issues are:
>
>
> 1. LightDM doesn't handle stale X server locks. I've fixed this in lp:~robert-ancell/lightdm/stale-xserver-lock
>
> 2. I'm still not understanding the requirement to have all three X servers running on the same VT. Could you open a bug with this requirement and attach the logs from /var/log/lightdm? I'm not sure if LightDM can properly handle the case you're describing - you have Seat:0 being the main X server and Seat:1 and Seat:2 running inside it. There's nothing to stop Seat:1 starting before Seat:0 and if Seat:0 and Seat:1 are Xephyr servers then they shouldn't have VTs?!

Here is the bug report:

https://bugs.launchpad.net/lightdm/+bug/1169724

I confirm that without the common-vt patch the solution does not work
but with the patch it works well. The difference in the log files that I
was able to spot is that in the successful run all the X servers report
using and activating the VT7, while in the unsuccessful run each X
server has its own VT number, e.g. VT7, VT8, VT9 and the computer locks
to display a blank console screen and a blinking cursor. No combinations
like Ctrl+Alt+F<X> work, nor Alt+F<x> to switch out of this locked
state; it is possible to ssh into the machine, though.

Thanks,

Andrzej

[Robert Ancell (robert-ancell)]

As discussed in bug 1169724 this merge doesn't work with the way LightDM uses VTs for session switching.

review: Disapprove

Unmerged revisions

1536. By Andrzej Pietrasiewicz on 2012-08-19

Optionally use a common vt number for each seat, FALSE by default.

1535. By Andrzej Pietrasiewicz on 2012-08-19

Unlink lock file in the temporary directory on stopped_cb

=== modified file 'src/lightdm.c'
--- src/lightdm.c	2012-05-14 14:52:37 +0000
+++ src/lightdm.c	2012-08-19 11:56:23 +0000
@@ -986,6 +986,8 @@
         config_set_boolean (config_get_instance (), "LightDM", "start-default-seat", TRUE);
     if (!config_has_key (config_get_instance (), "LightDM", "minimum-vt"))
         config_set_integer (config_get_instance (), "LightDM", "minimum-vt", 7);
+    if (!config_has_key (config_get_instance (), "LightDM", "common-vt"))
+        config_set_boolean (config_get_instance (), "LightDM", "common-vt", FALSE);
     if (!config_has_key (config_get_instance (), "LightDM", "guest-account-script"))
         config_set_string (config_get_instance (), "LightDM", "guest-account-script", "guest-account");
     if (!config_has_key (config_get_instance (), "LightDM", "greeter-user"))
 
=== modified file 'src/vt.c'
--- src/vt.c	2012-06-05 04:25:47 +0000
+++ src/vt.c	2012-08-19 11:56:23 +0000
@@ -125,15 +125,23 @@
 gint
 vt_get_unused (void)
 {
+    static gint common_number = -1;
+
+    gboolean common_vt;
     gint number;
 
     if (getuid () != 0)
         return -1;
 
+    common_vt = config_get_boolean (config_get_instance (), "LightDM", "common-vt");
+    if (common_vt && common_number > 0)
+	return common_number;
+
     number = vt_get_min ();
     while (vt_is_used (number))
         number++;
  
+    common_number = number;
     return number;
 }
 
 
=== modified file 'src/xserver-local.c'
--- src/xserver-local.c	2012-03-20 05:47:56 +0000
+++ src/xserver-local.c	2012-08-19 11:56:23 +0000
@@ -313,12 +313,16 @@
 static void
 stopped_cb (Process *process, XServerLocal *server)
 {
+    guint display_number;
+    gchar *path;
+
     g_debug ("X server stopped");
 
     g_object_unref (server->priv->xserver_process);
     server->priv->xserver_process = NULL;
 
-    xserver_local_release_display_number (xserver_get_display_number (XSERVER (server)));
+    display_number = xserver_get_display_number (XSERVER (server));
+    xserver_local_release_display_number (display_number);
   
     if (xserver_get_authority (XSERVER (server)) && server->priv->authority_file)
     {
@@ -351,6 +355,10 @@
         plymouth_quit (FALSE);
     }
 
+    path = g_strdup_printf ("/tmp/.X%d-lock", display_number);
+    g_unlink (path);
+    g_free (path);
+
     DISPLAY_SERVER_CLASS (xserver_local_parent_class)->stop (DISPLAY_SERVER (server));
 }