Merge lp:~ubuntu-multiseat/lightdm/new-automatic-multiseat into lp:lightdm

[Laércio de Sousa]

This new branch implements automatic multiseat support in LightDM, following steps 1 & 2 of "Complete porting" section in systemd-logind documentation [1]. This is a rework of old branch lp:~ubuntu-multiseat/lightdm/automatic-multiseat after introduction of login1 service object in upstream revision 2028.

This new implementation has some additional features, added after discussion with Robert Ancell:

 * If logind is running, LightDM won't try to load seats statically as before. Instead, any [Seat:seat*] config sections in lightdm.conf will be loaded on demand, as new seats are added from logind. Since logind returns at least one seat (namely, seat0), start-default-seat condition should never be reached in this case.

 * Seat properties "seat-name" and "xdg-seat" will be both automatically set to return value of function login1_seat_get_id(). It opens the possibility of merging both properties, or deprecating "xdg-seat", in the near future.

At the moment, this branch has some issues:

1. Test scripts have not been added yet.

2. What should it do if a logind seat has property CanGraphical=no?

3. It still doesn't watch PropertyChanged events on logind seats. What should it do if logind seat property CanGraphical changes?

[1] http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/

[lp:~ubuntu-multiseat/lightdm/new-automatic-multiseat]

2036. By Laércio de Sousa on 2014-08-22

Add a debug message before start monitoring logind for new/removed seats

2037. By Laércio de Sousa on 2014-08-22

Remove uneeded g_variant_unref() call inside g_variant_iter_loop().

See documentation at https://developer.gnome.org/glib/2.41/glib-GVariant.html#g-variant-iter-loop

2038. By Laércio de Sousa on 2014-08-22

Add/Update some debug messages.

2039. By Laércio de Sousa on 2014-08-25

Ensure start-default-seat condition will never be reached when
adding seats from logind.

[Laércio de Sousa (lbssousa)]

[NEW ISSUE] Should we support simple globbing in seat config sections when loading seats from logind? Examples:

[Seat:seatFoo*] would match any logind seat whose id starts with "seatFoo".

Side effect: [Seat:seat*], or just [Seat:*], would match any seat (including seat0), just like [SeatDefaults] does.

[lp:~ubuntu-multiseat/lightdm/new-automatic-multiseat]

2040. By Laércio de Sousa on 2014-08-25

Revert changes made in revision 2032.

[Robert Ancell (robert-ancell)]

I was also thinking about globbing. We should do it as a separate MP though as it extends the current behaviour. Please open a new bug/mp.

[Robert Ancell (robert-ancell)]

> seat = seat_new ("xlocal");

This should use the same logic as before for picking the seat type.

Reading the logind documentation we should ignore seats that have CanGraphical=false. We should watch for property changes on this, but it's not a blocker to landing this branch (since most real world cases probably wont have this change) - we can add it into another MP later.

Needs at least one regression test. tests/scripts/multi-seat.conf breaks with this change (as expected). You will need to add #?*ADD-SEAT lines into this to make it work.

Otherwise looks good.

review: Needs Fixing

[lp:~ubuntu-multiseat/lightdm/new-automatic-multiseat]

2041. By Laércio de Sousa on 2014-08-26

Update test-multi-seat script to deal with seats added/removed dinamically
from logind.

2042. By Laércio de Sousa on 2014-08-26

Make seat type configurable for seats added from logind.

2043. By Laércio de Sousa on 2014-08-26

Fix typo in g_strfreev() call.

[Laércio de Sousa (lbssousa)]

> > seat = seat_new ("xlocal");
>
> This should use the same logic as before for picking the seat type.

OK. Seat type is configurable now. Done in revisions 2042/2043.

> Reading the logind documentation we should ignore seats that have
> CanGraphical=false. We should watch for property changes on this, but it's not
> a blocker to landing this branch (since most real world cases probably wont
> have this change) - we can add it into another MP later.

OK.

> Needs at least one regression test. tests/scripts/multi-seat.conf breaks with
> this change (as expected). You will need to add #?*ADD-SEAT lines into this to
> make it work.

I'm still learning how to write test scripts for lightdm, nevertheless I've updated test/scripts/multi-seat.conf in revision 2041. For some reason, I was unable to test it in my local VirtualBox VM installation (running "make check" results in 100+ fails in my system). Could you please take a look and tell me if there's any errors in it?

[lp:~ubuntu-multiseat/lightdm/new-automatic-multiseat]

