Bug 72925 - Use systemd for dbus activation
Summary: Use systemd for dbus activation
Status: RESOLVED FIXED
Alias: None
Product: GeoClue
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Geoclue Bugs
QA Contact: Geoclue Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-20 16:51 UTC by Colin Guthrie
Modified: 2014-01-08 14:11 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Add systemd bus activation (2.88 KB, patch)
2013-12-20 16:54 UTC, Colin Guthrie
Details | Splinter Review
Improved commit message (3.18 KB, patch)
2013-12-22 14:55 UTC, Colin Guthrie
Details | Splinter Review
Same as previous patch, but with the service unit called geoclue2 (3.17 KB, patch)
2013-12-22 14:56 UTC, Colin Guthrie
Details | Splinter Review
Same as previous patch, but with the service unit called geoclue2 (3.18 KB, patch)
2013-12-22 14:57 UTC, Colin Guthrie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Guthrie 2013-12-20 16:51:36 UTC
When launching via dbus activation, the current service files will run geoclue under the same cgroup as the dbus daemon itself.

We should use systemd for dbus activation.

Patch will follow.
Comment 1 Colin Guthrie 2013-12-20 16:54:34 UTC
Created attachment 91052 [details] [review]
Add systemd bus activation
Comment 2 Zeeshan Ali 2013-12-20 17:40:48 UTC
Comment on attachment 91052 [details] [review]
Add systemd bus activation

Review of attachment 91052 [details] [review]:
-----------------------------------------------------------------

Some more details in the commit log please.

::: data/geoclue.service.in
@@ +1,1 @@
> +[Unit]

