GNOME Bugzilla – Bug 751158
Support /etc/gdm/env.d
Last modified: 2015-06-22 17:30:40 UTC
For xdg-app dbus integration to work I need to set XDG_DATA_DIRS before dbus is launched in the session, so that it inherits my values for it. One way is to support a .d directory where xdg-app can drop a file.
Created attachment 305578 [details] [review] gdm-common: Add gdm_shell_expand() and tests This allows shell-like expansion of strings. It will be later used to allow configuring the environment via config files.
Created attachment 305579 [details] [review] load /etc/gdm/env.d files and set in session environment
Review of attachment 305578 [details] [review]: interesting that added something to tests/. Pretty sure that hasn't been touched since it was added ! ::: common/gdm-common.c @@ +715,3 @@ +is_valid_variable_char (gchar c, gboolean first) +{ + return (first && g_ascii_isdigit (c)) || this should read !first right? The idea is it can be a digit as long as its not the first char? @@ +727,3 @@ +gdm_shell_expand (const char *str, + GdmExpandVarFunc expand_var_func, + gpointer user_data) This is a little scary looking. I think you probably could have used GRegex, but meh, whatever, doesn't matter, if this works, then fine. It looks like it wasn't really written with utf8 in mind, but on the other hand, it looks like it will still work with utf8 anyway, so again, probably fine. @@ +743,3 @@ + p++; + c = *p; + if (c != 0) { use '\0' instead of 0 @@ +764,3 @@ + break; + } else if (c == '$') { + brackets = FALSE; brackets declaration could be moved here @@ +771,3 @@ + } + start = p; + while (*p != 0 && '\0' @@ +780,3 @@ + g_string_append_c (s, '{'); + g_string_append_len (s, start, p - start); + } else { expanded and var declarations can be moved here ::: tests/s-common.c @@ +63,3 @@ + fail_unless (expands_to ("foo #bar gazonk", "foo ")); + fail_unless (expands_to ("foo #bar gazonk", "foo ")); + fail_unless (expands_to ("$FOO", "BAR")); Should add $9FOO $FOO9 and ${FOO}9 also $_FOO and $FOO_FOO
Review of attachment 305579 [details] [review]: ::: daemon/gdm-session-worker.c @@ +1366,3 @@ +is_valid_variable_char (gchar c, gboolean first) +{ + return (first && g_ascii_isdigit (c)) || again, should be !first i think. Maybe this function should just get exported from gdm-common? @@ +1389,3 @@ + p = line; + while (g_ascii_isspace (*p)) + p++; weird indentation through out this function @@ +1390,3 @@ + while (g_ascii_isspace (*p)) + p++; + if (*p == '#' || *p == 0) '\0' @@ +1400,3 @@ + if (var == var_end || *p != '=') + { + g_warning ("Invalid env.d line %s\n", line); should have quote marks or brackets or something around the line string @@ +1435,3 @@ + int i; + + dir = g_file_new_for_path (GDMCONFDIR "/env.d"); should probably check DATADIR "/gdm/env.d" too so xdg-app doesn't have to put a non-conf file in /etc @@ +1451,3 @@ + if (g_file_info_get_file_type (info) == G_FILE_TYPE_REGULAR && + !g_file_info_get_is_hidden (info) && + !g_file_info_get_is_backup (info)) maybe we should require particular suffix so .rpmsave's don't screw us? @@ +1452,3 @@ + !g_file_info_get_is_hidden (info) && + !g_file_info_get_is_backup (info)) + g_ptr_array_add (names, g_strdup (g_file_info_get_name (info))); If you use GSequence instead of GPtrArray then you can do g_sequence_insert_sorted (names, g_strdup (...), (GCompareDataFunc) g_strcmp0, NULL); . This has two advantages: 1) it avoids having to have the extra compare_str function 2) it means you don't have to sort it explicitly afterward
Review of attachment 305578 [details] [review]: ::: common/gdm-common.c @@ +715,3 @@ +is_valid_variable_char (gchar c, gboolean first) +{ + return (first && g_ascii_isdigit (c)) || Ugh, yeah.. @@ +727,3 @@ +gdm_shell_expand (const char *str, + GdmExpandVarFunc expand_var_func, + gpointer user_data) It will work with utf8, by virtue of only looking at ascii characters and passing everything on as-is. Of course, it won't validate utf8 or suchlike, so it can garbage in garbage out.
Review of attachment 305579 [details] [review]: ::: daemon/gdm-session-worker.c @@ +1435,3 @@ + int i; + + dir = g_file_new_for_path (GDMCONFDIR "/env.d"); Yeah, that makes sense. I'll parse datadir first so that sysadmin always wins. @@ +1451,3 @@ + if (g_file_info_get_file_type (info) == G_FILE_TYPE_REGULAR && + !g_file_info_get_is_hidden (info) && + !g_file_info_get_is_backup (info)) Yeah, lets go with ".env" @@ +1452,3 @@ + !g_file_info_get_is_hidden (info) && + !g_file_info_get_is_backup (info)) + g_ptr_array_add (names, g_strdup (g_file_info_get_name (info))); Creating a balanced tree to sort a few lines of text seems like a massive overkill to me. I'd rather have an explictit sort call. The extra compare_str is unfortunate, but, meh. @@ +1879,3 @@ #endif + gdm_session_worker_load_env_d (worker); I had to add an "if !is_program_session" here, because the greeter session itself relies on overriding XDG_APP_DIRS, and overall i think its a bad idea if the env.d things pollute gdm itself.
Created attachment 305809 [details] [review] gdm-common: Add gdm_shell_expand() and tests This allows shell-like expansion of strings. It will be later used to allow configuring the environment via config files.
Created attachment 305810 [details] [review] load env.d/*.env files and set in session environment This loads key-value files from /usr/share/gdm/env.d/*.env and /etc/gdm/env.d/*.env (in alphabetical filename order) and sets in the session environment.
Review of attachment 305810 [details] [review]: feel free to push. ::: daemon/gdm-session-worker.c @@ +1452,3 @@ + } + + g_ptr_array_sort (names, compare_str); I still think using g_sequence is better, but this is okay, too.