Restart spawns a password box

With:

http://cgit.freedesktop.org/upower/commit/?id=9843589d2d80e6dc2b3f51338e64bd1da1c53860

upower is deprecating suspend/hibernate interface.

http://lists.freedesktop.org/archives/devkit-devel/2013-January/001339.html

It looks like xfce4-session doesn't include upower.h, but has it's own interface to upower?

http://www.freedesktop.org/wiki/Software/systemd/logind

has information about the logind dbus interface.

Prelim patch:

http://pkgs.fedoraproject.org/cgit/xfce4-session.git/diff/xfce4-session-4.10.0-add-systemd-support.patch?id=54a810e82ab5251ed42e7505025a12d5af6ced82

To upower is deprecating the suspend/hibernate api in favour of using systemd?

Yes, if built with --enable-systemd option, now it will use systemd suspend/hibernate methods instead of upower.

Created attachment 4995
Add-systemd-logind-support-for-suspend-hibernate.patch

Patch for current master branch.

Created attachment 5004
Add-systemd-logind-support-for-suspend-hibernate2.patch

Updated patch with 'lock screen' support.

Created attachment 5085
Add logind runtime detection

I took the patch from this bug report and from Bug #10076 to combine them (suspend/hibernate support + logind runtime detection). It could probably be done in a nicer way, but it seems to work fine.

(I tested with the 4.10.1 release tarball so it won't apply as is over git, e.g. replace configure.ac with configure.ac.in in the patch.)

yeap, the logind / ConsoleKit detection(s) should be done at runtime, so user can boot with whatever init= he has, in Gentoo's case, OpenRC (ConsoleKit) _or_ systemd (logind)

just saying :)

Created attachment 5117
support suspend/hibernate through logind

