GNOME Bugzilla – Bug 680689
power: enforcing lid close
Last modified: 2013-01-20 11:46:46 UTC
With the switch to systemd for suspend, we've unfortunately lost some predictability for physical actions such as lid close or power button. These can now cause a polkit dialog to pop up (if inhibitors are engaged). This is particularly problematic for lid close, where you can't even see the dialog. We need a way to ensure that we reliably suspend in response to a lid close event. Inhibitors should at most cause a temporary delay; we could e.g. beep a few times, to prod the user to reopen the lid and find the inhibit dialog. But if he doesn't open the lid again, we have to enforce the suspend after a short timeout. systemd may not currently offer the right API for us to implement this.
*** Bug 682420 has been marked as a duplicate of this bug. ***
Rchard, I think we need to do something for this.
Right, I've been looking at the systemd API for this, and apart from providing a "interactive" boolean, there's no way to override the inhibits from the system or session context for laptop lid close. The only way to fix this I see is: * Add a new method to logind, such as SuspendLidClose() which ignores the inhibits completely, and doesn't need a PolkitAuth. Lennart isn't going to like that much. * Handle the lid close in logind * change the defaults for org.freedesktop.login1.suspend-ignore-inhibit from "auth_admin_keep" to "yes", although logind seems to use polkit to enforce the interaction in this case, which isn't well suited to the lid close use case * Switch back to upower and pm-suspend for 3.6 until we've worked out all the issues with the new polkit auths. I'll email Lennart and ask for his opinion, although I think he's still travelling this week.
Inhibits should not block power management, only allow a way to ask the user. If they force the lid close, I'm quite sure they're well aware that their CD burning or whatever is not going to finish properly.
FWIW, my experience this week-end was: 1. Drain laptop battery 2. Plug laptop in 3. Laptop boots automatically, but with the lid closed 4. Laptop never suspends because another user is apparently logged in 5. Laptop is hot to the touch, but you don't realise it 6. Open up laptop bag with a battery at 70% instead of 100%...
Any news on this? My laptop has ended up drained almost four or five times by now, and I'm deathly afraid it's going to end up not working and with the plastic melted. Closing the lid should not be inhibited by any means whatsoever.
I've talked to Lennart about this, its on the top of his todo...
(In reply to comment #8) > I've talked to Lennart about this, its on the top of his todo... Well if we cannot get this fixed in time for 3.6 we should revert the inhibit stuff because this is worse IMO.
Hmm, can somebody tell me which inhibitors were actually taken when this happened?
i.e. I am interested in "systemd-inhibit --list" when this happened.
Here's what I propose: http://lists.freedesktop.org/archives/systemd-devel/2012-September/006604.html Does that make sense to you?
(In reply to comment #12) > Here's what I propose: > > http://lists.freedesktop.org/archives/systemd-devel/2012-September/006604.html > > Does that make sense to you? This seems like more work for you (handling all those "buttons"), and more work for the graphical desktops (having to install the inhibitors). I don't really understand why people using other DEs can't handle the keys themselves, as they should have done anyway. I guess we can work with that, but it means more work in gnome-settings-daemon. In a hard freeze.
One point that came up during irc discussion is that gsd should hold on to the lid close inhibitor for a bit (like, 30 seconds or so) after an external monitor is unplugged, so that users get a chance to unplug the monitor first and then open the lid, without plunging into suspend.
(In reply to comment #14) > One point that came up during irc discussion is that gsd should hold on to the > lid close inhibitor for a bit (like, 30 seconds or so) after an external > monitor is unplugged, so that users get a chance to unplug the monitor first > and then open the lid, without plunging into suspend. I would hold onto the inhibitor all the time, and I would expect logind to just go through with whatever I ask it because g-s-d is both holding the inhibitor, and asking for suspend. Otherwise it makes the code even more complicated.
(In reply to comment #15) > (In reply to comment #14) > > One point that came up during irc discussion is that gsd should hold on to the > > lid close inhibitor for a bit (like, 30 seconds or so) after an external > > monitor is unplugged, so that users get a chance to unplug the monitor first > > and then open the lid, without plunging into suspend. > > I would hold onto the inhibitor all the time, and I would expect logind to just > go through with whatever I ask it because g-s-d is both holding the inhibitor, > and asking for suspend. Otherwise it makes the code even more complicated. Hmm, no. The thing is that allowing people to take inhibition locks thus blocking suspends, and allowing people to ignore inhibition locks (of others) to force suspends is *both* problematic from a security perspective. To make this somewhat secure we try to limit what people can do in this area: a) suspend inhibitors are protected via PK, and only available to processes of locally logged in users. b) suspending with ignored inhibitors is not available at all to unprivileged user code. It is only available to logind itself, i.e. trusted code. logind itself will normally watch the lid switch then and act appropriately. However, The user can request to disable the key handling via a handle-lid-switch inhibitor, which is protected via PK again. In this design the fact that the lid switch was toggled is known to logind and privileged code guarantees that suspend only happens in that case and no other. Or in other words: this approach does not allow random users to suspend the machine ignoring other user's suspends at free will. But the suspend key will still be handled properly. For GNOME two changes are necessary: a) at beginning of the GNOME session the handle-power-key and handle-sleep-key inhibitor locks of logind should be taken, and released when GNOME ends. This ensures that GNOME can handle these two keys on its own. For this gnome-session needs to invoke a single bus call and keep the returned fd around. Since the fd is closed by the kernel when the process exits it doesn't even have to clean it up manually. b) when a second monitor is plugged in the handle-lid-switch inhibitor lock of logind should be taken, and released 20s or so after the monitor is unplugged again. For this gnome-session needs to invoke one bus call when a 2nd screen is plugged in and one close() when it is unplugged again. And that's basically it.
Right now, if somebody hits Suspend from the UI and it's inhibited, do we show a dialog that allows someone to override it? If not, why not? Why can't the same system of overriding inhibitors by user feedback be applied here, rather than special casing lid close directly in the API? I think it's fairly obvious that lid close is a form of user feedback, even if what it should mean depends on context.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > One point that came up during irc discussion is that gsd should hold on to the > > > lid close inhibitor for a bit (like, 30 seconds or so) after an external > > > monitor is unplugged, so that users get a chance to unplug the monitor first > > > and then open the lid, without plunging into suspend. > > > > I would hold onto the inhibitor all the time, and I would expect logind to just > > go through with whatever I ask it because g-s-d is both holding the inhibitor, > > and asking for suspend. Otherwise it makes the code even more complicated. > > Hmm, no. > > The thing is that allowing people to take inhibition locks thus blocking > suspends, and allowing people to ignore inhibition locks (of others) to force > suspends is *both* problematic from a security perspective. To make this > somewhat secure we try to limit what people can do in this area: > > a) suspend inhibitors are protected via PK, and only available to processes of > locally logged in users. > > b) suspending with ignored inhibitors is not available at all to unprivileged > user code. It is only available to logind itself, i.e. trusted code. logind > itself will normally watch the lid switch then and act appropriately. However, > The user can request to disable the key handling via a handle-lid-switch > inhibitor, which is protected via PK again. > > In this design the fact that the lid switch was toggled is known to logind and > privileged code guarantees that suspend only happens in that case and no other. > > Or in other words: this approach does not allow random users to suspend the > machine ignoring other user's suspends at free will. But the suspend key will > still be handled properly. And you can't detect that the code requesting suspend is the currently logged in session? This whole approach sounds like a lot of code, a lot of changes, and it's far too late in the release process to make those changes. I'll start looking into reverting the use of systemd inhibitors in gnome-settings-daemon, we can't make those changes now.
(In reply to comment #18) > And you can't detect that the code requesting suspend is the currently logged > in session? That'd not be sufficient. This still would allow the fg user to suspend the machine ignoring all inhibitors all the time. With the new approach suspending without inhibitors is never allowed to the fg user. Only logind can do that and it will do that only if it saw a lid switch event before. > This whole approach sounds like a lot of code, a lot of changes, > and it's far too late in the release process to make those changes. I'll start > looking into reverting the use of systemd inhibitors in gnome-settings-daemon, > we can't make those changes now. Well, the systemd/logind side is now implemented and in git, soon in F18. It can't be that much work to add two bus calls and one close() syscall to GNOME session, no?
(In reply to comment #19) > (In reply to comment #18) > > > And you can't detect that the code requesting suspend is the currently logged > > in session? > > That'd not be sufficient. This still would allow the fg user to suspend the > machine ignoring all inhibitors all the time. With the new approach suspending > without inhibitors is never allowed to the fg user. Only logind can do that and > it will do that only if it saw a lid switch event before. You said that if g-s-d had an inhibitor, logind just wouldn't look at lid events. I'm pretty sure there will be a case where it won't have seen the event because g-s-d had an inhibitor, and we would want to suspend because the lid had been closed after a small period of time. Either that, or the inhibitor/handling of events interaction isn't well enough defined. > > This whole approach sounds like a lot of code, a lot of changes, > > and it's far too late in the release process to make those changes. I'll start > > looking into reverting the use of systemd inhibitors in gnome-settings-daemon, > > we can't make those changes now. > > Well, the systemd/logind side is now implemented and in git, soon in F18. It > can't be that much work to add two bus calls and one close() syscall to GNOME > session, no? Hard. Code. Freeze. It's been broken since the end of July, nobody took charge. I'm not adding more codepaths to the power plugin now, it's got enough corner case bugs as it is.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > > > And you can't detect that the code requesting suspend is the currently logged > > > in session? > > > > That'd not be sufficient. This still would allow the fg user to suspend the > > machine ignoring all inhibitors all the time. With the new approach suspending > > without inhibitors is never allowed to the fg user. Only logind can do that and > > it will do that only if it saw a lid switch event before. > > You said that if g-s-d had an inhibitor, logind just wouldn't look at lid > events. I'm pretty sure there will be a case where it won't have seen the event > because g-s-d had an inhibitor, and we would want to suspend because the lid > had been closed after a small period of time. Either that, or the > inhibitor/handling of events interaction isn't well enough defined. I don't follow. The choice GNOME has is basically: a) it can leave lid switch handling to logind. Which will suspend ignoring all inhibitors, by default. b) it can take the handle-lid-switch inhibitor lock and do the suspending on its own or not at all. if it decides to suspend then it can do that, but inhibitors would be respected.
Lennart, to make sure I understand this correctly, let me describe how I guess we should handle our use cases with this api: - gsd takes the handle-lid-switch inhibitor - when the lid is closed, and there are external monitors plugged in, and we are configured to not suspend in this case, we keep the inhibitor and do nothing - when the external monitors are unplugged (or the setting toggled, or whatever), and the lid is still closed, we drop the handle-lid-switch inhibitor to force a suspend without interference from other inhibitors - when the lid is closed and there's no external monitor, we drop the handle-lid-switch inhibitor and again, logind will do its thing - when the suspend action is triggered some other way, we keep the lid-switch-handle inhibitor and call Suspend to have inhibitors respected Sound right ?
and of course, whenever the lid is opened, we take the handle-lid-switch inhibitor back.
(In reply to comment #22) > Lennart, to make sure I understand this correctly, let me describe how I guess > we should handle our use cases with this api: > > - gsd takes the handle-lid-switch inhibitor Only if a second display is plugged in! > - when the lid is closed, and there are external monitors plugged in, and we > are configured to not suspend in this case, we keep the inhibitor and do > nothing This is the only time where you take the lock. > - when the external monitors are unplugged (or the setting toggled, or > whatever), and the lid is still closed, we drop the handle-lid-switch inhibitor > to force a suspend without interference from other inhibitors Yes, you simply release the lock on unplug of the screen. > - when the lid is closed and there's no external monitor, we drop the > handle-lid-switch inhibitor and again, logind will do its thing > > - when the suspend action is triggered some other way, we keep the > lid-switch-handle inhibitor and call Suspend to have inhibitors respected > > Sound right ? Not really. You never call Suspend() on your own. If the second screen is plugged you take the handle-lid-switch lock. If the second screen is unplugged you release the handle-lid-switch lock. That is all.
How does this fix the issue of "if there are other inhibitors, the system cannot suspend"? It sounds like it doesn't.
Thanks for the clarification, Lennart. Much simpler, indeed. > It sounds like it doesn't. It does by taking inhibitors out of the equation for lid close. If logind handles a lid close, suspend inhibitors are not looked at. They are only looked at if we call Suspend(). Which we are not going to do anymore for lid close.
(In reply to comment #24) > Not really. You never call Suspend() on your own. To clarify this: GNOME should never call Suspend() as part of the lid switch logic. It should however call it if the user asked for suspend via the UI
This is now fully implemented in systemd git. I have also added the docs for this, and added a paragraph here: http://www.freedesktop.org/wiki/Software/systemd/inhibit
systemd 190 with all this in place is now released (and on in Bodhi, waiting to enter F18). If due to the freeze it is decided not do adopt this just yet we could add a temporary hack in Fedora that simply wraps gnome-session in a call to systemd-inhibit. I file a bug against the package in Fedora, suggesting how this would look: https://bugzilla.redhat.com/show_bug.cgi?id=859224
You said that if g-s-d held an inhibitor, logind wouldn't see/handle the lid close. How can we have cool-off periods as we do now (unplug external monitor, then open lid and not have suspend) with this architecture?
(In reply to comment #30) > You said that if g-s-d held an inhibitor, logind wouldn't see/handle the lid > close. How can we have cool-off periods as we do now (unplug external monitor, > then open lid and not have suspend) with this architecture? GNOME should just keep the lock for a while after the monitor unplug. I carefully made sure that the right thing will happen for the lid switch: i.e. if the lid is being closed as the lock is taken no suspend happens. But if the lock is then released while the lid is still closed we will immediately suspend.
If the user chooses Suspend from the menu, and there's an active inhibitor, will pop up a dialog that lets the user bypass it?
(In reply to comment #32) > If the user chooses Suspend from the menu, and there's an active inhibitor, > will pop up a dialog that lets the user bypass it? Well, showing something nice is GNOME's job. Otherwise there'll just be a PK prompt asking for auth to ignore the blocker.
(In reply to comment #33) > (In reply to comment #32) > > If the user chooses Suspend from the menu, and there's an active inhibitor, > > will pop up a dialog that lets the user bypass it? > > Well, showing something nice is GNOME's job. Otherwise there'll just be a PK > prompt asking for auth to ignore the blocker. Why PolicyKit? That isn't required for the shutdown/logout case (unless it is and we just have sane policy by default) The thing about this is that it relies on a sane API to tell systemd, "yo, the user really wants to suspend, and yeah, he knows that his disk is still burning and doesn't care. Please just suspend for me? Thanks". From what I understand, we don't have that API.
Overriding suspend inhibitors is a privileged operation. We shouldn't allow user "Jasper" shutting down the machine if user "Lennart" is currently burning a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. Other than that, it's GNOME's job to show a nice UI for active inhibitors.
(In reply to comment #35) > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > user "Jasper" shutting down the machine if user "Lennart" is currently burning > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. Unless user "Jasper" is logged in remotely this is nonsense. He could as well just pull the power cord / remove the battery if he does not know the admin password ... i.e we should just warn the user but not ask for a password "Another user is logged in do you really want to shutdown [yes] [no]" ... but that's a different story.
(In reply to comment #35) > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > user "Jasper" shutting down the machine if user "Lennart" is currently burning > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. > Other than that, it's GNOME's job to show a nice UI for active inhibitors. What if I have no other users, it's just an app in my current session? We should allow a non-privileged override in that case, no?
(In reply to comment #36) > (In reply to comment #35) > > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > > user "Jasper" shutting down the machine if user "Lennart" is currently burning > > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. > > Unless user "Jasper" is logged in remotely this is nonsense. He could as well > just pull the power cord / remove the battery if he does not know the admin > password ... i.e we should just warn the user but not ask for a password > "Another user is logged in do you really want to shutdown [yes] [no]" ... but > that's a different story. That's a matter of PK policies.
(In reply to comment #37) > (In reply to comment #35) > > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > > user "Jasper" shutting down the machine if user "Lennart" is currently burning > > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. > > Other than that, it's GNOME's job to show a nice UI for active inhibitors. > > What if I have no other users, it's just an app in my current session? We > should allow a non-privileged override in that case, no? Yeah, agreed. I'll change logind to not require PK auth if the user wants to override his own inhibitors.
(In reply to comment #38) > (In reply to comment #36) > > (In reply to comment #35) > > > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > > > user "Jasper" shutting down the machine if user "Lennart" is currently burning > > > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. > > > > Unless user "Jasper" is logged in remotely this is nonsense. He could as well > > just pull the power cord / remove the battery if he does not know the admin > > password ... i.e we should just warn the user but not ask for a password > > "Another user is logged in do you really want to shutdown [yes] [no]" ... but > > that's a different story. > > That's a matter of PK policies. Why? Doesn't logind not differentiate between local and remote logins?
(In reply to comment #39) > Yeah, agreed. I'll change logind to not require PK auth if the user wants to > override his own inhibitors. What's the API to override inhibitors here?
(In reply to comment #40) > (In reply to comment #38) > > (In reply to comment #36) > > > just pull the power cord / remove the battery if he does not know the admin > > > password ... i.e we should just warn the user but not ask for a password > > > "Another user is logged in do you really want to shutdown [yes] [no]" ... but > > > that's a different story. > > > > That's a matter of PK policies. > > Why? Doesn't logind not differentiate between local and remote logins? Yes it does. But so does PK, and whether people can get a suspend blocker is (by default) bound to whether they are logged in locally.
(In reply to comment #41) > (In reply to comment #39) > > Yeah, agreed. I'll change logind to not require PK auth if the user wants to > > override his own inhibitors. > > What's the API to override inhibitors here? There is no special API for that. logind is just the mechanism that will execute what the user asks (if he has the permission). If the user has the privs to override the inhibitors logind will simply go ahead and execute the actions. It's the job of the UI to check for inhibitors first and present them if that's desired.
So they're not inhibitors... I was assuming that other things that try to shut down the system (pm-suspend) would be inhibited by this, too. But this is not Bugzilla territory. Sorry.
(In reply to comment #44) > > But this is not Bugzilla territory. Sorry. Yes, please take this to irc. The purpose of this blocker bug is to get an implementation of whats outlined in comments 22 and 24.
(In reply to comment #39) > (In reply to comment #37) > > (In reply to comment #35) > > > Overriding suspend inhibitors is a privileged operation. We shouldn't allow > > > user "Jasper" shutting down the machine if user "Lennart" is currently burning > > > a CD, unless "Jasper" can authorize as admin. That's why we use PK for this. > > > Other than that, it's GNOME's job to show a nice UI for active inhibitors. > > > > What if I have no other users, it's just an app in my current session? We > > should allow a non-privileged override in that case, no? > > Yeah, agreed. I'll change logind to not require PK auth if the user wants to > override his own inhibitors. Just a quick follow-up. This is now implemented, and I will upload this to F18, as well.
As discussed with hughsie on IRC I have now split up handle-sleep-key into handle-suspend-key and handle-hibernate-key. g-s-d/g-s should hence take these two new inhibitor locks, and not take handle-sleep-key anymore, as that one doesn't exist anymore.
Richard, I have systemd 191 on my system now, and can test a patch or branch.
Created attachment 225142 [details] [review] test patch for initial review Bastien, could you give this patch a 40,000ft quick review for me please? Once you're basically happy with the layout and concepts I'll work on tidying it up and splitting it into committable pieces for master. We probably can do something smaller for gnome-3-6. And, this also removes the upower and consolekit support for the actions. This might make it a bit controversial (as non-systemd distros will need to distro-patch stuff back in), but keeping the two codepaths was a quick way to insanity. If you want me to support both I can do it, but it'll be lots more code. Thanks.
Review of attachment 225142 [details] [review]: I think this use case won't work with this code: - lid is closed, external display is enabled - yank out video output - open lid This shouldn't suspend because there's a "safety timer" value that's not used anywhere anymore: GSD_POWER_MANAGER_LID_CLOSE_SAFETY_TIMEOUT ::: plugins/media-keys/gsd-media-keys-manager.c @@ +157,3 @@ + /* systemd stuff */ + GDBusProxy *sd_proxy; systemd_proxy, or better logind_proxy. @@ +1628,3 @@ +power_action_suspend (GsdMediaKeysManager *manager) +{ +#ifndef HAVE_SYSTEMD I think you want to make it a hard-dep. @@ +2439,3 @@ + g_warning ("Failed to connect to system bus: %s", + error->message); + g_error_free (error); return? @@ +2440,3 @@ + error->message); + g_error_free (error); + } else { And then drop the else branch. @@ +2460,3 @@ + } + + g_debug ("Adding system inhibitor"); Something a wee bit clearer would be nice. @@ +2467,3 @@ + "handle-power-key:handle-sleep-key", + g_get_user_name (), + "GNOME handling keypresses", Does this need to be translated? I hope it never gets to the UI... ::: plugins/power/gsd-power-manager.c @@ +3764,3 @@ + return FALSE; + manager->priv->sd_proxy = + g_dbus_proxy_new_sync (bus, If you're doing all that sync, just use g_dbus_proxy_new_sync() in the first place, saves you a separate call. @@ -3804,3 @@ } - kill_lid_close_safety_timer (manager); Where does the safety timer live now?
Review of attachment 225142 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +3610,3 @@ + +static void +inhibit_suspend (GsdPowerManager *manager) It would be good to have a comment here explaining this delay inhibitor business @@ +3676,3 @@ + inhibit_lid_switch (manager); + else + uninhibit_lid_switch (manager); Here is where the timeout handling is missing, I guess @@ +3748,3 @@ + } else { + handle_resume_actions (manager); + /* set up the delay again */ You say 'again' here - where is this set up initially ? @@ +3779,3 @@ + g_signal_connect (manager->priv->sd_proxy, "sd_proxy", + G_CALLBACK (sd_proxy_signal_cb), + manager); If you used codegen (or a hand-written subclass), you could get a proper prepare-for-sleep signal, but not a big deal...
taking off the blocker list, since we don't plan to merge this for 3.6.0
Review of attachment 225142 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +2465,3 @@ + "Inhibit", + g_variant_new ("(ssss)", + "handle-power-key:handle-sleep-key", handle-sleep-key got split into handle-suspend-key and handle-hibernate-key.
Review of attachment 225142 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +3779,3 @@ + g_signal_connect (manager->priv->sd_proxy, "sd_proxy", + G_CALLBACK (sd_proxy_signal_cb), + manager); And you actually want to connect to "g-signal" here. The current code gives: (gnome-settings-daemon:9074): GLib-GObject-WARNING **: gsignal.c:2459: signal `sd_proxy' is invalid for instance `0x98c8c0'
Review of attachment 225142 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +2435,3 @@ manager->priv = GSD_MEDIA_KEYS_MANAGER_GET_PRIVATE (manager); + + bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); You're passing an uninitialized error here: (gnome-settings-daemon:10686): GLib-GIO-CRITICAL **: g_bus_get_sync: assertion `error == NULL || *error == NULL' failed
+static void +sd_proxy_signal_cb (GDBusProxy *proxy, + gchar *sender_name, + gchar *signal_name, + GVariant *parameters, + gpointer user_data) It's "const gchar*", not just "gchar*" for param 2 and 3 (don't blindly trust gtk-doc).
Results from initial testing: - lid close works reliably, regardless of inhibitors, yay - taking an inhibitor (as root) and hitting the sleep key gives me a polkit dialog, as expected - the shell is calling into upower for suspend, and that seems to ignore the inhibitor - what gives ? Is this because upower is running as root, and the logind now ignores inhibitors by the same user ? - suspending from the user menu locks the screen, Fn-F4 doesn't - although I do see code that seems to be meant to do it...
Created attachment 225331 [details] [review] tested patch Here is a revised patch that mostly works. A few warts remain, eg gnome-shell should really take the delay inhibitor itself, to hold up suspend while the shield is coming down, I think. And suspend inhibitors don't work quite the same for the shell (because it suspends via upower, I believe)
tested with an external monitor in the office, and that works too
Filed bug 685053 for some followup work in the shell.
Not sure if there are other factors involved, but when applying this latest patch I was able to suspend once via the Suspend menu (network manager didn't properly recover on resume but that might be a separate systemd-suspend issue - I'll ignore it for now). After that I tried lid suspend and that worked OK too (NM OK on resume this time). After this, I was unable to suspend via the menu any longer (the shield comes down, but no actual suspend). Each time I get a message: Oct 01 09:37:56 jimmy dbus[2166]: [system] Rejected send message, 5 matched rules; type="method_return", sender=":1.3" (uid=0 pid=2158 comm="/usr/lib/systemd/systemd-logind ") interface="(unset)" member="(unset)" error name="(unset)" requested_reply="0" destination=":1.86" (uid=603 pid=4168 comm="gnome-session ") So some rule somewhere is causing gnome-session to not be able to talk to logind. That said, it seems the message also shows up with the lid, but suspend does still occur, so I guess this sounds like I have some inhibitor is present and my polkit auth stuff is messed up? I'm not sure how to check the inhibitors tho'
Review of attachment 225331 [details] [review]: I think there's still a problem with the timer never getting cleaned up. ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1628,3 @@ +power_action_suspend (GsdMediaKeysManager *manager) +{ +#ifndef HAVE_SYSTEMD I'd say we should require systemd and be done with it. @@ +2415,3 @@ + g_error_free (error); + } else { + g_variant_get (res, "(h)", &idx); If this necessary when only one fd is ever returned? idx = 0. ::: plugins/power/gsd-power-manager.c @@ +2200,3 @@ +{ + if (!external_monitor_is_connected (manager->priv->x11_screen) + || g_settings_get_boolean (manager->priv->settings, || on the previous line. @@ +2228,3 @@ + manager->priv->inhibit_lid_switch_timer_id = g_timeout_add_seconds (GSD_POWER_MANAGER_LID_CLOSE_SAFETY_TIMEOUT, + (GSourceFunc) inhibit_lid_switch_timer_cb, + manager); There's nothing cleaning this up if the lid status changes for example. @@ +3439,3 @@ + /* Wait until gnome-shell shield animation is done + * + * FIXME: the shell should mark the lock as active Add a bug reference. @@ +3550,3 @@ + g_error_free (error); + } else { + g_variant_get (res, "(h)", &idx); Ditto other file, single fd so no need for indexes. @@ +3572,3 @@ + g_dbus_proxy_call_with_unix_fd_list (manager->priv->logind_proxy, + "Inhibit", + g_variant_new ("(ssss)", Can you please split this out? Looks quite unsightly. @@ +3575,3 @@ + "handle-lid-switch", + g_get_user_name (), + "Multiple displays attached", Does this need translating? @@ +3600,3 @@ +inhibit_suspend_done (GObject *source, + GAsyncResult *result, + gpointer user_data) Use the target type directly in the signal handler, saves code later on. @@ +3754,3 @@ + + /* this displays the unlock dialogue so the user doesn't have + * to move the mouse or press any key before the window comes up */ Do we have any better APIs for this? @@ +4005,3 @@ g_signal_handlers_disconnect_by_data (manager->priv->up_client, manager); + g_clear_object (&manager->priv->connection); All those g_clear_object() cleanups should be done separately.
(In reply to comment #62) > ::: plugins/media-keys/gsd-media-keys-manager.c > @@ +1628,3 @@ > +power_action_suspend (GsdMediaKeysManager *manager) > +{ > +#ifndef HAVE_SYSTEMD > > I'd say we should require systemd and be done with it. Thats fine for landing the patch in Fedora, not sure it will work for upstreaming. > @@ +2415,3 @@ > + g_error_free (error); > + } else { > + g_variant_get (res, "(h)", &idx); > > If this necessary when only one fd is ever returned? > idx = 0. Sure, it will be 0 in practice, but this is the correct way to access passed fds, so I'd like to keep it that way. > @@ +2228,3 @@ > + manager->priv->inhibit_lid_switch_timer_id = g_timeout_add_seconds > (GSD_POWER_MANAGER_LID_CLOSE_SAFETY_TIMEOUT, > + > (GSourceFunc) inhibit_lid_switch_timer_cb, > + > manager); > > There's nothing cleaning this up if the lid status changes for example. The timers stays in place as long as an external monitor is connected. Think of the timer as the thing that removes the handle-lid-close inhibitor when external monitors go away. It is not involved with lid close at all. > @@ +3575,3 @@ > + > "handle-lid-switch", > + g_get_user_name > (), > + "Multiple displays > attached", > > Does this need translating? Not really. The only place it shows up is in d-feet, if you call ListInhibitors. > @@ +3754,3 @@ > + > + /* this displays the unlock dialogue so the user doesn't have > + * to move the mouse or press any key before the window comes up */ > > Do we have any better APIs for this? No, and it is not even implemented in the shell currently - I saw a patch for it land in bugzilla today. I'll attach a revised patch in a bit.
Created attachment 225515 [details] [review] use g_clear_object
Created attachment 225516 [details] [review] cleaned up patch
Review of attachment 225515 [details] [review]: ++
(In reply to comment #63) > > @@ +2228,3 @@ > > + manager->priv->inhibit_lid_switch_timer_id = g_timeout_add_seconds > > (GSD_POWER_MANAGER_LID_CLOSE_SAFETY_TIMEOUT, > > + > > (GSourceFunc) inhibit_lid_switch_timer_cb, > > + > > manager); > > > > There's nothing cleaning this up if the lid status changes for example. > > The timers stays in place as long as an external monitor is connected. Think of > the timer as the thing that removes the handle-lid-close inhibitor when > external monitors go away. It is not involved with lid close at all. That means that sometimes, instead of having a 30 seconds safety, you could have something much shorter. Plug the monitor, wait about 25 seconds, close the lid, inhibit_lid_switch_timer_cb() is called and an external monitor isn't there, sleeps. I think you should manually cancel the timeout in on_randr_event()'s other branch.
Review of attachment 225516 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +2202,3 @@ + g_settings_get_boolean (manager->priv->settings, + "lid-close-suspend-with-external-monitor")) { + g_debug ("no external monitors for a while; uninhibing lid close"); Typo.
(In reply to comment #67) > > > The timers stays in place as long as an external monitor is connected. Think of > > the timer as the thing that removes the handle-lid-close inhibitor when > > external monitors go away. It is not involved with lid close at all. > > That means that sometimes, instead of having a 30 seconds safety, you could > have something much shorter. Plug the monitor, wait about 25 seconds, close the > lid, inhibit_lid_switch_timer_cb() is called and an external monitor isn't > there, sleeps. > > I think you should manually cancel the timeout in on_randr_event()'s other > branch. Yeah, I had actually thought about that, but hadn't gotten around to doing it. I'll revise the patch to do so.
Created attachment 225544 [details] [review] another iteration Fixed the typo, and am now restarting the timer on xrandr and lid events, so the user always gets a 30 second window. Haven't tried this yet (need to get back to the second monitor in the office)
Review of attachment 225544 [details] [review]: Looks good! Richard, can you try and review the changes quickly?
Tested now, reinstalling the timer works as expected and gives me a 30 second window after each xrandr or lid event.
(In reply to comment #71) > Richard, can you try and review the changes quickly? Yes, this patch fixes all the problems with the second monitor and with the screensaver locking. It looks good to me too, thanks.
Building this for f18 now.
Comment on attachment 225544 [details] [review] another iteration Marking as needs-work for GNOME 3.8, as we need to remove the HAVE_SYSTEMD lines in the code, and require a logind compatible interface in 3.8.
Created attachment 225705 [details] [review] prevent inhibitor leaks
Created attachment 225709 [details] [review] prevent inhibitor leaks
Review of attachment 225709 [details] [review]: Looks good.
What |Removed |Added ------------------------------------- Attachment #225709 [details]|none |needs-work status| | (In reply to comment #78) > Review of attachment 225709 [details] [review]: > > Looks good. um
(In reply to comment #79) > What |Removed |Added > ------------------------------------- > Attachment #225709 [details]|none |needs-work > status| | > (In reply to comment #78) > > Review of attachment 225709 [details] [review] [details]: > > > > Looks good. > > um Read the whole bug. It's good for Fedora 18 as a distributor patch, it's not OK to commit to master.
Created attachment 226786 [details] [review] fix a typo The last version of the patch had a typo where we forgot to reset suspend_inhibit_taken to FALSE, causing the screen locking to only work for the first Fn-F4 press.
(In reply to comment #81) > The last version of the patch had a typo where we forgot to reset > suspend_inhibit_taken to FALSE, causing the screen locking to only work for the > first Fn-F4 press. Ahh that likely explains why my screen is not locked after resuming from suspend (via lid usually) - I meant to ask about that earlier, so thanks for fixing it anyway :)
Created attachment 226971 [details] [review] power and media-keys: Use logind for suspending and rebooting the system Use the new logind features to suspend and resume but making sure we opt out of logind handling the sleep and power keys, and also inhibiting for lid close auto-suspend if there is an external monitor connected. Also use a delay inihibit for logind so that we can do actions on suspend like blanking the screen using the screensaver and also poking the screensaver on resume.
Attachment 226971 [details] pushed as 6defe42 - power and media-keys: Use logind for suspending and rebooting the system
I tested the patch from comment 84 (rebased on gnome-settings-daemon 3.6.3 and this works 'mostly' good for me. There are still some fights (might be a different bug report?) though: - Having g-s-d configured to 'interactive' when the power button is pressed results in: + gnome-shell showing the dialog + systemd directly shutting down the machine.. I assume g-s needs to inhibt this as well, and announce that it's aware of how to do things there too.