* Its a pitty if we need to provide two separate .service files. :(
* To differentiate from existing service file, can we name it 'geoclue2-systemd.service'?
Comment 3 Colin Guthrie 2013-12-21 13:08:26 UTC
(In reply to comment #2)
> Comment on attachment 91052 [details] [review] [review]
> Add systemd bus activation
> 
> Review of attachment 91052 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Some more details in the commit log please.
> 
> ::: data/geoclue.service.in
> @@ +1,1 @@
> > +[Unit]
> 
> * Its a pitty if we need to provide two separate .service files. :(
> * To differentiate from existing service file, can we name it
> 'geoclue2-systemd.service'?

Well, I just copied the layout used in udisks to be honest.

Calling the file something other than the name it's installed as will require a more complex Makefile magic. I can do that but personally I don't really see the need and being at least consistent with other similar code in other projects and avoiding more complex Makefile (with the destination filename hidden therein) is probably preferable.

Not sure what more info you want in the commit message. Would something like this do? (It's kinda generic info about "what systemd does" which is why I didn't include it in my original commit message):

Add support for bus-activation via systemd service unit

On systemd systems it is preferred to use systemd to launch services in order to properly track processes and allocate system resources. When dbus launches binary daemons internally they will stay in the dbus cgroup and thus proper resource allocation and process tracking is not possible.
Comment 4 Zeeshan Ali 2013-12-22 13:51:40 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 91052 [details] [review] [review] [review]
> > Add systemd bus activation
> > 
> > Review of attachment 91052 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Some more details in the commit log please.
> > 
> > ::: data/geoclue.service.in
> > @@ +1,1 @@
> > > +[Unit]
> > 
> > * Its a pitty if we need to provide two separate .service files. :(
> > * To differentiate from existing service file, can we name it
> > 'geoclue2-systemd.service'?
> 
> Well, I just copied the layout used in udisks to be honest.
> 
> Calling the file something other than the name it's installed as will
> require a more complex Makefile magic.

I didn't think of installing w/ a different name but just renaming the file, both in source tree and install location. Its bad already that systemd is making us install two .service files, would be good to avoid confusion as much as possible.

 
> Not sure what more info you want in the commit message.

Information about why this change is needed. :)

> Would something like
> this do? (It's kinda generic info about "what systemd does" which is why I
> didn't include it in my original commit message):
> 
> Add support for bus-activation via systemd service unit
> 
> On systemd systems it is preferred to use systemd to launch services in
> order to properly track processes and allocate system resources. When dbus
> launches binary daemons internally they will stay in the dbus cgroup and
> thus proper resource allocation and process tracking is not possible.

Sounds good.
Comment 5 Colin Guthrie 2013-12-22 13:59:50 UTC
(In reply to comment #4)
> > Calling the file something other than the name it's installed as will
> > require a more complex Makefile magic.
> 
> I didn't think of installing w/ a different name but just renaming the file,
> both in source tree and install location. Its bad already that systemd is
> making us install two .service files, would be good to avoid confusion as
> much as possible.

Naming an installed systemd unit: /usr/lib/systemd/system/geoclue2-systemd.service would be absolutely horrible :)

It also means that people would have to run "systemctl status geoclue2-systemd" to get status information and "journalctl -u geoclue2-systemd" to view logs. Of all the systemd units I've ever seen, none have been installed with the "-systemd" suffix in their name. I suspect Lennart would mock you relentlessly if you go down this route ;)

Longer term the whole kdbus thing will change things even more. I suspect you'll need to ship two units for that in systemd - one bus unit and one service unit - with the bus unit doing much the same as the current dbus unit. That said I'm not fully read up on that side of things yet.
Comment 6 Zeeshan Ali 2013-12-22 14:38:50 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > > Calling the file something other than the name it's installed as will
> > > require a more complex Makefile magic.
> > 
> > I didn't think of installing w/ a different name but just renaming the file,
> > both in source tree and install location. Its bad already that systemd is
> > making us install two .service files, would be good to avoid confusion as
> > much as possible.
> 
> Naming an installed systemd unit:
> /usr/lib/systemd/system/geoclue2-systemd.service would be absolutely
> horrible :)
> 
> It also means that people would have to run "systemctl status
> geoclue2-systemd" to get status information and "journalctl -u
> geoclue2-systemd" to view logs.

Gah! didn't know of this. Well I just hope that nobody adds a similar service to some old geoclue package but even if they do, they are really on their own.

> Of all the systemd units I've ever seen,
> none have been installed with the "-systemd" suffix in their name. I suspect
> Lennart would mock you relentlessly if you go down this route ;)

Ouch! We don't want that, do we? :)

> Longer term the whole kdbus thing will change things even more. I suspect
> you'll need to ship two units for that in systemd - one bus unit and one
> service unit - with the bus unit doing much the same as the current dbus
> unit. That said I'm not fully read up on that side of things yet.

Hopefully not with the same name. :)

Anyways, ACK with the log updated.
Comment 7 Colin Guthrie 2013-12-22 14:55:49 UTC
Created attachment 91126 [details] [review]
Improved commit message

This improves the commit message but keeps the unit called geoclue.service on the systemd side.

I'll attach an alternative patch in a moment that calls it geoclue2.service, so pick whichever you prefer! (personally, I'd go for this one as keeping the version number in the service name is annoying IMO, fine for dbus paths/endpoints for API reasons of course, but for systemd units I think it can be avoided - but it's your call!)
Comment 8 Colin Guthrie 2013-12-22 14:56:17 UTC
Created attachment 91127 [details] [review]
Same as previous patch, but with the service unit called geoclue2
Comment 9 Colin Guthrie 2013-12-22 14:57:48 UTC
Created attachment 91128 [details] [review]
Same as previous patch, but with the service unit called geoclue2

Oops, git mv fail. This is a better one!
Comment 10 Zeeshan Ali 2013-12-22 17:44:26 UTC
(In reply to comment #7)
> Created attachment 91126 [details] [review] [review]
> Improved commit message
> 
> This improves the commit message but keeps the unit called geoclue.service
> on the systemd side.
> 
> I'll attach an alternative patch in a moment that calls it geoclue2.service,
> so pick whichever you prefer! (personally, I'd go for this one as keeping
> the version number in the service name is annoying IMO, fine for dbus
> paths/endpoints for API reasons of course, but for systemd units I think it
> can be avoided - but it's your call!)

I agree and hence the reason I ack'ed your first patch already. :)
Comment 11 Zeeshan Ali 2013-12-22 17:45:21 UTC
Comment on attachment 91126 [details] [review]
Improved commit message

Review of attachment 91126 [details] [review]:
-----------------------------------------------------------------

ack
Comment 12 Bastien Nocera 2014-01-08 13:57:10 UTC
What's the status of the patches? Zeeshan, only the two of us can commit to geoclue2, so you should commit those patches if you can.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.