2044. By Laércio de Sousa on 2014-08-26

Use login1_service_get_instance() more extensively in lightdm.c.

2045. By Laércio de Sousa on 2014-08-26

Fix mistakes in updated test-multi-seat script.

[Robert Ancell (robert-ancell)]

I've pushed this with some changes. The particular things that were causing the tests to break:
- None of the logind created seats were marked with "exit-on-failure" so the daemon didn't stop in the failure tests
- Failure to start any seats didn't cause the daemon to exit like it used to
- The multi-seat test script had "#*?ADD-SEAT" instead of "#?*ADD-SEAT"
- There was a bug in my mock login1 interface that didn't export the seat properties

I also took the opportunity to deprecate the old manual seat method. We'll see if anyone screams and if we need to add it back (my guess is probably not).

review: Approve

=== modified file 'src/lightdm.c'
--- src/lightdm.c	2014-08-22 09:01:20 +0000
+++ src/lightdm.c	2014-08-26 14:51:50 +0000
@@ -979,6 +979,96 @@
     exit (EXIT_FAILURE);
 }
 
+static void
+login1_service_seat_added_cb (Login1Service *service, Login1Seat *login1_seat)
+{
+    const gchar *seat_name = login1_seat_get_id (login1_seat);
+    gchar **groups, **i;
+    gchar *config_section = NULL;
+    gchar **types = NULL, **type;
+    Seat *seat = NULL;
+
+    g_debug ("New seat added from logind: %s", seat_name);
+    groups = config_get_groups (config_get_instance ());
+
+    for (i = groups; !config_section && *i; i++)
+    {
+        if (g_str_has_prefix (*i, "Seat:") &&
+            g_str_has_suffix (*i, seat_name))
+            config_section = *i;
+    }
+
+    if (config_section)
+    {
+        g_debug ("Loading properties from config section %s", config_section);
+        types = config_get_string_list (config_get_instance (), config_section, "type");
+    }
+
+    if (!types)
+        types = config_get_string_list (config_get_instance (), "SeatDefaults", "type");
+
+    for (type = types; !seat && type && *type; type++)
+        seat = seat_new (*type);
+
+    g_strfreev (types);
+
+    if (seat)
+    {
+        set_seat_properties (seat, NULL);
+
+        if (!login1_seat_get_can_multi_session (login1_seat))
+        {
+            g_debug ("Seat %s has property CanMultiSession=no", seat_name);
+            seat_set_property (seat, "allow-user-switching", "false");
+        }
+
+        if (config_section)
+            set_seat_properties (seat, config_section);
+
+        seat_set_property (seat, "seat-name", seat_name);
+        seat_set_property (seat, "xdg-seat", seat_name);
+        g_strfreev (groups);
+    }
+    else
+    {
+        // FIXME: Need to make proper error
+        g_warning ("Unable to create seat: %s", seat_name);
+        g_strfreev (groups);
+        return;
+    }
+
+    if (!display_manager_add_seat (display_manager, seat)) // FIXME: Need to make proper error
+        g_warning ("Failed to start seat: %s", seat_name);
+
+    g_object_unref (seat);
+}
+
+static void
+login1_service_seat_removed_cb (Login1Service *service, Login1Seat *login1_seat)
+{
+    GList *seats, *link;
+    Seat *seat;
+    const gchar *seat_name = login1_seat_get_id (login1_seat);
+
+    /* Stop all seats matching given xdg-seat property value.
+     * Copy the list as it might be modified if a seat stops during this loop */
+    seats = g_list_copy (display_manager_get_seats (display_manager));
+
+    /* FIXME: This loop should be uneeded, provided we can ensure
+     *        there's only one Seat object in DisplayManager list
+     *        matching given Login1Seat object id. */
+    g_debug ("Seat removed from logind: %s", seat_name);
+    for (link = seats; link; link = link->next)
+    {
+        seat = link->data;
+
+        if (g_strcmp0 (seat_get_name (seat), seat_name) == 0)
+            seat_stop (seat);
+    }
+
+    g_list_free (seats);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -986,7 +1076,6 @@
     GOptionContext *option_context;
     gboolean result;
     gchar **groups, **i, *dir;
-    gint n_seats = 0;
     gboolean test_mode = FALSE;
     gchar *pid_path = "/var/run/lightdm.pid";
     gchar *log_dir = NULL;
@@ -1311,78 +1400,91 @@
     shared_data_manager_start (shared_data_manager_get_instance ());
 
     /* Connect to logind */
-    login1_service_connect (login1_service_get_instance ());
-
-    /* Load the static display entries */
-    groups = config_get_groups (config_get_instance ());
-    for (i = groups; *i; i++)
-    {
-        gchar *config_section = *i;
-        gchar **types;
-        gchar **type;
-        Seat *seat = NULL;
-        const gchar *const seatpfx = "Seat:";
-
-        if (!g_str_has_prefix (config_section, seatpfx))
-            continue;
-
-        g_debug ("Loading seat %s", config_section);
-        types = config_get_string_list (config_get_instance (), config_section, "type");
-        if (!types)
+    if (login1_service_connect (login1_service_get_instance ()))
+    {
+        /* Load dynamic seats from logind */
+        g_debug ("Start monitoring logind for new/removed seats");
+        g_signal_connect (login1_service_get_instance (), "seat-added", G_CALLBACK (login1_service_seat_added_cb), NULL);
+        g_signal_connect (login1_service_get_instance (), "seat-removed", G_CALLBACK (login1_service_seat_removed_cb), NULL);
+
+        for (link = login1_service_get_seats (login1_service_get_instance ()); link; link = link->next)
+            login1_service_seat_added_cb (login1_service_get_instance (), (Login1Seat *) link->data);
+    }
+    else
+    {
+        gint n_seats = 0;
+
+        /* Load the static display entries */
+        groups = config_get_groups (config_get_instance ());
+        for (i = groups; *i; i++)
+        {
+            gchar *config_section = *i;
+            gchar **types;
+            gchar **type;
+            Seat *seat = NULL;
+            const gchar *const seatpfx = "Seat:";
+
+            if (!g_str_has_prefix (config_section, seatpfx))
+                continue;
+
+            g_debug ("Loading seat %s", config_section);
+            types = config_get_string_list (config_get_instance (), config_section, "type");
+            if (!types)
+                types = config_get_string_list (config_get_instance (), "SeatDefaults", "type");
+            for (type = types; type && *type; type++)
+            {
+                seat = seat_new (*type);
+                if (seat)
+                    break;
+            }
+            g_strfreev (types);
+            if (seat)
+            {
+                const gsize seatpfxlen = strlen(seatpfx);
+                gchar *seatname = config_section + seatpfxlen;
+
+                seat_set_property (seat, "seat-name", seatname);
+
+                set_seat_properties (seat, config_section);
+                display_manager_add_seat (display_manager, seat);
+                g_object_unref (seat);
+                n_seats++;
+            }
+            else
+                g_warning ("Failed to create seat %s", config_section);
+        }
+        g_strfreev (groups);
+
+        /* If no seats start a default one */
+        if (n_seats == 0 && config_get_boolean (config_get_instance (), "LightDM", "start-default-seat"))
+        {
+            gchar **types;
+            gchar **type;
+            Seat *seat = NULL;
+
+            g_debug ("Adding default seat");
+
             types = config_get_string_list (config_get_instance (), "SeatDefaults", "type");
-        for (type = types; type && *type; type++)
-        {
-            seat = seat_new (*type);
-            if (seat)
-                break;
-        }
-        g_strfreev (types);
-        if (seat)
-        {
-            const gsize seatpfxlen = strlen(seatpfx);
-            gchar *seatname = config_section + seatpfxlen;
-
-            seat_set_property (seat, "seat-name", seatname);
-
-            set_seat_properties (seat, config_section);
-            display_manager_add_seat (display_manager, seat);
-            g_object_unref (seat);
-            n_seats++;
-        }
-        else
-            g_warning ("Failed to create seat %s", config_section);
-    }
-    g_strfreev (groups);
-
-    /* If no seats start a default one */
-    if (n_seats == 0 && config_get_boolean (config_get_instance (), "LightDM", "start-default-seat"))
-    {
-        gchar **types;
-        gchar **type;
-        Seat *seat = NULL;
-
-        g_debug ("Adding default seat");
-
-        types = config_get_string_list (config_get_instance (), "SeatDefaults", "type");
-        for (type = types; type && *type; type++)
-        {
-            seat = seat_new (*type);
-            if (seat)
-                break;
-        }
-        g_strfreev (types);
-        if (seat)
-        {
-            set_seat_properties (seat, NULL);
-            seat_set_property (seat, "exit-on-failure", "true");
-            if (!display_manager_add_seat (display_manager, seat))
+            for (type = types; type && *type; type++)
+            {
+                seat = seat_new (*type);
+                if (seat)
+                    break;
+            }
+            g_strfreev (types);
+            if (seat)
+            {
+                set_seat_properties (seat, NULL);
+                seat_set_property (seat, "exit-on-failure", "true");
+                if (!display_manager_add_seat (display_manager, seat))
+                    return EXIT_FAILURE;
+                g_object_unref (seat);
+            }
+            else
+            {
+                g_warning ("Failed to create default seat");
                 return EXIT_FAILURE;
-            g_object_unref (seat);
-        }
-        else
-        {
-            g_warning ("Failed to create default seat");
-            return EXIT_FAILURE;
+            }
         }
     }
 

=== modified file 'src/login1.c'
--- src/login1.c	2014-08-22 01:56:45 +0000
+++ src/login1.c	2014-08-26 14:51:50 +0000
@@ -269,7 +269,6 @@
                 seat->priv->can_graphical = g_variant_get_boolean (value);
             else if (strcmp (name, "CanMultiSession") == 0 && g_variant_is_of_type (value, G_VARIANT_TYPE_BOOLEAN))
                 seat->priv->can_multi_session = g_variant_get_boolean (value);
-            g_variant_unref (value);
         }
         g_variant_iter_free (properties);
         g_variant_unref (result);

=== modified file 'tests/scripts/multi-seat.conf'
--- tests/scripts/multi-seat.conf	2014-03-17 18:33:02 +0000
+++ tests/scripts/multi-seat.conf	2014-08-26 14:51:50 +0000
@@ -1,17 +1,15 @@
 #
-# Check can run two seats at once
+# Check can run two seats at once (added from logind)
 #
 
-[Seat:0]
-xdg-seat=seat0
-
-[Seat:1]
-xdg-seat=seat1
-
 #?*START-DAEMON
 #?RUNNER DAEMON-START
 
-# Seat0 starts
+# Check if seat0 was added
+#*?LIST-SEATS
+#?RUNNER LIST-SEATS SEATS=/org/freedesktop/DisplayManager/Seat0
+
+# seat0 starts
 #?XSERVER-0 START VT=7 SEAT=seat0
 #?*XSERVER-0 INDICATE-READY
 #?XSERVER-0 INDICATE-READY
@@ -23,22 +21,36 @@
 #?GREETER-X-0 CONNECT-TO-DAEMON
 #?GREETER-X-0 CONNECTED-TO-DAEMON
 
-# Seat1 starts (can't use VTs)
-#?XSERVER-1 START SEAT=seat1
+# Add seat-1 (can't use VTs)
+#*?ADD-SEAT ID=seat-1 CAN-GRAPHICAL=TRUE CAN-MULTI-SESSION=FALSE
+
+# Check if seat-1 was added
+#*?LIST-SEATS
+#?RUNNER LIST-SEATS SEATS=/org/freedesktop/DisplayManager/Seat0,/org/freedesktop/DisplayManager/Seat1
+
+# seat-1 starts
+#?XSERVER-1 START SEAT=seat-1
 #?*XSERVER-1 INDICATE-READY
 #?XSERVER-1 INDICATE-READY
 #?XSERVER-1 ACCEPT-CONNECT
-#?GREETER-X-1 START XDG_SEAT=seat1 XDG_SESSION_CLASS=greeter
+#?GREETER-X-1 START XDG_SEAT=seat-1 XDG_SESSION_CLASS=greeter
 #?LOGIN1 ACTIVATE-SESSION SESSION=c1
 #?XSERVER-1 ACCEPT-CONNECT
 #?GREETER-X-1 CONNECT-XSERVER
 #?GREETER-X-1 CONNECT-TO-DAEMON
 #?GREETER-X-1 CONNECTED-TO-DAEMON
 
+# Remove seat-1
+#*REMOVE-SEAT ID=seat-1
+#?GREETER-X-1 TERMINATE SIGNAL=15
+#?XSERVER-1 TERMINATE SIGNAL=15
+
+# Check if seat-1 was removed
+#*?LIST-SEATS
+#?RUNNER LIST-SEATS SEATS=/org/freedesktop/DisplayManager/Seat0
+
 # Cleanup
 #?*STOP-DAEMON
 #?GREETER-X-0 TERMINATE SIGNAL=15
-#?GREETER-X-1 TERMINATE SIGNAL=15
 #?XSERVER-0 TERMINATE SIGNAL=15
-#?XSERVER-1 TERMINATE SIGNAL=15
 #?RUNNER DAEMON-EXIT STATUS=0