Merge lp:~ubuntu-multiseat/lightdm/simple-globbing-in-seat-config-section into lp:lightdm

[Laércio de Sousa]

[UPDATE] Resubmitting this merge proposal after Robert Ancell's updates

Add support for simple globbing in seat config sections.

Example: if a config section [Seat:seatFoo*] is available in lightdm.conf, it will match any seat name starting with "seatFoo", like "seatFooBar", "seatFooBaz", etc.

In particular, [Seat:seat*] will match any seat added from logind (including seat0), and [Seat:*] will match any seat (including those eventually added from other mechanisms than logind), just as [SeatDefaults] currently does.

[Laércio de Sousa (lbssousa)]

[NEW ISSUE] New function get_config_section() should return a list of all available globbing config sections that match a given seat name (including the exact match), not only the first occurrence.

[Robert Ancell (robert-ancell)]

Yes, you could match against n-sections. Also, you should use the GLib pattern matcher [1]

[1] https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.html

review: Needs Fixing

[Robert Ancell (robert-ancell)]

Will also need a regression test to confirm this matching works

[Laércio de Sousa (lbssousa)]

> Yes, you could match against n-sections. Also, you should use the GLib pattern
> matcher [1]
>
> [1] https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-
> matching.html

Done.

[Laércio de Sousa (lbssousa)]

> Will also need a regression test to confirm this matching works

I've written a regression test that starts 3 seats and enables autologin as guest with different timeouts for the non-seat0 ones. My local "make check" returns FAIL to this test, but I could not see any relevant error message in test log.

Could you review it, please?

[Laércio de Sousa (lbssousa)]

[NEW IDEA] Support wildcard %s when setting seat property values, which will be replaced with corresponding seat name.

[Laércio de Sousa (lbssousa)]

Example:

[Seat:seat-*]
xserver-command=X -config xorg.conf.%s

[Robert Ancell (robert-ancell)]

There were some typos in the script but the main reason was the seat IDs (e.g. "seat-AAA-Foo") were being converted into invalid D-Bus address names (e.g. "/org/freedesktop/login1/seat/seat-AAA-Foo" - "_" is not allowed in D-Bus addresses). So there was a bug in my mock login1 code in tests/src/test-runner.c

I fixed that up and simplified the test.

Thanks!

[Robert Ancell (robert-ancell)]

The wildcard idea sounds good, things to think about:
- Does the wildcard work in all config values or just command?
- Could this wildcard be a valid value in other cases and will those values now have to be escaped (potentially breaking existing config)? For this reason, we should use a bigger token, e.g. "%(seat-name)" which is both more self-explanatory and less likely to collide with something else.
- What are the escape rules if you need a literal "%s"?
- Are there other values we should give?

As always, please open a new MP and/or bug for this feature.

[lp:~ubuntu-multiseat/lightdm/simple-globbing-in-seat-config-section]

2047. By Laércio de Sousa on 2014-09-03

Reintroduce test after merge with trunk.

2048. By Laércio de Sousa on 2014-09-03

Remove dashes from logind seat IDs in test script

2049. By Laércio de Sousa on 2014-09-03

Fix iteration pointer when iterating backwards on GList object

2050. By Laércio de Sousa on 2014-09-03

Move debug message from add_login1_seat() to set_seat_properties() body.

2051. By Laércio de Sousa on 2014-09-10

Merge with trunk

2052. By Laércio de Sousa on 2014-09-10

Remove duplicated line after merging with trunk

[Laércio de Sousa (lbssousa)]

Just updated this branch after 1.11.9 release. Some conflicts resolved.

[Robert Ancell (robert-ancell)]

This is going to be too late for the 1.12 release, we will land it into 1.14.

[Robert Ancell (robert-ancell)]

Seems to be working now, thanks Laércio!

[Robert Ancell (robert-ancell)]

review: Approve

=== modified file 'src/lightdm.c'
--- src/lightdm.c	2014-09-10 03:33:25 +0000
+++ src/lightdm.c	2014-09-10 13:35:17 +0000
@@ -147,32 +147,54 @@
     g_free (path);
 }
 
