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

[Laércio de Sousa]

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

Unmerged revisions

=== modified file 'src/lightdm.c'
--- src/lightdm.c	2014-08-28 00:11:30 +0000
+++ src/lightdm.c	2014-08-28 14:40:56 +0000
@@ -145,6 +145,27 @@
     g_free (path);
 }
 
+static GList*
+get_config_sections (const gchar *seat_name)
+{
+    gchar **groups, **i;
+    GList *config_sections = NULL;
+
+    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));
+        }
+    }
+    if (groups)
+        g_strfreev (groups);
+    return config_sections;
+}
+
 static void
 set_seat_properties (Seat *seat, const gchar *config_section)
 {
@@ -221,11 +242,13 @@
 
     if (next_seat)
     {
-        gchar *config_section;
+        GList *config_sections, *link;
 
-        config_section = g_strdup_printf ("Seat:%s", seat_get_name (seat));
-        set_seat_properties (next_seat, config_section);
-        g_free (config_section);
+        config_sections = get_config_sections (seat_get_name (seat));
+        for (link = config_sections; link; link = link->next)
+            set_seat_properties (next_seat, (gchar*) link->data);
+        if (config_sections)
+            g_list_free_full (config_sections, g_free);
 
         // We set this manually on default seat.  Let's port it over if needed.
         if (seat_get_boolean_property (seat, "exit-on-failure"))
@@ -938,29 +961,18 @@
 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++)
-    {
-        if (g_str_has_prefix (*i, "Seat:") &&
-            g_str_has_suffix (*i, seat_name))
-        {
-            config_section = g_strdup (*i);
-            break;
-        }
-    }
-    g_strfreev (groups);
-
-    if (config_section)
-    {
+    config_sections = get_config_sections (seat_name);
+    for (link = config_sections; link; link = link->next)
+    {
+        gchar *config_section = link->data;
         g_debug ("Loading properties from config section %s", config_section);
         types = config_get_string_list (config_get_instance (), config_section, "type");
     }
@@ -981,8 +993,8 @@
             seat_set_property (seat, "allow-user-switching", "false");
         }
 
-        if (config_section)
-            set_seat_properties (seat, config_section);
+        for (link = config_sections; link; link = link->next)
+            set_seat_properties (seat, (gchar*) link->data);
 
         if (is_seat0)
             seat_set_property (seat, "exit-on-failure", "true");
@@ -997,7 +1009,8 @@
             g_debug ("Failed to start seat: %s", seat_name);
     }
 
-    g_free (config_section);
+    if (config_sections)
+        g_list_free_full (config_sections, g_free);
     g_object_unref (seat);
   
     return started;
 
=== modified file 'tests/Makefile.am'
--- tests/Makefile.am	2014-08-28 00:11:30 +0000
+++ tests/Makefile.am	2014-08-28 14:40:56 +0000
@@ -183,6 +183,7 @@
 	test-multi-seat \
 	test-multi-seat-non-graphical \
 	test-multi-seat-change-graphical \
+	test-multi-seat-with-globbing-config-sections \
 	test-mir-autologin \
 	test-mir-greeter \
 	test-mir-session \
@@ -448,6 +449,7 @@
 	scripts/multi-seat.conf \
 	scripts/multi-seat-non-graphical.conf \
 	scripts/multi-seat-change-graphical.conf \
+	scripts/multi-seat-with-globbing-config-sections.conf \
 	scripts/no-accounts-service.conf \
 	scripts/no-config.conf \
 	scripts/no-console-kit.conf \
 
