*** Bug 57364 has been marked as a duplicate of this bug. ***

*** Bug 135337 has been marked as a duplicate of this bug. ***

*** Bug 89421 has been marked as a duplicate of this bug. ***

*** Bug 51012 has been marked as a duplicate of this bug. ***

*** Bug 90052 has been marked as a duplicate of this bug. ***

Note that bug 135337 (closed as a duplicate of this bug) has a patch that restarts the Xserver based on the user's group.  This patch was ultimately rejected since using groups in this way isn't really a good idea.  There are only a limited number of groups on UNIX, and encouraging sysadmins to create a bunch of groups to support starting Xserver with different configurations isn't really a good usage of UNIX groups.

If someone wanted to tweak this patch to make it work with one of the suggestions above, that would be great.

Note that one of the pieces of functionality described in one of the duplicates is to allow running multiple Xservers, so that more than one GUI user can log in at the same time.

Yes, but isn't that already provided reasonably with gdmflexiserver and virtual terminals?

Sorry, I should have been more specific...yes, you can actually already run multiple graphical logins if you like, if you know where and how to edit some text configuration file.  What I would actually like to see is for this to become possible for people to do using only the user-friendly graphical interface.  It seems a common enough thing for residential users to want to do, as well as businesses that have a high employee-to-computer ratio, so it should be relatively easy to do.

Yes, but I believe this is already possible.  Most distros provide a "New Login" choice in the Start menu under "System Tools".  Recent xscreensaver/gnome-screensaver builds support launching gdmflexiserver from the screensaver screen.  Users can create launchers that run the gdmflexiserver command if they want a button on the screen to do this.  You can run the gdmflexiserver command with the --startnew option if you don't want to see the dialog showing you currently open sessions and you just want to start a new session.  What more should be done?

good to see this bug is still in the works :)

I am still trying to get used to the GDM code

why can't we 
1)parse the .desktop file
if extensions are found....
2) send
gdm_server_stop(GdmDisplay *)
gdm_slave_send_num(GDM_SOP_XPID, 0)
3) send (with modified X extensions)
gdm_server_start  (d, TRUE, FALSE, 20, 5)
gdm_slave_send_num(GDM_SOP_XPID, d->servpid)

Yes, it wouldn't be hard to implement this fix, really.  As I point out above in comment #6, there is a patch that could probably be easily modified to do this.  This issue has not been important enough, it seems, for anybody to provide a patch.

Obviously, it would be useful to be able to launch the Xserver with specific extensions.  I think specifying the custom command in the *.desktop file is probably the cleanest place to specify this.  The patch in comment #6 bases it on the user's group id, which isn't really scalable.

I think using desktop files makes more sense than other approach since the desktop file causes a new entry to be added to the session dialog that can explain what the user is picking.  Though we can also discuss other approaches.  For example, some have suggested adding a new "Xserver command" menu/dialog.  The problem with this apporach is that most user's would probably have a harder time understanding what the choices mean since Xserver command options can be used for such a wide variety of things.

Normally when extra options are required per-display, they are required for a reason, so having an "Accessibility" session, for example, would probably make more sense to the user.  

Ideas or suggestions about other approaches is, of course, welcome.

Hi Brian, 

As discussed almost a year ago, what about checking for .xinitrc/.xserverrc in the user's home directory?  if those files are found, obviously the user wants X initiated a certain wait for their session.  The gdm server would have to issue xinit rather than X however to get these scripts to be picked up.  

Are these files depreciated nowadays?  If they are, then we should stick to one of your methods.  If they are still used, why not let xinit, .xservercc, .xinitrc do the work instead of GDM?

I am finally taking a real look at bug 135337.  It looks like the patch has everything I would need except instead of figuring out which user I am (which appears to be done in gdm_slave_parse_a11y_args) I can look for those files and simply excute

gdm_server_stop(d);
gdm_slave_send_num (GDM_SOP_XPID, 0);
gdm_server_start (d, TRUE, FALSE, 20, 5);
gdm_slave_send_num (GDM_SOP_XPID, (long) d->servpid); 

When playing around with these a little bit I noticed X would shutdown and then would not come back correctly.  Am I wrong in using this format?  

I don't know if .xinitrc or .xserverrc is deprecated or not.  You would have to check with the Xorg people.  I'd be happy to accept a patch that made GDM support these files, if this seems a better way to support the behavior.

I'm not sure why the Xserver wouldn't start correctly - the code does look right.  You'll need to debug.  Any clues in the xorg log output?

