[Kde-hardware-devel] Review Request 108407: systemd-login1 support for PowerDevil
Lukáš Tinkl
lukas at kde.org
Mon Jan 14 14:23:46 UTC 2013
> On Jan. 14, 2013, 2:37 p.m., Dario Freddi wrote:
> > powerdevil/daemon/backends/upower/login1suspendjob.cpp, lines 66-77
> > <http://git.reviewboard.kde.org/r/108407/diff/3/?file=107206#file107206line66>
> >
> > This, instead, should be really handled in a fully asynchronous way. We had deadlocking problems in similar situations before, and I can see the same problems potentially arising here, especially because I don't know how the call is internally handled in systemd and when it returns.
> >
> > Moreover, giving it a second look, there is another thing: you are emitting the result without any kind of error checking. Is that intentional, as in: is there a way to check such a thing? And moreover: do you need to have the job return when the call finishes?
> >
> > This for two reasons: if you just need the job merely to start the call, then a KJob is definitely overkill, since the operation is already asynchronous through the bus. OTOH, if you need such a thing, you are processing the whole thing synchronously and not reporting a potential error, defying the purpose of a KJob at all. Depending on what you need to do, I'd rather either make the job truly async or not have it as a KJob at all.
> >
> > If you don't have a way to detect errors but need to have the job return when the operation ends, a mere connect(QDBusWatcher, SIGNAL(finished()), SLOT(emitFinished())) would do.
I think just emitting the signal will do fine, the login1 calls (Suspend, Hibernate, etc) return no value anyway
- Lukáš
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108407/#review25458
-----------------------------------------------------------
On Jan. 14, 2013, 2:22 p.m., Lukáš Tinkl wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108407/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2013, 2:22 p.m.)
>
>
> Review request for Solid and Dario Freddi.
>
>
> Description
> -------
>
> This patch adds support for systemd-login1 service to Powerdevil's upower backend. The main purpose is that UPower will be soon dropping support[1] for suspend/resume features so we have to rely on systemd. With this login1, we are also gaining support for HybridSleep, where implemented by the system.
>
> One caveat: the current login1 implementation doesn't support[2] emitting the "resume from suspend" signal
>
> [1] http://lists.freedesktop.org/archives/devkit-devel/2013-January/001339.html
> [2] http://www.freedesktop.org/wiki/Software/systemd/inhibit
>
>
> This addresses bug https://bugzilla.redhat.com/show_bug.cgi?id=859227.
> http://bugs.kde.org/show_bug.cgi?id=https://bugzilla.redhat.com/show_bug.cgi?id=859227
>
>
> Diffs
> -----
>
> powerdevil/daemon/BackendConfig.cmake 5dbe6f6
> powerdevil/daemon/backends/upower/login1suspendjob.h PRE-CREATION
> powerdevil/daemon/backends/upower/login1suspendjob.cpp PRE-CREATION
> powerdevil/daemon/backends/upower/powerdevilupowerbackend.h ba942bd
> powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp 97a409b
> powerdevil/daemon/backends/upower/upowersuspendjob.h bbe2f45
> powerdevil/daemon/backends/upower/upowersuspendjob.cpp fa64ab0
>
> Diff: http://git.reviewboard.kde.org/r/108407/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lukáš Tinkl
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130114/64bb0c10/attachment-0001.html>
More information about the Kde-hardware-devel
mailing list