DeviceKit-power and latency control

[Richard Hughes]

On Thu, 2008-11-06 at 12:26 -0500, David Zeuthen wrote:
> On Thu, 2008-11-06 at 16:47 +0000, Richard Hughes wrote:
> > I’ve put an interface file up here:
> > http://people.freedesktop.org/~hughsient/DeviceKit-power/gtk-doc/Latency.html
> 
> Some comments
> 
> 1. Regarding GetRequests() returning a(ussi), e.g. an array of
> 
>     - pid, connection, type, value
> 
> why is the connection necessary?

If more than one process has two different requests -- mainly so the
application didn't have to track it's own requests. For instance, you
could have one request from two different GObjects. Think of it like a
cookie.

> That should probably go. The docs
> should also tell about the envisioned usage, e.g. "an application can
> use this to show a dialog of active applications with outstanding
> latency requests". I'm guessing that is the intended usage (am I
> right?), e.g. purely informational purposes (as opposed to controlling
> stuff).

Yes, it's just for a admin UI.

> For that you want a bit more than information actually, you need
> something like this
> 
> http://hal.freedesktop.org/docs/DeviceKit-disks/Device.html#Device.FilesystemListOpenFiles

With the PID, can't we get all this other stuff?

> that uses more suitable names / icons for representing the applications
> with active outstanding requests. You probably also want to lock this
> method down using PolicyKit and default to granting the authorization to
> active local sessions.

You think we need to lock down this method? Surely anyone can read /proc
and get essentially the same data.

> 2. nitpick; Don't use abbrev for things like Minimum; e.g. SetMinLatency
> -> SetMinimumLatency

Cool, agreed. It should probably be called SetMinimum anyway.

> 3. SetLatency should probably be called RequestLatencyGuarantee since
> that is what it's doing? Or maybe a better name... but Set* sounds wrong
> since you're not setting anything; you are making a request that the
> latency is at least XYZ (it may already be that, another client may have
> an even harder request outstanding).

Agreed. "Request" sounds better, as it's already on the .Latency
interface, and just by asking doesn't mean you'll get it if the admin
has set an override.

> If a client disconnects from the bus, will the request be removed? I
> definitely think it should (you want to listen to NameOwnerChanged), we
> need to document this in the docs for SetLatency + friends.

Yes. I'll add this to the docs.

> 4. What's the difference between SetLatency() and SetMinLatency()
> anyway? When would I use on over the other?

The latter would be set by an admin to over-ride the latency request for
applications.

> 5. How do I undo a latency request? Probably the Request* methods needs
> to return a cookie and we need a CancelLatencyRequest() method to cancel
> it that you pass the obtained cookie in.

I figured the connection would be disconnected or the client could do
Request(-1). If this is done per-connection, I don't think we need to
return a cookie.

> 6. Should we return the cookie in the GetRequests() thus allowing other
> clients to cancel a request? Maybe

I hadn't thought of that. That's probably the exact sort of thing you
would do on the admin interface. Returning a proper cookie would allow
us to do that.

> 7. GetLatency() is probably the right name since it will be computed
> from the set of outstanding requests. What does it return if there are
> no outstanding requests?

I guess -1, although that needs to be in the docs.

> 8. I see that you have a separate polkit action for each method call.
> That's probably wrong (and confusing for admins), I think you just need
> a single action for everything, e.g. something like
> 
>  org.freedesktop.devicekit.power.configure-latency

No, I think one method to be used for users (for instance to be used on
the local session without a password) and one for admins (to cancel
other requests and to set a minimum and maximum latency).

> 9. Why is there a separate error name space (with a single error
> org.freedesktop.DeviceKit.Power.Latency.GeneralError) for this? Also, I
> note that for the Device interface we have
> 
>   org.freedesktop.DeviceKit.Power.Device.GeneralError
> 
> and for the Power interface we have none (I believe this is partly my
> fault). Suggest to have a single error name space for the entire project
> and s/GeneralError/Failed/, e.g. we'll have the name space
> 
>   org.freedesktop.DeviceKit.Power.Error
> 
> with (probably) only a single error
> 
>   org.freedesktop.DeviceKit.Power.Error.Failed
> 
> Doing this will make it much easier to map to e.g. GObject on the client
> since you'll only a single Error enumeration. Makes sense?

Yes, I'll fix this, thanks. Cheers for the review, I appreciate it.

Richard.