GNOME Bugzilla – Bug 621581
PostSession not run if session closed by shutdown or restart
Last modified: 2011-03-11 04:48:55 UTC
PostSession/Default is not run if I close my session by shutting down or restarting my computer. It is only run if I log out. To reproduce, put #!/bin/sh touch /var/log/gdm-postsession-timestamp into /etc/gdm/PostSession/Default, and end your session by restarting or shutting down your computer. The file is not touched. If you end a session by logging out, the file is touched.
(In reply to comment #0) > PostSession/Default is not run if I close my session by shutting down or > restarting my computer. It is only run if I log out. To reproduce, put > > #!/bin/sh > touch /var/log/gdm-postsession-timestamp > > into /etc/gdm/PostSession/Default, and end your session by restarting or > shutting down your computer. The file is not touched. If you end a session by > logging out, the file is touched. I can confirm that I see exactly the same behavior using Ubuntu 10.04 on a Dell 1545 laptop. Keith W
When logout the session, gdm will take it as 'session exit normally' and emit a 'session-exited' signal which will trigger the script. When restart or shut down the computer, gdm will take it as 'session finished' and emit a 'session-died' signal which will not trigger the script. It is a current design.
I suspected exactly that. You do, I hope, realise that the current design makes the PostSession script totally useless for any post-session-cleanup purposes? In fact, I cannot fathom any real situation where the current PostSession trigger design is appropriate. It certainly is not sufficient for any purposes I have. Thank you for your time and effort, though.
Agree. To me, restart and shutdown were used much more than logout.
The PostSession script *should* get run when shutting down or rebooting. If this does not work, then this isn't by design, it is a bug. Please submit a patch fixing this issue for consideration.
Created attachment 165394 [details] [review] Patch against gdm-2.31.1 to run the PostSession script even when the session dies, not just at logout. In order to be useful, the PostSession script must be run whenever the user session dies, not just at logout. This untested patch copies the missing bits from gdm-simple-slave.c:on_session_exited() to gdm-simple-slave.c:on_session_died(), making the two functions equivalent (other than the debug comment, of course). Thanks to David Liang for pointing me at the right direction.
It looks good. However, with this change on_session_exited and on_session_died do the same thing. Perhaps they should have the same callback function instead, or both call a common function. Also, is it possible for a session to die and "on_session_died" to be called before the PreSession script is run? It only makes sense to call PostSession if the PreSession script was actually executed for the session. It might make sense to set a boolean flag when the PreSession script is run, and make sure to only run the PostSession script if the PreSession script did run.
Created attachment 165400 [details] [review] Updated postsession patch for GDM-2.31.1 Patch against gdm-2.31.1 to run the PostSession script even when the session dies, not just at logout. This updated patch adds a bitfield need_to_run_postsession to GdmSimpleSlavePrivate structure. It is used to indicate whether or not PostSession should be run if the session dies. (PostSession is always run if the session exits normally.) The flag is cleared at session creation time, and when PostSession is run; it is set after the PreSession script has been run.
Created attachment 165401 [details] [review] Same as above, but with wide context for easier review Same as the above patch (165400), but with a lot of context to make review easier.
I'd appreciate if somebody could test the patch (165400). I'm currently stuck with gconf-2.28.1, and leaving for a three-week holiday tomorrow, so I'm afraid I cannot test it myself before the end of this month. And in any case, it should be tested well by others before inclusion in GDM. However, the patch is simple and straight-forward; if it compiles and passes the test I outlined in the first comment, it is as thoroughly tested as I can think of. (Well, one could kill the entire user session from the console using kill, pkill, or killall, and check whether the PostSession script is run. In real life, that situation indicates either a system library bug (through disk corruption, typically), or out-of-memory condition, and in both cases multiple processes are killed anyway. It would not be reasonable to expect the PostSession script to be somehow shielded from these effects.)
This looks good to me. Ray, what do you think about committing this?
I'm back, and finally got around to testing my patch. First, I added the conditional test into on on_session_exited(), too. I wanted to make absolutely sure PostSession is run if and only if PreSession was run. (In my previous version of the patch, on_session_exited() runs PostSession unconditionally.) There should be absolutely no change in behaviour; I'm just paranoid. Second, I had to adapt the patch to Debian/Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 for my desktop machine, because I do not have a separate test box handy. That version of GDM has Plymouth (boot splash) support patches which conflict with these changes, but I fixed that. I'll attach equivalent patches for both gdm_2.30.2.is.2.30.0-0ubuntu3 and gdm-2.31.1 shortly. I tested the patched gdm_2.30.2.is.2.30.0-0ubuntu3, and it seems to work perfectly.
Created attachment 167079 [details] [review] Tested and working debian/patches/96_postsession.patch for Debian/Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 Extract the sources and apply the diff from https://launchpad.net/ubuntu/+source/gdm/2.30.2.is.2.30.0-0ubuntu3, copy this attachment as debian/patches/96_postsession.patch, add a changelog entry to debian/changelog, and build with 'dpkg-buildpackage -b'. Works for me.
Created attachment 167080 [details] [review] Updated postsession patch for GDM-2.31.1 (equivalent to patch 167079 for gdm_2.30.2.is.2.30.0-0ubuntu3)
Created attachment 168154 [details] [review] PostSession patch for gdm-2.31.90 Respun for gdm-2.31.90, same as 167079 (fully tested 96_postsession.patch for Debian gdm_2.30.2.is.2.30.0-0ubuntu3).
Review of attachment 168154 [details] [review]: Overall the patch looks good. My main question is the additional state variable. ::: gdm-2.31.90.original/daemon/gdm-simple-slave.c @@ +85,3 @@ guint start_session_when_ready : 1; guint waiting_to_start_session : 1; + guint need_to_run_postsession : 1; Why is this boolean here? @@ +110,3 @@ g_debug ("GdmSimpleSlave: session started %d", pid); + You added extra whitespace here. @@ +114,3 @@ username = gdm_session_direct_get_username (slave->priv->session); if (username != NULL) { + slave->priv->need_to_run_postsession = 1; This should be TRUE not 1 (or dropped entirely if we don't need the boolean) @@ +141,3 @@ + if (username != NULL) { + gdm_slave_run_script (GDM_SLAVE (slave), GDMCONFDIR "/PostSession", username); + slave->priv->need_to_run_postsession = 0; FALSE not 0. @@ +160,3 @@ g_strsignal (signal_number)); + /* Run the PostSession script if necessary. */ I really don't like duplicated comments. Can you refactor the two cloned chunks of code to call one subroutine? @@ +165,3 @@ + if (username != NULL) { + gdm_slave_run_script (GDM_SLAVE (slave), GDMCONFDIR "/PostSession", username); + slave->priv->need_to_run_postsession = 0; FALSE @@ +680,3 @@ } + slave->priv->need_to_run_postsession = 0; FALSE
The patch ensures PostSession is run iff PreSession was run. The state variable is a simple, reliable indicator -- regardless of causes and call chain. The private slave structure seemed the most logical place to put it in. Why a state variable at all? Originally, I did not use one. Brian Cameron wondered whether it is possible for a session to die and on_session_died() to be called before the PreSession script is run. After looking at the sources, I assumed it might be. So, having a single state variable as a safeguard seemed appropriate. I'll respun the patch shortly, with your comments applied, but I'd really prefer to keep the state variable -- unless you know it to be unnecessary. (I'm personally not familiar enough with the gdm internals to say.)
Created attachment 168166 [details] [review] Updated PostSession patch for gdm-2.31.90 I moved running both PreSession and PostSession scripts to helper functions (run_presession_script and run_postsession_script). Both return FALSE if successful (like gdm_slave_run_script). Although PreSession is only run from one function, the functions track and modify slave private state thorough need_to_run_postsession. I thought it was better to put them both in separate functions. need_to_run_postsession is set to TRUE iff presession is run successfully, and FALSE when postsession is run successfully. (It is initially set to FALSE in create_new_session.) Let me know if you prefer an if clause instead of reusing the return variable from gdm_slave_run_script, or if you think a comment is needed there.
Created attachment 168168 [details] [review] Updated PostSession patch for Debian/Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 This patch contains the same changes as attachment 168166 [details] [review], but respun for Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 for easier testing on Ubuntu Lucid (10.04). I verified that with this patch, PostSession is run at reboot and shutdown, not just when logging out.
(In reply to comment #17) > The patch ensures PostSession is run iff PreSession was run. The state variable > is a simple, reliable indicator -- regardless of causes and call chain. > The private slave structure seemed the most logical place to put it in. > > Why a state variable at all? Originally, I did not use one. Brian Cameron > wondered whether it is possible for a session to die and on_session_died() to > be called before the PreSession script is run. After looking at the sources, I > assumed it might be. So, having a single state variable as a safeguard seemed > appropriate. Ah, okay, sounds fine.
(In reply to comment #18) > I moved running both PreSession and PostSession scripts to helper functions > (run_presession_script and run_postsession_script). Both return FALSE if > successful (like gdm_slave_run_script). > > Although PreSession is only run from one function, the functions track and > modify slave private state thorough need_to_run_postsession. I thought it was > better to put them both in separate functions. I agree, thanks for doing that. > need_to_run_postsession is set to TRUE iff presession is run successfully, Makes sense. > and FALSE when postsession is run successfully. Should be FALSE even if postsession fails, right?
Created attachment 168212 [details] [review] Yet another updated PostSession patch for gdm-2.31.90 Right, PostSession should only be run at most once per session. This version of the patch sets need_to_run_postsession to FALSE whenever PostSession is run (even if it fails); otherwise the patch is the same as the previous one (attachment 168166 [details] [review] at bugzilla.gnome.org).
Created attachment 168213 [details] [review] Yet another updated PostSession patch for Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 This patch contains the same changes as bugzilla.gnome.org attachment #168212 [details], but respun on top of Ubuntu gdm_2.30.2.is.2.30.0-0ubuntu3 (latest gdm for Ubuntu Lucid) for those who wish to test this on Ubuntu.
Comment on attachment 168212 [details] [review] Yet another updated PostSession patch for gdm-2.31.90 Thanks, feel free to commit.
Well, one more change: Don't elide braces for one statement if clauses.
Created attachment 168263 [details] [review] Final PostSession patch against gdm HEAD Same as gnome bugzilla attachment 168212 [details] [review], except with braces around all three if clauses, and respun against git HEAD (as of . The last pair of braces causes diff to chunk the patch rather strangely, though. I'm not a registered GNOME developer, so I'd appreciate if somebody could commit this.
+1 from me. The code might be a bit cleaner if the run_pressesion_script and run_postsession_script functions were just made one function. The only difference between the two functions is the name of the script to run, which could be passed in via an argument rather than hardcoding it into two separate, nearly-identical functions. But that is a nit, really. I think this fix should go into the next 2.30 release. This is a significant interface improvement which should be supported in the latest stable branch if possible.
I reworked the patch a bit and committed the fix just now.
(In reply to comment #28) > I reworked the patch a bit and committed the fix just now. Hi Brian I'm totally new to bugzilla and have looked at the Help section of this page. I'd simply like to know how I would go about applying the fix mentioned in this discussion. I've updated my Ubuntu 10.04 and I'm running gdm 2.30.2.is.2.30.0-0ubuntu5 and have confirmed that my PostSession/Default script is only run when logging out, not when shutting down/rebooting. Do I have to manually edit some files and how would that be effected once running future updates on my system. Thanx in advance for this.
The patch for fixing this issue is here: http://git.gnome.org/browse/gdm/commit/?h=gnome-2-32&id=6e1858d3c4a51373da425a5240acc452397ea02b Unfortunately you need to rebuild GDM with this patch to get the fix. You probably need to file a bug with Ubuntu to ask them to backport the fix, or you need to rebuild GDM by hand.
(In reply to comment #30) > The patch for fixing this issue is here: > > http://git.gnome.org/browse/gdm/commit/?h=gnome-2-32&id=6e1858d3c4a51373da425a5240acc452397ea02b > > Unfortunately you need to rebuild GDM with this patch to get the fix. You > probably need to file a bug with Ubuntu to ask them to backport the fix, or you > need to rebuild GDM by hand. Hi thanx Brian I've never filed a bug before. What prevents Ubuntu from simply saying its a GDM issue and not theirs? I don't at this stage have the technical know-how to go head to head with a Ubuntu developers about who's responsible for fixing the issue. Also how long in general does it take to backport a fix like this? Does the fact that its a LTS version of Ubuntu make a difference at all?
It is not a GDM issue since it has already been fixed in upstream GDM. You need to file a bug in Canonical's bug system, which I think is LaunchPad. You can just explain the problem and point them at this bug report, and they should be able to provide updated packages with the fix. I have no idea how long it takes them to respond to requests like this. You'ld need to ask them.
(In reply to comment #32) > It is not a GDM issue since it has already been fixed in upstream GDM. You > need to file a bug in Canonical's bug system, which I think is LaunchPad. You > can just explain the problem and point them at this bug report, and they should > be able to provide updated packages with the fix. I have no idea how long it > takes them to respond to requests like this. You'ld need to ask them. Thanks Brian. I will file a report with launchpad. Thank again for your help in this.