(In reply to Lionel Le Folgoc from comment #7)
> Created attachment 5085 [details]
> Add logind runtime detection
>
> I took the patch from this bug report and from Bug #10076 to combine them
> (suspend/hibernate support + logind runtime detection). It could probably be
> done in a nicer way, but it seems to work fine.
>
> (I tested with the 4.10.1 release tarball so it won't apply as is over git,
> e.g. replace configure.ac with configure.ac.in in the patch.)

Your patch calls g_object_unref() on potentially uninitialized or NULL pointers (shutdown->consolekit and shutdown->systemd in xfce4-session/xfsm-shutdown.c:xfsm_shutdown_finalize()). The attached patch fixes that and also moves the LOGIND_RUNNING macro into xfce4-session/xfsm-systemd.h and caches its result in xfce4-session/xfsm-shutdown.c. It applies to current git.

What's the status on these patches?

(In reply to Guido Berhoerster from comment #9)
> Created attachment 5117 [details]
> support suspend/hibernate through logind

Would it be possible to implement the HybridSleep() method as well?

Is there some reason why no solution of some kind has been integrated into the official repository? People still can even work around it by passing --enable-deprecated to upower's configure (as we do on NixOS now), but in time most distros will need a better solution.

(In reply to Vladimír Čunát from comment #12)
> Is there some reason why no solution of some kind has been integrated into
> the official repository? People still can even work around it by passing
> --enable-deprecated to upower's configure (as we do on NixOS now), but in
> time most distros will need a better solution.

Because nobody volunteered for maintaining xfce4-session long term, unfortunately.

Created attachment 5498
Suspend/hibernate upower 0.99 (no logind)

Since everyone else is doing the logind piece, I wrote a small patch so xfce4-session will work with upower 0.99 but without logind.

(In reply to Eric Koegel from comment #14)
> Created attachment 5498 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> Since everyone else is doing the logind piece, I wrote a small patch so
> xfce4-session will work with upower 0.99 but without logind.

Although I think it would be a good thing not to rely on logind, please do not extend this crude and ugly sudo hack that BSD and Solaris users have to put up with, but replace xfsm-shutdown-helper with a small DBus service and use PolicyKit to authorize access to shutdown/suspend methods. xfsm-shutdown-helper should just go away, PolicyKit is nowadays available even on the BSDs and Solaris.

(In reply to Guido Berhoerster from comment #15)
> (In reply to Eric Koegel from comment #14)
> > Created attachment 5498 [details]
> > Suspend/hibernate upower 0.99 (no logind)
> >
> > Since everyone else is doing the logind piece, I wrote a small patch so
> > xfce4-session will work with upower 0.99 but without logind.
>
> Although I think it would be a good thing not to rely on logind, please do
> not extend this crude and ugly sudo hack that BSD and Solaris users have to
> put up with, but replace xfsm-shutdown-helper with a small DBus service and
> use PolicyKit to authorize access to shutdown/suspend methods.
> xfsm-shutdown-helper should just go away, PolicyKit is nowadays available
> even on the BSDs and Solaris.

Indeed, I can look into doing that. I didn't know how long it would take to get a new project accepted by distros, especially something running as root. In the meantime, extending shutdown-helper will work until I (or someone) makes a daemon for that.

(In reply to Eric Koegel from comment #14)
> Created attachment 5498 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> Since everyone else is doing the logind piece, I wrote a small patch so
> xfce4-session will work with upower 0.99 but without logind.

There is a problem with this patch, it makes upower a hardcoded mandatory depend, whereas it should be considered optional, see http://bugs.gentoo.org/512084 for example
Should not use XDT_CHECK_PACKAGE([UPOWER],[upower-glib], [0.9.8]) but perhaps XDT_CHECK_OPTIONAL_PACKAGE instead, or put it somehow behind some 'ifdef'

Thanks

(In reply to Eric Koegel from comment #16)
> (In reply to Guido Berhoerster from comment #15)
> > (In reply to Eric Koegel from comment #14)
> > > Created attachment 5498 [details]
> > > Suspend/hibernate upower 0.99 (no logind)
> > >
> > > Since everyone else is doing the logind piece, I wrote a small patch so
> > > xfce4-session will work with upower 0.99 but without logind.
> >
> > Although I think it would be a good thing not to rely on logind, please do
> > not extend this crude and ugly sudo hack that BSD and Solaris users have to
> > put up with, but replace xfsm-shutdown-helper with a small DBus service and
> > use PolicyKit to authorize access to shutdown/suspend methods.
> > xfsm-shutdown-helper should just go away, PolicyKit is nowadays available
> > even on the BSDs and Solaris.
>
> Indeed, I can look into doing that. I didn't know how long it would take to
> get a new project accepted by distros, especially something running as root.
> In the meantime, extending shutdown-helper will work until I (or someone)
> makes a daemon for that.

I don't think that it's big deal, it will not be used by most major Linux distros anyway since they are all systemd-based and there is already precedence with xfpm-backlight-helper which gets executed as root through polkit.
Speaking of, could you just execute xfpm-pm-helper through pkexec instead of sudo in the same way it is done with xfpm-backlight-helper?
xfpm-pm-helper should probably disabled by default on Linux since distros either carry upower < 0.99 or they have upower >= 0.99 and systemd.
Finally it'd be nice to install all of these helpers under $(libexecdir) instead of $(bindir) since they're not meant to be executed directly by the user and clutter $PATH. Thanks in advance.

(In reply to Guido Berhoerster from comment #18)
> I don't think that it's big deal, it will not be used by most major Linux
> distros anyway since they are all systemd-based and there is already
> precedence...
> xfpm-pm-helper should probably disabled by default on Linux since distros
> either carry upower < 0.99 or they have upower >= 0.99 and systemd.

Gentoo will provide UPower >= 0.99 by default for both, OpenRC (non-systemd) and systemd users. We have no plans in dropping the support for the sysvinit+OpenRC combination, but we plan to support systemd in parallel fully. We have no plans on retiring pm-utils either anytime soon.

Created attachment 5541
Suspend/hibernate upower 0.99 (no logind)

So this version makes upower optional but still uses the shutdown helper.

(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

Hmhm, so this one completely ignores logind? Is it really an improvement wrt. the runtime detection patch?

(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

A couple of notes:
- OpenBSD supports hibernation via /usr/sbin/ZZZ or /usr/sbin/apm -Z
- on FreeBSD you might want to call /usr/sbin/acpiconf -s 3 for suspend for consistency, /usr/sbin/zzz still supports APM but I doubt there is any APM-supporting hardware left
- on FreeBSD you can check for suspend/hibernate support by looking at the hw.acpi.supported_sleep_state property through the sysctl API, see sysctl(3)[1] and acpi(4)[2]

[1] http://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3&apropos=0&manpath=FreeBSD+10.0-stable
[2] http://www.freebsd.org/cgi/man.cgi?query=acpi&apropos=0&sektion=4&manpath=FreeBSD+10.0-stable&arch=default&format=html

Also the long term plan still is to use a DBus-actiavted helper instead of sudo? Where should this live, both xfce4-session and xfce4-power-manager need to make use of it but either one depending on the other would probably not be nice. People run Xfce without xfpm and and xfpm is used outside Xfce.

(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

The patch is still missing bunch of #ifdef UPOWER lines:

xfsm-upower.c:24:20: fatal error: upower.h: No such file or directory

And when you add #ifdef UPOWER around that #include <upower.h>, the same file still has code without #ifdef's:

xfsm-upower.c:304:22: error: missing binary operator before token "("

Please try the patch with a system that doesn't have upower headers installed, and with --disable-upower

Thanks!

Created attachment 5545
Suspend/hibernate upower 0.99 (no logind)

(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

Hmhm, so this one completely ignores logind? Is it really an improvement wrt. the runtime detection patch?
-------------

The hope is that this path will be used if the logind running check fails (i.e. FreeBSD, OpenBSD, Gentoo, Slackware, etc). So if you're using systemd this patch is nice but not useful.

------------
(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

A couple of notes:
- OpenBSD supports hibernation via /usr/sbin/ZZZ or /usr/sbin/apm -Z
- on FreeBSD you might want to call /usr/sbin/acpiconf -s 3 for suspend for consistency, /usr/sbin/zzz still supports APM but I doubt there is any APM-supporting hardware left
- on FreeBSD you can check for suspend/hibernate support by looking at the hw.acpi.supported_sleep_state property through the sysctl API, see sysctl(3)[1] and acpi(4)[2]

[1] http://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3&apropos=0&manpath=FreeBSD+10.0-stable
[2] http://www.freebsd.org/cgi/man.cgi?query=acpi&apropos=0&sektion=4&manpath=FreeBSD+10.0-stable&arch=default&format=html

Also the long term plan still is to use a DBus-actiavted helper instead of sudo? Where should this live, both xfce4-session and xfce4-power-manager need to make use of it but either one depending on the other would probably not be nice. People run Xfce without xfpm and and xfpm is used outside Xfce.
--------------

Thanks for all the info and links! I've hopefully got all that in there, but let me know if I've got it wrong.

My idea for the Dbus helper is actually to write a daemon to provide some of the logind API, as in: PowerOff, Reboot, Suspend, Hibernate, HybridSleep, the Can stuff (like CanPowerOff), Inhibit, and the PrepareForShutdown, PrepareForSleep signals. This way other applications outside of xfce can use it too and without having to adapt to yet another API since they'll be similar. But it may take me some time which is why I wrote this patch.

--------------
(In reply to Eric Koegel from comment #20)
> Created attachment 5541 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
> So this version makes upower optional but still uses the shutdown helper.

The patch is still missing bunch of #ifdef UPOWER lines:

xfsm-upower.c:24:20: fatal error: upower.h: No such file or directory

And when you add #ifdef UPOWER around that #include <upower.h>, the same file still has code without #ifdef's:

xfsm-upower.c:304:22: error: missing binary operator before token "("

Please try the patch with a system that doesn't have upower headers installed, and with --disable-upower

Thanks!
------------

Sorry! This new patch should be much better (it actually compiles with and without upower on everything the Jenkins instance currently supports).

(In reply to Eric Koegel from comment #24)
> Created attachment 5545 [details]
> Suspend/hibernate upower 0.99 (no logind)
>
>
> (In reply to Eric Koegel from comment #20)
> > Created attachment 5541 [details]
> > Suspend/hibernate upower 0.99 (no logind)
> >
> > So this version makes upower optional but still uses the shutdown helper.
>
> Hmhm, so this one completely ignores logind? Is it really an improvement
> wrt. the runtime detection patch?
> -------------
>
> The hope is that this path will be used if the logind running check fails
> (i.e. FreeBSD, OpenBSD, Gentoo, Slackware, etc). So if you're using systemd
> this patch is nice but not useful.

So it should be added on top of the logind runtime detection patch?

Pushed Guido's patch in:
http://git.xfce.org/xfce/xfce4-session/commit/?id=0bc5eb603a3ab4af930fac70e03cfe7aa63a399e

Then another two related patches in (for the pkexec fallback):
http://git.xfce.org/xfce/xfce4-session/commit/?id=29d87f559aadba51e7bd6afcf4aeeb615ecb2145

http://git.xfce.org/xfce/xfce4-session/commit/?id=b1ee368dcf54e4335b42f657839bfe6359af8326

Thanks for all the help, testing, and feedback with this.

I have to reopen because the pushed patch does not include proper support for systems with no systemd, but with UPower 0.99.0
The patch from Eric in this bug does it all, full support for both systemd and non-systemd systems, it's what we rely upon in Gentoo, for example
Otherwise Xfce wouldn't have Hibernate & Suspend at all for us by default

As in, Comment #24 has working patch

In xfsm_shutdown_try_suspend the logic is:
SystemD ? use LoginD suspend
Upower < 0.99 ? use Upower suspend
Otherwise lock the screen and call pkexec xfsm-shutdown-helper --suspend

http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.c#n312

Same for hibernate. I've been testing with a system that has consolekit + upower git master (it calls itself 0.99.2). Or is this for systems without polkit installed (or polkit-gobject-1 which is used for the fallback instead of sudo)? Just trying to figure out where it's gone wrong so we can fix it.

Does pkaction --action-id org.xfce.session.xfsm-shutdown-helper -v return anything?

(In reply to Eric Koegel from comment #29)
> In xfsm_shutdown_try_suspend the logic is:
> SystemD ? use LoginD suspend
> Upower < 0.99 ? use Upower suspend
> Otherwise lock the screen and call pkexec xfsm-shutdown-helper --suspend
>
> http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.
> c#n312
>
> Same for hibernate. I've been testing with a system that has consolekit +
> upower git master (it calls itself 0.99.2). Or is this for systems without
> polkit installed (or polkit-gobject-1 which is used for the fallback instead
> of sudo)? Just trying to figure out where it's gone wrong so we can fix it.
>
> Does pkaction --action-id org.xfce.session.xfsm-shutdown-helper -v return
> anything?

The target system where I *assumed* the committed patch wouldn't work is with active ConsoleKit session, working PolicyKit, no systemd, UPower 0.99.1 (or git, no real difference in this regard, as in, just UPower with no hibernate and suspend capabilities) -- a system, where I expect Hibernate and Suspend buttons to do direct pm-* command calls like the patch in Comment #24 does

So, sorry, I didn't actually had chance to test it out yet, but I just can't see the code that calls pm-suspend anywhere

(In reply to Samuli Suominen from comment #30)
> (In reply to Eric Koegel from comment #29)
> > In xfsm_shutdown_try_suspend the logic is:
> > SystemD ? use LoginD suspend
> > Upower < 0.99 ? use Upower suspend
> > Otherwise lock the screen and call pkexec xfsm-shutdown-helper --suspend
> >
> > http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.
> > c#n312
> >
> > Same for hibernate. I've been testing with a system that has consolekit +
> > upower git master (it calls itself 0.99.2). Or is this for systems without
> > polkit installed (or polkit-gobject-1 which is used for the fallback instead
> > of sudo)? Just trying to figure out where it's gone wrong so we can fix it.
> >
> > Does pkaction --action-id org.xfce.session.xfsm-shutdown-helper -v return
> > anything?
>
> The target system where I *assumed* the committed patch wouldn't work is
> with active ConsoleKit session, working PolicyKit, no systemd, UPower 0.99.1
> (or git, no real difference in this regard, as in, just UPower with no
> hibernate and suspend capabilities) -- a system, where I expect Hibernate
> and Suspend buttons to do direct pm-* command calls like the patch in
> Comment #24 does
>
> So, sorry, I didn't actually had chance to test it out yet, but I just can't
> see the code that calls pm-suspend anywhere

Well for that case:
xfsm_shutdown_try_suspend
http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.c#n312
  xfsm_upower_lock_screen (which just ends up calling xflock4 and ignoring the other stuff)
http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-upower.c#n289
  xfsm_shutdown_fallback_try_action XFSM_SHUTDOWN_SUSPEND
http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.c#n242
    pkexec " XFSM_SHUTDOWN_HELPER_CMD " --%s", action

http://git.xfce.org/xfce/xfce4-session/tree/xfsm-shutdown-helper/main.c#n86
#define UP_BACKEND_SUSPEND_COMMAND "/usr/sbin/pm-suspend"
#define UP_BACKEND_HIBERNATE_COMMAND "/usr/sbin/pm-hibernate"
Which gets used in:
http://git.xfce.org/xfce/xfce4-session/tree/xfsm-shutdown-helper/main.c#n218

That is assuming your PC can suspend and you are authorized to suspend,
http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.c#n577
http://git.xfce.org/xfce/xfce4-session/tree/xfce4-session/xfsm-shutdown.c#n667

Once everyone has migrated to UPower 1.0 we can actually remove all the upower stuff from xfsm since it doesn't do anything anymore. But that could be several years down the road.

Sorry, totally my bad, I should have investigated futher. This new code setup looks nicely organized. Thanks!

I'll give it proper testing, meanwhile, sorry for reopening...