GDM would launch xinit, but xinit launches .xinitrc and .xservercc from $HOME, I am afraid I cannot determine who would be running it.  In Slave.c where is $HOME pointing to?  root, gdm, or the user?  I have the code for checking for these files, and understand the original patch, but I am afraid this might not be the right solution.

I was looking into your .desktop solution, but I have some questions. How can implementing a .desktop solution restart the X server?  If I understand correctly, the .desktop file can only reporduce what might occur in typical xsession files where the X server has already loaded.  Even if my custom file stopped X and restarted it based on my extensions, GDM would not have control of these processes.  Were you thinking we could make gdm calls somehow to take care of that?  I noticed some comments in the default.desktop file where you can make a custom.desktop file, which I did, but I cannot think of where to go from there.

The GDM program processes the *.desktop files - look at gdm_slave_session_start ().  Note it process the file and gets the "Exec" value calling the is_session_ok function.

If we added a new key to the file to specify a different X command to run, then the code would need to be enhanced to load the key and, if present, restart the Xserver with the specified command.  

In other words, this would be an extension to the .desktop file specification that would work just for GDM.  

As I said, there might be other approaches to supporting this feature.  If you have better suggestions, that would be great.  Making GDM support processing .xservercc and restarting the Xserver based on that file might also make sense.

I believe the user's $HOME directory is set up in the session_child_run function.  You'll notice it sets the HOME environment variable and a host of other variables.  Probably after this it would be okay to access the user's home directory via the $HOME environment variable.  Since restarting the Xserver probably requires root, it would likely be best to do this before the set_euid_egid function is called to set the uid/gid to the user logging in.

Created attachment 70334 [details] [review]
Patch to allow additional X server arguments in session files

This patch adds the optional key X-Gdm-XserverArgs to session files.  This makes it possible to specify additional X server arguments.

The patch looks good, but I won't accept a patch that affects the format of the desktop file unless it is documented in the docs/C/gdm.xml file.  Please add information about the new X-Gdm_XserverArgs desktop extension to the documentaiton and resubmit the patch.  This is currently discussed in section 5.1, but it might be nicer if the desktop files had their own subsection in the Configuration chapter that explained what all the fields in the .desktop file does, in terms of the display manager.

Created attachment 70455 [details] [review]
Revised patch with documentation

Thanks so much, this patch looks ready.  However, it does add API and we are past the API freeze for 2.16.  Therefore I recommend that we wait until after the 2.16 GDM branch and apply this to the next release of GDM.  I will commit this patch after the branch happens, probably after the 2.16 official release.

The current version of the patch doesn't work properly if AlwaysRestartServer=false.  In that case when a user terminates a session which has additional server arguments the server isn't restarted and the login screen runs in that X server.  If the next login session doesn't have any additional server arguments the server is still not restarted and continues to run with the (now incorrect) arguments.  This is clearly wrong and is a bug.

