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.
Created attachment 91052 [details] [review] Add systemd bus activation
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'?
(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.
(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.
(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.
(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.
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!)
Created attachment 91127 [details] [review] Same as previous patch, but with the service unit called geoclue2
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!
(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 on attachment 91126 [details] [review] Improved commit message Review of attachment 91126 [details] [review]: ----------------------------------------------------------------- ack
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.