+static GList*
+get_config_sections (const gchar *seat_name)
+{
+    gchar **groups, **i;
+    GList *config_sections = NULL;
+
+    config_sections = g_list_append (config_sections, g_strdup ("SeatDefaults"));
+
+    if (!seat_name)
+        return config_sections;
+
+    groups = config_get_groups (config_get_instance ());
+    for (i = groups; *i; i++)
+    {
+        if (g_str_has_prefix (*i, "Seat:"))
+        {
+            const gchar *seat_name_glob = *i + strlen ("Seat:");
+            if (g_pattern_match_simple (seat_name_glob, seat_name))
+                config_sections = g_list_append (config_sections, g_strdup (*i));
+        }
+    }
+    g_strfreev (groups);
+
+    return config_sections;
+}
+
 static void
-set_seat_properties (Seat *seat, const gchar *config_section)
+set_seat_properties (Seat *seat, const gchar *seat_name)
 {
+    GList *sections, *link;
     gchar **keys;
     gint i;
 
-    keys = config_get_keys (config_get_instance (), "SeatDefaults");
-    for (i = 0; keys && keys[i]; i++)
-    {
-        gchar *value = config_get_string (config_get_instance (), "SeatDefaults", keys[i]);
-        seat_set_property (seat, keys[i], value);
-        g_free (value);
-    }
-    g_strfreev (keys);
-
-    if (config_section)
-    {
-        keys = config_get_keys (config_get_instance (), config_section);
+    sections = get_config_sections (seat_name);
+    for (link = sections; link; link = link->next)
+    {
+        const gchar *section = link->data;
+        g_debug ("Loading properties from config section %s", section);
+        keys = config_get_keys (config_get_instance (), section);
         for (i = 0; keys && keys[i]; i++)
         {
-            gchar *value = config_get_string (config_get_instance (), config_section, keys[i]);
+            gchar *value = config_get_string (config_get_instance (), section, keys[i]);
             seat_set_property (seat, keys[i], value);
             g_free (value);
         }
         g_strfreev (keys);
     }
+    g_list_free_full (sections, g_free);
 }
 
 static void
@@ -223,11 +245,7 @@
 
     if (next_seat)
     {
-        gchar *config_section;
-
-        config_section = g_strdup_printf ("Seat:%s", seat_get_name (seat));
-        set_seat_properties (next_seat, config_section);
-        g_free (config_section);
+        set_seat_properties (next_seat, seat_get_name (seat));
 
         // We set this manually on default seat.  Let's port it over if needed.
         if (seat_get_boolean_property (seat, "exit-on-failure"))
@@ -940,42 +958,31 @@
 add_login1_seat (Login1Seat *login1_seat)
 {
     const gchar *seat_name = login1_seat_get_id (login1_seat);
-    gchar **groups, **i;
-    gchar *config_section = NULL;
     gchar **types = NULL, **type;
+    GList *config_sections = NULL, *link;
     Seat *seat = NULL;
     gboolean is_seat0, started = FALSE;
 
     g_debug ("New seat added from logind: %s", seat_name);
     is_seat0 = strcmp (seat_name, "seat0") == 0;
 
-    groups = config_get_groups (config_get_instance ());
-    for (i = groups; !config_section && *i; i++)
+    config_sections = get_config_sections (seat_name);
+    for (link = g_list_last (config_sections); link; link = link->prev)
     {
-        if (g_str_has_prefix (*i, "Seat:") &&
-            g_str_has_suffix (*i, seat_name))
-        {
-            config_section = g_strdup (*i);
+        gchar *config_section = link->data;
+        types = config_get_string_list (config_get_instance (), config_section, "type");
+        if (types)
             break;
-        }
-    }
-    g_strfreev (groups);
-
-    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");
+    }
+    g_list_free_full (config_sections, g_free);
+
     for (type = types; !seat && type && *type; type++)
         seat = seat_new (*type, seat_name);
     g_strfreev (types);
 
     if (seat)
     {
-        set_seat_properties (seat, NULL);
+        set_seat_properties (seat, seat_name);
 
         if (!login1_seat_get_can_multi_session (login1_seat))
         {
@@ -983,9 +990,6 @@
             seat_set_property (seat, "allow-user-switching", "false");
         }
 
-        if (config_section)
-            set_seat_properties (seat, config_section);
-
         if (is_seat0)
             seat_set_property (seat, "exit-on-failure", "true");
     }
@@ -999,7 +1003,6 @@
             g_debug ("Failed to start seat: %s", seat_name);
     }
 
-    g_free (config_section);
     g_object_unref (seat);
   
     return started;

=== modified file 'src/unity-system-compositor.c'
--- src/unity-system-compositor.c	2014-09-05 02:18:15 +0000
+++ src/unity-system-compositor.c	2014-09-10 13:35:17 +0000
@@ -85,6 +85,8 @@
 unity_system_compositor_set_command (UnitySystemCompositor *compositor, const gchar *command)
 {
     g_return_if_fail (compositor != NULL);
+    g_return_if_fail (command != NULL);
+
     g_free (compositor->priv->command);
     compositor->priv->command = g_strdup (command);
 }

=== modified file 'tests/Makefile.am'
--- tests/Makefile.am	2014-09-10 03:33:25 +0000
+++ tests/Makefile.am	2014-09-10 13:35:17 +0000
@@ -187,6 +187,7 @@
 	test-multi-seat-non-graphical-disabled \
 	test-multi-seat-change-graphical \
 	test-multi-seat-change-graphical-disabled \
+	test-multi-seat-globbing-config-sections \
 	test-mir-autologin \
 	test-mir-greeter \
 	test-mir-session \
@@ -456,6 +457,7 @@
 	scripts/multi-seat-non-graphical-disabled.conf \
 	scripts/multi-seat-seat0-non-graphical.conf \
 	scripts/multi-seat-seat0-non-graphical-disabled.conf \
+	scripts/multi-seat-globbing-config-sections.conf \
 	scripts/no-accounts-service.conf \
 	scripts/no-config.conf \
 	scripts/no-console-kit.conf \

=== added file 'tests/scripts/multi-seat-globbing-config-sections.conf'
--- tests/scripts/multi-seat-globbing-config-sections.conf	1970-01-01 00:00:00 +0000
+++ tests/scripts/multi-seat-globbing-config-sections.conf	2014-09-10 13:35:17 +0000
@@ -0,0 +1,105 @@
+#
+# Check can set globbing config sections matching different seats
+#
+
+[Seat:*]
+autologin-user=have-password1
+user-session=default
+
+[Seat:seat*Foo]
+autologin-user-timeout=99
+
+[Seat:seatAAAFoo]
+autologin-user=have-password2
+
+[Seat:seatBBB*]
+autologin-user=have-password3
+
+#?*START-DAEMON
+#?RUNNER DAEMON-START
+
+# seat0 starts
+#?XSERVER-0 START VT=7 SEAT=seat0
+#?*XSERVER-0 INDICATE-READY
+#?XSERVER-0 INDICATE-READY
+#?XSERVER-0 ACCEPT-CONNECT
+
+# Session starts for configured user
+#?SESSION-X-0 START XDG_SEAT=seat0 XDG_VTNR=7 XDG_GREETER_DATA_DIR=.*/have-password1 XDG_SESSION_TYPE=x11 XDG_SESSION_DESKTOP=default USER=have-password1
+#?LOGIN1 ACTIVATE-SESSION SESSION=c0
+#?XSERVER-0 ACCEPT-CONNECT
+#?SESSION-X-0 CONNECT-XSERVER
+
+# Add seatAAAFoo
+#?*ADD-SEAT ID=seatAAAFoo
+
+# seatAAAFoo starts
+#?XSERVER-1 START SEAT=seatAAAFoo SHAREVTS=TRUE
+#?*XSERVER-1 INDICATE-READY
+#?XSERVER-1 INDICATE-READY
+#?XSERVER-1 ACCEPT-CONNECT
+
+# Greeter starts
+#?GREETER-X-1 START XDG_SEAT=seatAAAFoo 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
+
+# Greeter is requested to timeout
+#?GREETER-X-1 AUTOLOGIN-USER USERNAME=have-password2 TIMEOUT=99
+
+# Trigger autologin
+#?*GREETER-X-1 AUTHENTICATE-AUTOLOGIN
+#?GREETER-X-1 AUTHENTICATION-COMPLETE USERNAME=have-password2 AUTHENTICATED=TRUE
+#?*GREETER-X-1 START-SESSION
+#?GREETER-X-1 TERMINATE SIGNAL=15
+
+# Session starts for configured user
+#?SESSION-X-1 START XDG_SEAT=seatAAAFoo XDG_GREETER_DATA_DIR=.*/have-password2 XDG_SESSION_TYPE=x11 XDG_SESSION_DESKTOP=default USER=have-password2
+#?LOGIN1 ACTIVATE-SESSION SESSION=c2
+#?XSERVER-1 ACCEPT-CONNECT
+#?SESSION-X-1 CONNECT-XSERVER
+
+# Add seatBBBFoo
+#?*ADD-SEAT ID=seatBBBFoo
+
+# seatBBBFoo starts
+#?XSERVER-2 START SEAT=seatBBBFoo SHAREVTS=TRUE
+#?*XSERVER-2 INDICATE-READY
+#?XSERVER-2 INDICATE-READY
+#?XSERVER-2 ACCEPT-CONNECT
+
+# Greeter starts
+#?GREETER-X-2 START XDG_SEAT=seatBBBFoo XDG_SESSION_CLASS=greeter
+#?LOGIN1 ACTIVATE-SESSION SESSION=c3
+#?XSERVER-2 ACCEPT-CONNECT
+#?GREETER-X-2 CONNECT-XSERVER
+#?GREETER-X-2 CONNECT-TO-DAEMON
+#?GREETER-X-2 CONNECTED-TO-DAEMON
+
+# Greeter is requested to timeout
+#?GREETER-X-2 AUTOLOGIN-USER USERNAME=have-password3 TIMEOUT=99
+
+# Trigger autologin
+#?*GREETER-X-2 AUTHENTICATE-AUTOLOGIN
+#?GREETER-X-2 AUTHENTICATION-COMPLETE USERNAME=have-password3 AUTHENTICATED=TRUE
+#?*GREETER-X-2 START-SESSION
+#?GREETER-X-2 TERMINATE SIGNAL=15
+
+# Session starts for configured user
+#?SESSION-X-2 START XDG_SEAT=seatBBBFoo XDG_GREETER_DATA_DIR=.*/have-password3 XDG_SESSION_TYPE=x11 XDG_SESSION_DESKTOP=default USER=have-password3
+#?LOGIN1 ACTIVATE-SESSION SESSION=c4
+#?XSERVER-2 ACCEPT-CONNECT
+#?SESSION-X-2 CONNECT-XSERVER
+
+# Cleanup
+#?*STOP-DAEMON
+#?XSERVER-0 TERMINATE SIGNAL=15
+#?SESSION-X-0 TERMINATE SIGNAL=15
+#?XSERVER-1 TERMINATE SIGNAL=15
+#?SESSION-X-1 TERMINATE SIGNAL=15
+#?XSERVER-2 TERMINATE SIGNAL=15
+#?SESSION-X-2 TERMINATE SIGNAL=15
+#?RUNNER DAEMON-EXIT STATUS=0

=== added file 'tests/test-multi-seat-globbing-config-sections'
--- tests/test-multi-seat-globbing-config-sections	1970-01-01 00:00:00 +0000
+++ tests/test-multi-seat-globbing-config-sections	2014-09-10 13:35:17 +0000
@@ -0,0 +1,2 @@
+#!/bin/sh
+./src/dbus-env ./src/test-runner multi-seat-globbing-config-sections test-gobject-greeter