GNOME Bugzilla – Bug 594818
The greeter should have separate GConf settings per seat
Last modified: 2010-07-02 19:08:51 UTC
Created attachment 142950 [details] [review] patch fixing problem Currently all GDM login GUI's share the same GConf directory (/var/lib/gdm/.gconf) which causes the following problems: 1) If a user starts or exits an a11y program, it will be launched or shut down on all displays that are running the login GUI at the same time. 2) The recent_layouts and recent_languages keys are a mish-mash of all seat values, which is not so useful. This sort of problem could happen, for example, if one user is logged into the console while another user is logged in via XDMCP. Once GDM supports multi-seat then this sort of problem would be even more serious since it would be more likely that multiple users would be logging in at the same time. I am attaching a patch that fixes this problem. This sets the environment variable GDM_SEAT_ID before launching D-Bus and the greeter session. It also creates a directory in the gdm user's $HOME directory with the same name as the GDM_SEAT_ID environment variable. The directory is owned by the gdm user and group with 750 permissions. The .gconf.path file now specifies the following line that makes sure that a separate .gconf directory is used for each Seat. xml:readwrite:$(HOME)/$(ENV_GDM_SEAT_ID)/.gconf This overrides the $(HOME)/.gconf value in the /etc/gconf/2/path file since this one is defined first.
Comment on attachment 142950 [details] [review] patch fixing problem I'm a little worried about the + strlen ("/org/freedesktop/ConsoleKit/") bit. Especially, since there is talk on ConsoleKit-list about making seats get their own namespace. A more futureproof solution might be to do short_seat_id = strrchr (full_seat_id, '/'); Otherwise, makes sense to me.
That + strlen ("/org/freedesktop/ConsoleKit/") logic was copied from gui/simple-greeter/gdm-user-manager.c from the function reload_ck_history(), which does the same thing. I just assumed that was a reasonable way to get the SeatID, since that's what the code was already using. It wouldn't be hard to fix the code in both places to use strrchr, though. At any rate, this patch doesn't seem to actually work. I noticed that although this does seem to correctly make each GDM login GUI use a separate GConf directory to save its settings (~gdm/Seat#/.gconf), you still have the problem that when you launch an AT tool in one login GUI it gets launched in all running GUI's. So, I'm wondering if GConf lacks the feature of sending notifications properly to autostart clients when different clients are using different GConf paths?
Created attachment 149066 [details] [review] updated patch Here is an updated patch that does work. Testing this patch, I am able to launch GDM on multiple screens and when I launch an AT program, it only starts or stops on the display where the user clicked in the a11y dialog. Note that this patch changes the name of /var/lib/gdm/.gconf.path to /var/lib/gdm/gconf.path. This is because it is now the top-level path file and not something that should be included by the /etc/gconf/2/path file. You were right that using GCONF_DEFAULT_SOURCE_PATH does make a difference. My previous patch did not use this environment variable. But once I started using GCONF_DEFAULT_SOURCE_PATH it started working. An odd behavior of GCONF_DEFAULT_SOURCE_PATH is that if the following line xml:readwrite:$(HOME)/$(ENV_GDM_SEAT_ID)/.gconf exists in the specific file specified by GCONF_DEFAULT_SOURCE_PATH, then it works. However, if the above line is in a file that is included, then it does not work. I am not completely 100% happy with this patch for two reasons: 1) In daemon/gdm-welcome-session.c, I am hardcoding GCONF_DEFAULT_SOURCE_PATH to "/var/lib/gdm/gconf.path". Should I look up the GDM_KEY_USER value, then look up the user's $HOME directory and append gconf.path instead? That might be better. Thoughts? 2) The fact that for $(ENV_GDM_SEAT_ID) does not work if it is in an included file makes me have to copy stuff from /etc/gconf/2/path into /var/lib/gdm/gconf.path. This may not be acceptable to go upstream since there is no reason to assume that distros install gconf files to the same places. So, perhaps the best way to move forward is to file a bug against GConf to see if issue #2 can be fixed. If so, then it would be easier to rework this patch so there isn't so much cut-and-pasting. What do you think? Or do people have any other suggestions? For now, distros can consider using this patch to fix this problem. While it is perhaps not ready to go upstream, a distro can ensure that the /var/lib/gdm/gconf.path file has the right contents if they use this patch. And it also fixes a particularly nasty usability issue.
Note that I filed bug #604206 about the GConf issue I ran into. It would be possible to make the changes to the /var/lib/gdm/gconf.path file less intrusive if we could make use of just setting $(ENV_GDM_SEAT_ID) in the ~gdm/.gconf.path file. Perhaps we wouldn't need to set $GCONF_DEFAULT_SOURCE_PATH at all if the right readwrite directory could just be specified there. If so, then we could fix this bug with the much less intrusive patch suggested in comment #1, I'd think.
Created attachment 159468 [details] [review] Updated patch After spending several hours today looking into this, I discovered how to make this work better, and am attaching an updated patch. I discovered that my previous assumption that there was an issue with using environment variables with the GConf "include" was wrong. So I already went ahead and closed the GConf bug I filed. After further research I discovered the problem I was seeing is that the GCONF_DEFAULT_SOURCE_PATH is ignored if both gconfd and the client are loading the same files with the same values. So if you use an environment variable in the path file and you have the environment variable set to the same value when starting both gconfd and the client, it gets ignored. In my previous patch when I moved the client's path file (and GCONF_DEFAULT_SOURCE_PATH setting) to a separate file (/var/lib/gdm/gconf.path), it worked and I incorrectly assumed this was due to an issue with how "include" was working. So, the trick is you need to use the environment variable in the gdm user's ".gconf.path" file, start gconfd without the environment variable set, and start the clients with the environment variable set. For the clients, it is necessary to set GCONF_DEFAULT_SOURCE_PATH to "/etc/gconf/2/path" before launching them. If I use the same patch without setting this, turning on or off an a11y program will cause the program to start or stop on all displays. When I do set this, it only starts or stops on the display where the request was made. So this updated patch adds the following line in the /var/lib/gdm/.gconf.path file: xml:readwrite:$(HOME)/$(ENV_GDM_SEAT_ID)/.gconf To ensure that the gconfd daemon is started without GDM_SEAT_ID set, I modified the code a bit so that gconfd is started by the slave right after starting D-Bus and without the GDM_SEAT_ID or GCONF_DEFAULT_SOURCE_PATH environment variables set. Then the code starts gnome-session with the two environment variables set. And this works well. This is much better than the previous patch since it allows all processes to just use the same default GConf path file, and avoids duplicating the /etc/gconf/2/path file in a separate file used only by the clients, which I was doing in my previous patch. This also avoids needing to set the environment variable to the "gdm" user's $HOME directory. So this addresses my concerns with the previous version of the patch. Note that the patch modifies configure.ac to check where gconfd-2 is located so it uses the right path.
I verified this patch works nicely with Local/SunRay/XDMCP sessions run at the same time.
Ray, I did the research you suggested about GConf, and you were right. The suggested patch to GConf to disable the processing of "def" also fixes the problem. I filed bug #617017 about the GConf issue and proposed two patches, either one can be used by GDM to fix this bug. I'm marking that bug as a bug this one depends on. You can refer to bug #617017 for more information about what I discovered. How should we move forward to get one or both of these fixes into GConf so we can finally fix this?
Oh, also, a comment about patch #5. That patch makes GDM launch gconfd right after launching the session D-Bus. I discovered that this is not necessary. Doing research I notice that the D-Bus session bus launches gconfd, so it is only necessary to make sure that the GDM_SEAT_ID and GCONF_DEFAULT_SOURCE_PATH get set in the environment before starting gnome-session and not before starting the D-Bus session bus. So, the code in the patch to launch gconfd and to keep track of its pid can be removed from that patch and it still works fine. However, I'll wait to provide and updated patch until we decide how to fix bug #617017. In that bug I propose a new interface GCONF_DISABLE_DEFAULT, and if that is accepted, then that makes the a little code easier and cleaner since we can just set that all the time (when launching both D-Bus and gnome-session) and don't have to set GCONF_DEFAULT_SOURCE_PATH at all.
Created attachment 159843 [details] [review] updated patch Here is a more simple patch that also works. In the last patch it started up gconfd after starting D-Bus but this isn't needed. It is just necessary to ensure that GDM_SEAT_ID and GCONF_DEFAULT_DEFAULT_SOURCE_PATH are not set when starting D-Bus. So, the only change needed for this patch is to fix GConf to expose the default path directory in its pkg-config file and to get that in configure and use it instead of hardcoding this. Also, Ray suggested that another way to fix this problem would be to fix GConf to save separate saved_state files per-session. He provided me with a patch to implement this. I tested this patch and this also works. With using this patch it is no longer necessary to set GCONF_DEFAULT_SOURCE_PATH at all, and it is okay to set GDM_SEAT_ID when starting both D-Bus and gnome-session. So, if the fix to make GConf save per-session saved_state files goes upstream, these additional changes can be made to the patch to make it more simple. In other words, with per-session saved_state files, it is only necessary to set GDM_SEAT_ID and reference it in gdm's .gconf.path file. At any rate, I recommend that we get the change to GConf upstream so it exposes the location of the default path file, rework this patch a bit to use that value, and put this code in 2.31. Then, we can rework it again later if the per-session saved_state fix goes into GConf.
Created attachment 160874 [details] [review] updated patch This patch makes use of the new GConf "gconf_defaultpath" setting in the gconf-2.0.pc file rather than hardcoding the value to "/etc/gconf/2/path" Also this sets the GCONF_REQUIRED_VERSION to 2.31.3 (since 2.31.2 is the latest released). Note that if bug #617017 gets fixed so the gconfd-2 process for each session has its own saved-state, then the following changes can be made to clean this code up a bit: - The start_session argument to get_welcome_environment() can be removed. - It will be okay to set GDM_SEAT_ID all the time. It will no longer be necessary to only set it when start-session argument is TRUE. - It will be no longer necessary to set GCONF_DEFAULT_SOURCE_PATH in get_welcome_environment().
I committed this patch upstream.
Note that Ray Strode reviewed this patch and approved it.
Created attachment 161489 [details] [review] updated patch Apologies. My previous patch broke the build. I didn't realize that this patch depends on some code from the MultiSeat branch. This updated patch forward ports the code to pass the seat_id into the greeter_session from the MultiSeat branch so that it is available for the welcome session to use.
Comment on attachment 161489 [details] [review] updated patch looks good