Re: [gdm-list] Help with GDM 2.20.2: gdm ignoring custom server config

[Brian Cameron <Brian Cameron Sun COM>]

Martin:

> I recently upgraded to GDM 2.20.2 via Debian/Lenny.  I now notice that
> my custom [server-foo] instances are being ignored without messages.
> I turned on debug to look for a problem.

GDM 2.20 has a bug that [server-foo] sections are not read by the daemon
unless they are actually used by a display in the [servers] section.
Refer to bug #462613.

  http://bugzilla.gnome.org/show_bug.cgi?id=462613

For example, if you have the following in your [servers] section:

[servers]
0=Standard
1=Foo

Then *only* [server-Standard} and [server-Foo} will be read by the
daemon and recognized.  If you have other "server-xyz" sections, they
will be ignored.

This was recently fixed in the 2.20 branch so now it should work
better and read in all the server-* sections even if they aren't
used in the [servers] section.

You don't mention the contents of the "[servers]" section of your
configuration file, so I'm not 100% sure this is your problem, but
I'm guessing this known issue is what you are seeing?

Could you try the attached patch and see if it fixes your issues?
This fix will go into the next release of GDM 2.20.

If this doesn't fix your problem, please provide additional details
and we can look into the problems further.

Thanks,

Brian



> E.g 
>
> > gdmflexiserver -c GET_SERVER_LIST
> OK Standard
>
> but my gdm.conf has:
>
> [server-Standard]
> name=Standard server
> command=/usr/bin/X -br -audit 0 -layout Single
> flexible=true
>
> [server-Dual]
> name=Dual Head
> command=/usr/bin/X -br -audit 0 -layout Dual
> flexible=true
>
> [server-Xinerama]
> name=Xinerama
> command=/usr/bin/X -br -audit 0 -layout Xinerama
> flexible=true
>
> If I do:
>
> > gdmflexiserver -c "GET_SERVER_DETAILS Standard COMMAND"
> OK /usr/bin/X -br -audit 0 -layout Single
>
> which shows my nonstandard command.  I tried changing order
> of the stanzas while grabbing at straws.
>
> Is this a Debian problem? This all worked fine on an earlier version.
> Does anyone know what I'm missing?  A new default feature?  I tried
> scanning the changelogs but didn't find anything relevant.
>
> Thanks in advance!!
>
> --Martin
>
>
> P.S. I'm not suscribed so please cc me . . . 
> _______________________________________________
> gdm-list mailing list
> gdm-list gnome org
> http://mail.gnome.org/mailman/listinfo/gdm-list

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 5598)
+++ ChangeLog	(working copy)
@@ -1,3 +1,20 @@
+2008-01-02 Brian Cameron <brian cameron sun com>
+
+	* common/gdm-config.h, common/gdm-config.c:  Add function for
+	  getting a list of server-foo sections from the configuration
+	  files.  Also fix bug that was causing the comparison of
+	  STRING_ARRAY keys to not work.  This was causing gdmsetup
+	  to behave badly when changing the value of the
+	  Halt/Shutdown/Reboot commands.  Fixes bug #502074.
+	* daemon/gdm-daemon-config.c: Now use the above functions to
+	  get the server-foo section names rather than just parsing the
+	  ones used in the [server] section.  This makes gdmsetup work
+	  better and fixes bug #462613.
+	* daemon/gdm.c, daemon/gdm-daemon-config.c: Set debug as soon as
+	  the configuration value is read, not after the configuration
+	  file parsing is done.  This is useful for debugging problems
+	  while parsing.
+
 2007-12-21 Brian Cameron <brian cameron sun com>
 
 	* daemon/gdm.c, daemon/slave.c: Fix some casting issues pointed out
@@ -7,7 +24,12 @@
 
 	* gui/gdmflexiserver.c: Revert to the old logic for handling 
 	  options.  This uses g_option_context* rather than gtk_init.
