[Kde-hardware-devel] Review Request 108407: systemd-login1 support for PowerDevil

[Dario Freddi]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108407/#review25458
-----------------------------------------------------------


Some more coming:


powerdevil/daemon/backends/upower/login1suspendjob.cpp
<http://git.reviewboard.kde.org/r/108407/#comment19481>

    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.



powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp
<http://git.reviewboard.kde.org/r/108407/#comment19477>

    This still gets created regardless, in fact making the check fail at line 116 and causing disasters in case the interface doesn't exist. You need to wrap this around an if(!isServiceRegistered) to make this all work.



powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp
<http://git.reviewboard.kde.org/r/108407/#comment19479>

    Giving this a second look, there's not much to do besides waitForFinished here, given the synchronous nature of that call.



powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp
<http://git.reviewboard.kde.org/r/108407/#comment19478>

    Sorry for the nitpicking, but usual code style issue here [if () {} else {}]


- Dario Freddi


On Jan. 14, 2013, 1: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, 1: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/87debd67/attachment-0001.html>