A possible fix is to restart the server at the end of a session to remove the additional arguments.  (Of course, the restart would only happen if the user's initial session had additional arguments, otherwise AlwaysRestartServer=false would become meaningless.)

The alternative is to respect the AlwaysRestartServer=false setting for as long as possible and not restart the server when a session with additional arguments is terminated.  The patch could be fixed to restart the server only if the server arguments of the next chosen session differ from those then in effect.

Does anyone have any preference?  Or other ideas?

My vote goes for deprecating the AlwaysRestartServer option and just restarting the server unconditionally.

It's always sort of struck me as an odd option to punt to the user.

The advantage of AlwaysRestartServer is that on some systems, it improves the performance of the machine since we avoid the need to restart the server 
unless needed.  However, this can cause some issues if, for example, the Xserver leaks memory then never restarting the Xserver will result in possible resource issues.

I wouldn't be opposed to deprecating the AlwaysRestartServer option and changing GDM to always restart.  This is probably safer since relying on the Xserver not having any resource consumption issues is probably dangerous anyway.

However, it might also be reasonable to continue supporting this flag but
restart the Xserver only when necessary.  This could be done by the daemon remembering the previous options and restartng the Xserver only if the options are different.  However it might be easier to just simply restart the Xserver on the next session after non-default Xserver options are used.  So if I use special Xserver options by picking session X, it will restart the Xserver on the next session (even if session X is picked again).  This might be easier than making the daemon keep track of the arguments used and comparing them, and would work reasonably I'd think.  Note that the "GdmDisplay" structure (typically the variable "d") contains flags and such to keep track of the state of each display.  Might need to add a variable to the structure to keep track of 
additional state information to do this sort of comparison.

I'd be happy either way.  I think the added value of users being able to 
have more control over Xserver arguments is more valuable than the 
AlwaysRestartServer option, but we should think about the best way to support
this.

Created attachment 70892 [details] [review]
Restart X server after session with additional arguments

It's easier to restart the X server unconditionally than remember the arguments for later comparison.  So that's what I've implemented in this patch, just to see what it feels like.

Any additional arguments specified in a selected session are only used for the duration of that session, after that we always revert to the default arguments (whatever the setting of AlwaysRestartServer).

This is fine.  But if we are going to deprecate the AlwaysRestartServer configure option, then the docs/gdm.xml file should be updated to reflect this deprecation, explain why it is deprecated, etc.  Could you update the patch so that the documentation explains more about this.

Also, the documentation you wrote for the new feature is a bit light.  You only describe the new option, and not the format of the .desktop files in full.  It would be nice if the documentation were updated to explain how these files work more fully.  You don't have to do this, but it would be nice since you are mucking around in this part of the code.  It would probably be good if we at least referred to the FreeDesktop "Desktop Entry Spec" and explained that this new option is a GDM specific extension to the specification:

   http://www.freedesktop.org/wiki/Standards/desktop-entry-spec

Probably also good to highlight that the new option will obviously be ignored by other display managers, such as KDM, unless they support this extension in the future, etc.

Also, if we are deprecating this key, we should remove the key (and its comments) from the default.conf file, and no longer read in or process they key in the daemon code.

If the AlwaysRestartServer key is to be deprecated or removed I think it should be the subject of a separate bug report, discussion and patch, not a side effect of this one.

Agreed, yes.  This will give people time to consider the ramifications of this proposed change for 2.17.  I don't suspect anyone would have any concerns because AlwaysRestartServer really only adds performance benefit, and is risky to use since leaving the Xserver running constantly invites any resource consumption problems introduced by the Xserver.

I think that for this reason, it is probably a good reason to deprecate
this interface.  However, I suppose it is possible that someone could think
that this interface adds value to them and would like to continue using it.
However, there really is no reason that users can't continue using this 
feature if they wanted to, they just would need to be aware of how using
.xsession files with Xargs affects them.  I suppose sysadmins could be 
encouraged to make use of this feature in a sane way, and we can make sure
that the default GDM configuration is also sane.  Or do people expect that
that there would be serious problems with the problem as described?  What
bad would happen if special Xargs were carried over into future sessions?

For example.  In terms of a11y, some Xserver extensions are useful for users
with specific problems.  An example is XEViE which allows AT programs to have
additional information about Xevents.  For example, some systems may want to
set up an "accessibility" xsession file that turns on the -XEvIE extension.
It's only needed for AT programs to work, so you really don't need to load
the extension if a user with a11y needs isn't using the system.  So making
a special a11y session allows systems that have no a11y users to perform
better than machines that truly require XEViE for a11y reasons.  

In this example, I'm not sure that the "bug" as described is so bad.  Are
there other planned usages of this feature that would cause a serious problem
if the Xserver arguments specified in the Xargs field were carried over into
the next session?

Obviously users who use AlwaysRestartServer=true are willing to deal with
the possible resource issues introduced by the Xserver.  Since XEvIE is a
new Xserver extension, I suppose any memory leaks it might introduce might
cause AlwaysRestartServer users to have their machine fall over when all
their memory is consumed.  Users who turn AlwaysRestartServer on like to live 
on the edge, and their efforts help to correct memory leak bugs in the Xserver
code, I think.  We should perhaps encourage them to keep up the good work by 
adding a feature that could expose memory consumption problems in the Xserver, 
do you think?  :)

I really don't recommend that any distro use this feature by default.  Perhaps
the documentation should be more clear that this feature is a bit dangerous
to use?  

AlwaysRestartServer perhaps should not be set to "true" by default.  Perhaps we could address this problem by changing the default value and documenting
the above sort of issues?

This is now in SVN head.  Sorry it took so long to commit.  I reworked the patch quite a bit to get it to compile against head, and also store the extra args in the display structure rather than using the ugly global variables.  Also added an if-test to see if we are running Xnest.  If so, we don't want to add any extra args since Xnest doesn't work with restarting.  In this case they are ignored (updated docs to mention this also).