-	  This fixes bug #438939.
+	  Now we only call gtk_init when not handling the "--command"
+	  argument.  This fixes bug #438939.  The problem here is that
+	  you should be able to run gdmflexiserver with the --command
+	  option even if running setuid or setgid, but the gtk_init
+	  function doesn't allow this.  So we need to use the g_option
+	  functions instead.
 
 2007-12-10 Brian Cameron <brian cameron sun com>
 
Index: common/gdm-config.h
===================================================================
--- common/gdm-config.h	(revision 5598)
+++ common/gdm-config.h	(working copy)
@@ -126,6 +126,7 @@
 							  const gchar     *group_name,
 							  gsize           *length,
 							  GError         **error);
+GPtrArray *            gdm_config_get_server_groups      (GdmConfig       *config);
 
 gboolean               gdm_config_peek_value             (GdmConfig             *config,
 							  const char            *group,
Index: common/gdm-config.c
===================================================================
--- common/gdm-config.c	(revision 5598)
+++ common/gdm-config.c	(working copy)
@@ -286,7 +286,7 @@
 			int   res;
 
 			str_a = gdm_config_value_to_string (value_a);
-			str_b = gdm_config_value_to_string (value_a);
+			str_b = gdm_config_value_to_string (value_b);
 			res = safe_strcmp (str_a, str_b);
 			g_free (str_a);
 			g_free (str_b);
@@ -615,6 +615,71 @@
 	g_slice_free (GdmConfig, config);
 }
 
