GNOME Bugzilla – Bug 693204
split up g_get_{hostname,username,realname,home_dir} etc.
Last modified: 2013-02-20 11:10:20 UTC
Now that we have $HOME being respected, and particularly since this was done in the name of testing, we should expect to see code like this in tests: temp = g_dir_make_tmp ("dconf-testcase.XXXXXX", &error); g_assert_no_error (error); g_assert (temp != NULL); home = g_build_filename (temp, "home", NULL); run = g_build_filename (temp, "run", NULL); s = g_mkdir (home, 0777); g_assert (s == 0); s = g_mkdir (run, 0777); g_assert (s == 0); g_setenv ("HOME", home, TRUE); g_setenv ("XDG_RUNTIME_DIR", run, TRUE); g_assert_cmpstr (g_get_home_dir (), ==, home); g_assert_cmpstr (g_get_user_runtime_dir (), ==, run); But with current glib, this will fail. The reason for that is that g_dir_make_tmp() internally calls g_get_tmp_dir(). All of these type of functions currently share a bunch of global variables and work by acquiring a lock to check if these variables have been setup (dropping the lock and returning if they have) and calling one global setup function to set them up if needed. Would be nice to reduce use of that global lock as well as removing a lock acquire/drop from the 'fast case'. Another negative side effect of the above is that /etc/password is opened and read in for the case of just wanting to know the tmpdir or homedir (which are probably the ones that apps want to know most often). If $HOME is set then we really only need /etc/passwd for the realname (which most apps probably never ask for). Restructuring the code to be more separated would prevent this needless work. I also want to take this chance to clean up the old tricks we used to do for _utf8 vs. system-codepage support on win32. Continuing to maintaining this ancient ABI is pointless considering that we've already had ABI breaks on windows already between the point where those binaries were produced and today. Anyway... patchset coming soon.
Created attachment 235216 [details] [review] win32: Drop old codepage ABI from gutils.c This is a source-compatible change and only breaks ABI with respect to truly ancient binaries (and those binaries are already broken for other reasons). Back in the day, functions like g_get_user_name() used to return strings in the system codepage instead of utf8 (as they do today). It was decided at some point to change these functions to return utf8, breaking source compatibility but keeping ABI compatibility. This was done by exporting new symbols with names like g_get_user_name_utf8() and using a #define of the old name over to the new name (so that newly compiled code would link against the _utf8 version, but old binaries would continue to use the non-utf8 variant). Meanwhile, glib has undergone several ABI breaks on Windows since, so those old binaries don't work anymore. Start to clean up this mess by removing the #define renaming. New binaries calling g_get_user_name() will now link against g_get_user_name() and it will return utf8. We must keep the functions like g_get_user_name_utf8() for binary compatibility with recently built programs (ie: ones built with the renaming). Nobody should have ever been calling these directly and of course they can return utf8, so just add them as internal wrappers in the .c file and declare them _GLIB_EXTERN there. One day, if we feel like breaking Windows ABI again, we can finish the cleanup by dropping the wrappers. There is some talk of introducing something like 'ABI compatible for two years' and this change would be compatible with such a regime.
Created attachment 235217 [details] [review] gutils: split out g_get_host_name() Remove the code for getting the hostname from g_get_any_init_do() and outside of the g_utils_global lock.
Created attachment 235218 [details] [review] gutils: split out g_get_tmp_dir() Remove the code for getting the tmpdir from g_get_any_init_do() and outside of the g_utils_global lock.
Created attachment 235219 [details] [review] gutils: replace direct references to g_home_dir Some code was directly calling g_get_any_init() and then expecting to be able to use the static 'g_home_dir' variable directly. Change these over to g_get_home_dir() instead.
Created attachment 235220 [details] [review] gutils: stop g_get_home_dir() from reading passwd In the case that the "HOME" environment variable is set (as it is under normal circumstances), we don't really need to be opening /etc/passwd. For historical reasons (ie: how we used to ignore $HOME) and due to the grouping of many unrelated things together (reading username, hostname, home directory, tmpdir, etc.) into one function we were still opening /etc/passwd in g_get_home_dir(), even if $HOME was set. Since earlier commits removed code from it, all that remains in g_get_any_init_do() is the logic for dealing with $HOME and reading the password database. We now split the logic to deal with $HOME into g_get_home_dir(). With only the password database functionality remaining, g_get_any_init_do() is renamed to g_get_user_database_entry() and modified not to set global variables but rather return a struct. If g_get_home_dir() cannot find $HOME, it falls back to calling g_get_user_database_entry() and using the home directory from there. Use of the 'g_utils_global' lock is further reduced by using g_once_init_enter() to protect the critical sections in each of g_get_user_database_entry() and g_get_home_dir(). Finally, the g_get_user_name() and g_get_real_name() functions are modified to use the new regime.
Review of attachment 235216 [details] [review]: ::: glib/gutils.c @@ +2366,3 @@ +const gchar *g_get_real_name_utf8 (void) { return g_get_real_name (); } +const gchar *g_get_home_dir_utf8 (void) { return g_get_home_dir (); } +const gchar *g_get_tmp_dir_utf8 (void) { return g_get_tmp_dir (); } Can't you just make these aliases, instead of adding actual code here ? If msvc has aliases, that is...
Review of attachment 235217 [details] [review]: looks good to me
Review of attachment 235218 [details] [review]: Looks fine to me.
Review of attachment 235219 [details] [review]: ::: glib/gutils.c @@ +1210,3 @@ { + if (g_get_home_dir ()) + data_dir = g_build_filename (g_get_home_dir (), ".local", "share", NULL); Looks a little sloppy to me to call g_get_home_dir() twice in a row; You could be sneaky and do if (!data_dir) data_dir = g_build_filename (g_get_home_dir (), ...) if (!data_dir) data_dir = g_build_filename (g_get_tmp_dir (), ...) relying on the fact that g_build_filename (NULL,...) returns NULL
Review of attachment 235220 [details] [review]: Looks fine to me, otherwise ::: glib/gutils.c @@ +817,3 @@ g_get_user_name (void) { + return g_get_user_database_entry ()->user_name; ()-> looks a bit ugly
(In reply to comment #9) > relying on the fact that g_build_filename (NULL,...) returns NULL It doesn't. I'll just use a temp variable to avoid calling twice.
(In reply to comment #6) > If msvc has aliases, that is... as far as I can tell (after researching it), we can't do this from C. I think this is fine as a temporary hack -- we will probably want to remove them again in the future (as I hint at in the commit log).
(In reply to comment #11) > ()-> looks a bit ugly OK. Fixed this as well by using a temp variable.
Attachment 235216 [details] pushed as 8c42a66 - win32: Drop old codepage ABI from gutils.c Attachment 235217 [details] pushed as 3c9691f - gutils: split out g_get_host_name() Attachment 235218 [details] pushed as 9879c7f - gutils: split out g_get_tmp_dir() Attachment 235219 [details] pushed as 167c73f - gutils: replace direct references to g_home_dir Attachment 235220 [details] pushed as cfafad5 - gutils: stop g_get_home_dir() from reading passwd