=== added file 'tests/scripts/multi-seat-with-globbing-config-sections.conf'
--- tests/scripts/multi-seat-with-globbing-config-sections.conf	1970-01-01 00:00:00 +0000
+++ tests/scripts/multi-seat-with-globbing-config-sections.conf	2014-08-28 14:40:56 +0000
@@ -0,0 +1,106 @@
+#
+# Check can set globbing config sections matching different seats
+#
+
+[Seat:seat*Foo]
+autologin-guest=true
+user-session=default
+
+[Seat:seat-AAA-Foo]
+autologin-user-timeout=5
+
+[Seat:seat-BBB*]
+autologin-user-timeout=10
+
+#?*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
+#?GREETER-X-0 START XDG_SEAT=seat0 XDG_VTNR=7 XDG_SESSION_CLASS=greeter
+#?LOGIN1 ACTIVATE-SESSION SESSION=c0
+#?XSERVER-0 ACCEPT-CONNECT
+#?GREETER-X-0 CONNECT-XSERVER
+#?GREETER-X-0 CONNECT-TO-DAEMON
+#?GREETER-X-0 CONNECTED-TO-DAEMON
+
+# Add seat-AAA-Foo
+#?*ADD-SEAT ID=seat-AAA-Foo
+
+# seat-AAA-Foo starts
+#?XSERVER-1 START SEAT=seat-AAA-Foo SHAREVTS=TRUE
+#?*XSERVER-1 INDICATE-READY
+#?XSERVER-1 INDICATE-READY
+#?XSERVER-1 ACCEPT-CONNECT
+#?GREETER-X-1 START XDG_SEAT=seat-AAA-Foo 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-GUEST TIMEOUT=5
+
+# Trigger autologin
+#?*GREETER-X-1 AUTHENTICATE-AUTOLOGIN
+#?GREETER-X-1 AUTHENTICATION-COMPLETE AUTHENTICATED=TRUE
+#?*GREETER-X-1 START-SESSION
+#?GREETER-X-1 TERMINATE SIGNAL=15
+
+# Guest account created
+#?GUEST-ACCOUNT ADD USERNAME=guest-.*
+
+# Guest session starts
+#?SESSION-X-1 START XDG_SEAT=seat-AAA-Foo XDG_GREETER_DATA_DIR=.*/guest-.* XDG_SESSION_TYPE=x11 XDG_SESSION_DESKTOP=default USER=guest-.*
+#?LOGIN1 ACTIVATE-SESSION SESSION=c2
+#?XSERVER-1 ACCEPT-CONNECT
+#?SESSION-X-1 CONNECT-XSERVER
+
+# Add seat-BBB-Foo
+#?*ADD-SEAT ID=seat-BBB-Foo
+
+# seat-BBB-Foo starts
+#?XSERVER-2 START SEAT=seat-BBB-Foo SHAREVTS=TRUE
+#?*XSERVER-2 INDICATE-READY
+#?XSERVER-2 INDICATE-READY
+#?XSERVER-2 ACCEPT-CONNECT
+#?GREETER-X-2 START XDG_SEAT=seat-BBB-Foo 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-1 AUTOLOGIN-GUEST TIMEOUT=10
+
+# Trigger autologin
+#?*GREETER-X-2 AUTHENTICATE-AUTOLOGIN
+#?GREETER-X-2 AUTHENTICATION-COMPLETE AUTHENTICATED=TRUE
+#?*GREETER-X-2 START-SESSION
+#?GREETER-X-2 TERMINATE SIGNAL=15
+
+# Guest account created
+#?GUEST-ACCOUNT ADD USERNAME=guest-.*
+
+# Guest session starts
+#?SESSION-X-2 START XDG_SEAT=seat-BBB-Foo XDG_GREETER_DATA_DIR=.*/guest-.* XDG_SESSION_TYPE=x11 XDG_SESSION_DESKTOP=default USER=guest-.*
+#?LOGIN1 ACTIVATE-SESSION SESSION=c4
+#?XSERVER-2 ACCEPT-CONNECT
+#?SESSION-X-2 CONNECT-XSERVER
+
+# Cleanup
+#?*STOP-DAEMON
+#?GREETER-X-0 TERMINATE SIGNAL=15
+#?XSERVER-0 TERMINATE SIGNAL=15
+#?XSESSION-X-1 TERMINATE SIGNAL=15
+#?XSERVER-1 TERMINATE SIGNAL=15
+#?GUEST-ACCOUNT REMOVE USERNAME=guest.*
+#?XSESSION-X-2 TERMINATE SIGNAL=15
+#?XSERVER-2 TERMINATE SIGNAL=15
+#?GUEST-ACCOUNT REMOVE USERNAME=guest.*
+#?RUNNER DAEMON-EXIT STATUS=0
 
=== added file 'tests/test-multi-seat-with-globbing-config-sections'
--- tests/test-multi-seat-with-globbing-config-sections	1970-01-01 00:00:00 +0000
+++ tests/test-multi-seat-with-globbing-config-sections	2014-08-28 14:40:56 +0000
@@ -0,0 +1,2 @@
+#!/bin/sh
+./src/dbus-env ./src/test-runner multi-seat-with-globbing-config-sections test-gobject-greeter