+static void
+add_server_group_once (GPtrArray *server_groups, char *group)
+{
+	int i;
+
+	for (i=0; i < server_groups->len; i++) {
+		if (strcmp (g_ptr_array_index (server_groups, i), group) == 0) {
+			g_debug ("server group %s already exists, skipping",
+				group);
+			return;
+		}
+	}
+	g_ptr_array_add (server_groups, g_strdup (group));
+}
+
+GPtrArray *
+gdm_config_get_server_groups (GdmConfig *config)
+{
+	GPtrArray       *server_groups;
+	GError          *error;
+	char           **groups;
+	gsize            len;
+	int              i;
+	
+	server_groups = g_ptr_array_new ();
+
+	if (config->mandatory_key_file != NULL) {
+		groups = g_key_file_get_groups (config->mandatory_key_file, &len);
+
+		for (i = 0; i < len; i++)
+		{
+			if (g_str_has_prefix (groups[i], "server-")) {
+				add_server_group_once (server_groups, groups[i]);
+			}
+		}
+		g_strfreev (groups);
+	}
+
+	if (config->default_key_file != NULL) {
+		groups = g_key_file_get_groups (config->default_key_file, &len);
+
+		for (i = 0; i < len; i++)
+		{
+			if (g_str_has_prefix (groups[i], "server-")) {
+				add_server_group_once (server_groups, groups[i]);
+			}
+		}
+		g_strfreev (groups);
+	}
+
+	if (config->custom_key_file != NULL) {
+		groups = g_key_file_get_groups (config->custom_key_file, &len);
+
+		for (i = 0; i < len; i++)
+		{
+			if (g_str_has_prefix (groups[i], "server-")) {
+				add_server_group_once (server_groups, groups[i]);
+			}
+		}
+		g_strfreev (groups);
+	}
+
+	return server_groups;
+}
+
 const GdmConfigEntry *
 gdm_config_lookup_entry (GdmConfig  *config,
 			 const char *group,
Index: daemon/gdm.c
===================================================================
--- daemon/gdm.c	(revision 5598)
+++ daemon/gdm.c	(working copy)
@@ -1575,7 +1575,6 @@
 	gdm_log_init ();
 	/* Parse configuration file */
 	gdm_daemon_config_parse (config_file, no_console);
-	gdm_log_set_debug (gdm_daemon_config_get_bool_for_id (GDM_ID_DEBUG));
 
 	/* XDM compliant error message */
 	if G_UNLIKELY (getuid () != 0) {
Index: daemon/gdm-daemon-config.c
===================================================================
--- daemon/gdm-daemon-config.c	(revision 5598)
+++ daemon/gdm-daemon-config.c	(working copy)
@@ -1067,12 +1067,11 @@
  */
 static void
 gdm_daemon_config_load_xserver (GdmConfig  *config,
-				const char *key,
+				const char *group,
 				const char *name)
 {
 	GdmXserver     *svr;
 	int             n;
-	char           *group;
 	gboolean        res;
 	GdmConfigValue *value;
 
@@ -1084,8 +1083,6 @@
 	svr = g_new0 (GdmXserver, 1);
 	svr->id = g_strdup (name);
 
-	group = g_strdup_printf ("server-%s", name);
-
 	/* string */
 	res = gdm_config_get_value (config, group, "name", &value);
 	if (res) {
@@ -1141,8 +1138,6 @@
 	}
 
 	xservers = g_slist_append (xservers, svr);
-
-	g_free (group);
 }
 
 static void
@@ -1187,78 +1182,45 @@
 static void
 load_xservers_group (GdmConfig *config)
 {
-	char     **keys;
-	gsize      len;
-	int        i;
+	GPtrArray  *server_groups;
+	char      **vname_array;
+	char       *xserver_value;
+	int         i, j;
 
-	keys = gdm_config_get_keys_for_group (config,
-		GDM_CONFIG_GROUP_SERVERS, &len, NULL);
+	server_groups = gdm_config_get_server_groups (config);
 
-	/* now construct entries for these groups */
-	for (i = 0; i < len; i++) {
-		GdmConfigEntry   entry;
-		GdmConfigValue  *value;
-		const char      *display_value;
-		const char      *name;
-		char           **value_list;
-		char            *new_group;
-		int              j;
-		gboolean         res;
+	for (i=0; i < server_groups->len; i++) {
+		xserver_value = g_ptr_array_index (server_groups, i);
+		gdm_debug ("Processing server group <%s>", xserver_value);
 
-	        entry.group         = GDM_CONFIG_GROUP_SERVERS;
-		entry.key           = keys[i];
-		entry.type          = GDM_CONFIG_VALUE_STRING;
-		entry.default_value = NULL;
-		entry.id            = GDM_CONFIG_INVALID_ID;
+		if (g_str_has_prefix (xserver_value, "server-")) {
+			char * xserver_group;
+			char * xserver_name;
 
-		gdm_config_add_entry (config, &entry);
-		gdm_config_process_entry (config, &entry, NULL);
+			xserver_group = g_strdup (xserver_value);
+			
+			for (j = 0; j < G_N_ELEMENTS (gdm_daemon_server_config_entries); j++) {
+				GdmConfigEntry *srv_entry;
+				if (gdm_daemon_server_config_entries[j].key == NULL) {
+					continue;
+				}
+				srv_entry = gdm_config_entry_copy (&gdm_daemon_server_config_entries[j]);
+				g_free (srv_entry->group);
+				srv_entry->group = xserver_group;
+				gdm_config_process_entry (config, srv_entry, NULL);
+				gdm_config_entry_free (srv_entry);
+			}
 
-		res = gdm_config_get_value (config, entry.group, entry.key,
-			&value);
-		if (! res) {
-			continue;
-		}
+			/* Strip "server-" prefix from name */
+			xserver_name = xserver_group + strlen ("server-");
 
-		display_value = gdm_config_value_get_string (value);
-		value_list = g_strsplit (display_value, " ", -1);
-		if (value_list == NULL || value_list[0] == '\0') {
-			gdm_config_value_free (value);
-			g_strfreev (value_list);
-			continue;
+			/* Now we can add this server */
+			if (xserver_name != NULL)
+				gdm_daemon_config_load_xserver (config, xserver_group, xserver_name);
 		}
-		
-		name = value_list[0];
-
-		/* Skip servers marked as inactive */
-		if (name == NULL || name[0] == '\0' ||
-		    g_ascii_strcasecmp (name, "inactive") == 0) {
-			gdm_config_value_free (value);
-			g_strfreev (value_list);
-			continue;
-		}
-
-		new_group = g_strdup_printf ("server-%s", name);
-		for (j = 0; j < G_N_ELEMENTS (gdm_daemon_server_config_entries); j++) {
-			GdmConfigEntry *srv_entry;
-			if (gdm_daemon_server_config_entries[j].key == NULL) {
-				continue;
-			}
-			srv_entry = gdm_config_entry_copy (&gdm_daemon_server_config_entries[j]);
-			g_free (srv_entry->group);
-			srv_entry->group = g_strdup (new_group);
-			gdm_config_process_entry (config, srv_entry, NULL);
-			gdm_config_entry_free (srv_entry);
-		}
-		g_free (new_group);
-
-		/* Now we can add this server */
-		gdm_daemon_config_load_xserver (config, entry.key, name);
-                gdm_config_value_free (value);
-		g_strfreev (value_list);
         }
 
-	g_strfreev (keys);
+	g_ptr_array_free (server_groups, TRUE);
 }
 
 static void
@@ -1471,16 +1433,36 @@
 	keys = gdm_config_get_keys_for_group (config,
 		GDM_CONFIG_GROUP_SERVERS, &len, NULL);
 
-        /* now construct entries for these groups */
-        for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++) {
+		GdmConfigEntry   entry;
+		GdmConfigValue  *value;
+		const char      *display_value;
+		const char      *name;
+		char           **value_list;
+		char            *new_group;
+		int              j;
+		gboolean         res;
+
+		entry.group         = GDM_CONFIG_GROUP_SERVERS;
+		entry.key           = keys[i];
+		entry.type          = GDM_CONFIG_VALUE_STRING;
+		entry.default_value = NULL;
+		entry.id            = GDM_CONFIG_INVALID_ID;
+
+		gdm_config_add_entry (config, &entry);
+		gdm_config_process_entry (config, &entry, NULL);
+	}
+
+	/* Now construct entries for these groups */
+	for (i = 0; i < len; i++) {
+		char           **value_list;
 		GString         *command     = NULL;
-		GdmDisplay     *disp;
-		GdmConfigValue *value;
+		GdmDisplay      *disp;
+		GdmConfigValue  *value;
 		const char      *name        = NULL;
 		const char      *device_name = NULL;
-		char           **value_list;
-		int             keynum;
-		gboolean        res;
+		int              keynum;
+		gboolean         res;
 
 		name   = keys[i];
 
@@ -1490,15 +1472,20 @@
 
 		keynum = atoi (name);
 
-                res = gdm_config_get_value (config, GDM_CONFIG_GROUP_SERVERS,
+		res = gdm_config_get_value (config, GDM_CONFIG_GROUP_SERVERS,
 			keys[i], &value);
 		if (! res) {
 			continue;
 		}
 
-                display_value = gdm_config_value_get_string (value);
-		value_list    = g_strsplit (display_value, " ", -1);
+		display_value = gdm_config_value_get_string (value);
 
+		/* Skip displays marked as inactive */
+		if (g_ascii_strcasecmp (display_value, "inactive") == 0)
+			continue;
+
+		value_list = g_strsplit (display_value, " ", -1);
+
 		if (value_list == NULL || value_list[0] == '\0') {
 			gdm_config_value_free (value);
 			g_strfreev (value_list);
@@ -1524,7 +1511,7 @@
 				g_string_append (command, " ");
 			}
 			j++;
-                }
+		}
 
 		gdm_debug ("Loading display for key '%d'", keynum);
 
@@ -1786,7 +1773,21 @@
 	return TRUE;
 }
 
+/* Cause debug to affect logging as soon as the config value is read */
 static gboolean
+validate_debug (GdmConfig          *config,
+		GdmConfigSourceType source,
+		GdmConfigValue     *value)
+{
+	gboolean debugval;
+
+	debugval = gdm_config_value_get_bool (value);
+	gdm_log_set_debug (debugval);
+
+	return TRUE;
+}
+
+static gboolean
 validate_at_least_int (GdmConfig          *config,
 		       GdmConfigSourceType source,
 		       GdmConfigValue     *value,
@@ -1814,6 +1815,9 @@
 	res = TRUE;
 
         switch (id) {
+        case GDM_ID_DEBUG:
+		res = validate_debug (config, source, value);
+		break;
         case GDM_ID_PATH:
 		res = validate_path (config, source, value);
 		break;