Work on Systemd service unit by Anton-Latukha · Pull Request #6806 · qbittorrent/qBittorrent

Anton-Latukha · 2017-05-18T14:58:20Z

On Ubuntu qBittorrent-nox comes without service. Because service unit was in bad state.

qBittorrent-nox should provide a good service, because its purpose is to run as a daemon, or, better, a service on server.

Without service, on every restart user needs to start daemon manually, or make service by himself.

Pull request does work adding a reliable service.

Possibility to pick user:

systemctl start qbittorrent-nox@user.service
systemctl enable qbittorrent-nox@user.service

On why old service removed

Old-one service unit had issues:

hardcoded to 1000 user
demonise and Type=forking is not a proper way to build Systemd service, because Systemd loose control over that process and it's stdout (logs).

Also old service on change become redundant, so it was removed.

Packaging

Maintainers know what to do in this situation.
If link /etc/systemd/system/multi-user.target.wants/qbittorrent-nox.service exists - package manager instruction need to leave that old file on hard drive. If link not exists - service was not enabled, so it is safe to remove old unit file.

If someone has it working on old service unit - it is going to continue to work.
On updates new service file is going to be added.
On new installations - only new service file is going to be added.

Placing new service file

Corresponding to systemd standards, there are no segregation on { @ : %i } feature for units, they placed just like other ones. In other words this new service file goes in the same directory as previous one. So only minor renaming in cmake was needed.

Anton-Latukha · 2017-05-18T16:47:32Z

I don't use any.

It is not my agenda, I don't have those systems and I have no applications for that. Most of users is going to be satisfied with auto-start.

Create Windows service is not so hard, I used to do it couple of times years ago. To do SNMP and active-agent monitoring on hosts.

There is a number of tools.

I think I used this one: 1. Because it is in default Windows installation.

And call execution as an Administrator. 2

Also there is other similar utilities: 3, 4

For other way, is to use programming languages. Like C++: main callback if binary is not service capable.

zeule · 2017-05-18T17:13:41Z

Since web UI port can be set via config file (and as such via web UI itself), this breaks the POLA principle.

zeule · 2017-05-18T17:16:30Z

You have to make changes to configure.ac as well.

Anton-Latukha · 2017-05-18T17:17:33Z

Ok.

Understood.

This can be closed.

zeule · 2017-05-18T17:18:35Z

This can be closed.

Why?

Anton-Latukha · 2017-05-18T17:24:09Z

Ok.

Sorry, I'm not experienced, in pull requests workflow.

I'm going to look what is going on, and how to update my pull request to full extend.

Anton-Latukha · 2017-05-18T17:25:02Z

'Add more commits by pushing to the master branch on Anton-Latukha/qBittorrent.' Ok 👍

zeule · 2017-05-18T17:30:28Z

Create a new branch for the PR, based on master branch: $ git checkout master; git checkout -b my-branch-name
Make changes and commit them.
Create a PR.
When you are asked to make a correction, make a fixup commit, optionally using git --fixup option $ git commit --fixup=... and push new commits.
When PR is ready to be merged, squash your fixup commits: $ git rebase -i --autosquash master and force push the branch: $ git push --force

zeule · 2017-05-18T17:40:53Z

Changed all mentions of service to new file.

Good, thank you. ~~Now what about --webui-port option?~~

oops, you've done it too. Thanks!

Anton-Latukha · 2017-05-18T17:42:46Z

Done.

Now I know, do all commits and one push in this situations.
CI reacts on every push.

zeule · 2017-05-18T17:57:03Z

It's funny, but configure does not process qbittorrent-nox.service.in nor it does that in master. So I can not test this PR yet.

Anton-Latukha · 2017-05-18T18:14:43Z

I going to wait on test.

This service works for myself. But I am not knowledgeable how provisioning of it through code to package must happen.

I know only this: dist/unix/CMakeLists.txt

configure_file(systemd/qbittorrent-nox@.service.in ${CMAKE_CURRENT_BINARY_DIR}/qbittorrent-nox@.service @ONLY)

I don't know cmake or configure process, I thought it processes qbittorrent-nox.service.in to qbittorrent-nox@.service and moves ahead, and then maintainers work with those files.

Sorry, I am a DevOps )

zeule · 2017-05-18T18:24:21Z

It's funny, but configure does not process qbittorrent-nox.service.in nor it does that in master.

My bad, that was evening and two qBt copies on disk...

zeule · 2017-05-18T19:18:18Z

@qbittorrent/frequent-contributors, does anybody want to review?

zeule · 2017-05-18T19:20:41Z

@Anton-Latukha, please squash all your commits.

Chocobo1 · 2017-05-19T07:30:51Z

how about qBittorrent-nox service for user %I?
it's not a daemon anymore.

IMO, better be explicit, pls add back Type=simple

Chocobo1 · 2017-05-19T07:42:55Z

@Anton-Latukha
Have you tried running multiple qbt-nox instances at the same time? Does it work?

KingLucius · 2017-05-19T07:53:12Z

Sorry for hijacking the thread.
@evsh squash your fixup commits: $ git rebase -i --autosquash master
do I have to rebase against master or the first commit in the new feature branch?

My current approach,

create new feature branch
make then commit changes
when you ask for correction I commit the changes with "amend" option (which will squash but there is no return)
force push

zeule · 2017-05-19T08:55:39Z

@KingLucius, rebasing against master serves two purposes: you not only squash commits, but also resolve merge conflicts. So you need up-to-date master branch,

zeule · 2017-05-19T10:52:23Z

Have you tried running multiple qbt-nox instances at the same time? Does it work?

It is possible with an isolated /tmp/ dir. We can, actually, add PrivateTmp=true to the service file for that.

Chocobo1 · 2017-05-20T03:16:23Z

We can, actually, add PrivateTmp=true to the service file for that.

Actually, I think that is required in case of this PR.

zeule · 2017-05-20T09:10:48Z

But that would disable passing parameters to the running instance.

Chocobo1 · 2017-05-23T16:19:01Z

@Anton-Latukha
Is running multiple instance of qbt the target of this PR?

Anton-Latukha · 2017-06-07T16:43:02Z

@Chocobo1

I did source build of my fork and some testing:

This moments:

Service file is not placed in service directory on build.
Instances use the same port.
Instances not report, or error, when port already used.

Fully user-based service:
There's also a other possible implementation, as a clearly user service (great article on ArchWiki).

I have chosen to pull current one, root based, because it is more used and encountered, by me anyway. And it work-around a moment, when users start to run instances, - those port and other inconsistencies lays on the hands of OS root administrator. It is better something than nothing.

User-based service going to enable for use that now available switch (%i) for port, but it going to collude then with conf file setting. Then that needs to be resolved.

I have chosen a more simple solution.

P.S. If that moments are going to be addressed - we can think to do multi-user qB-nox.

Anton-Latukha · 2017-06-07T17:05:52Z

@evsh
I've made a commit squash, as you asked.

Chocobo1 · 2017-06-10T06:39:29Z

If I understand your reply correctly, qbt is not ready for multi-user usage, then this should be PrivateTmp=false for now.

And there is a typo in commit message: w to systemd service ... ??

PrivateTmp=true is a recommended secure option by Systemd team.
Since it doesn't interfere with work, it can be left as is.

But as you request, I'll remove it.

There is no typo, it is intuitive vocabulary I use in commits names, in Git or on wikis.

Like:
w - for 'write'
rw - for 'rewrite'
del - for 'deletions'
add
mv - for 'move'
sec - for 'section'
subsec - for 'subsection'
and so on.

So:

add subsec "anton" in sec "Draft" rw subsec "anton" mv subsec "anton" to sec "Name" w to subsec "anton" useful information about qBittorrent.

I don't know why something like this is not widely adopted, so I promote it by example.

I don't know why something like this is not widely adopted

My reaction is exactly why and honestly this is the first time I encounter this kind of shorthand...

@Chocobo1:

this is the first time I encounter this kind of shorthand...

Actually, that shorthand is pretty standard command-line syntax for defining file access permissions, and database variables.

That being said, it does make very little sense to use it in an otherwise conventional English sentence.

Actually, that shorthand is pretty standard command-line syntax for defining file access permissions, and database variables.

right... the shorthand usage depends on the context.
And my thoughts are clearly expressed in your second sentence.

But as you request, I'll remove it.

It's actually a bit more complex under the hood, if PrivateTmp=true you won't be able to add torrents to qbt via cmdline easily, I'm not sure users uses this extensively or not...
also: #6806 (comment)

Chocobo1 · 2017-06-10T06:40:30Z

I would prefer moving this line on top of User=

Anton-Latukha · 2017-06-12T19:42:42Z

I opened a bug report:

If WebUI port already used - qBittorrent-nox does nothing #6955

rm previous systemd unit service cmake now placing new file 'qbittorrent-nox@.service.in' systemd service now "Type=simple"

Anton-Latukha · 2017-06-12T20:08:12Z

If bug #6955 sorts-out, than probably we can use this service.

Or move and do real user-service.

Considering proper usage of:
XDG Basedir specification

$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.
$XDG_CACHE_HOME defines the base directory relative to which user specific non-essential data files should be stored.

So PrivateTmp - qBt-nox CLI on user-service is non an issue.

Anton-Latukha · 2017-06-12T20:19:46Z

@Chocobo1 Done what you being asking: set PrivateTmp=false and moved things around.

Chocobo1

LGTM

Chocobo1 · 2017-06-13T01:23:36Z

Considering proper usage of:
XDG Basedir specification

Thanks for the information! This might be a good idea...
AFAIK qbt isn't using $XDG_RUNTIME_DIR right now.

Anton-Latukha · 2017-06-15T16:03:05Z

Ok @Chocobo1. Thank you.

I remind about, that all XDG Basedir variables have established fallback.

For $XDG_RUNTIME_DIR it is not literally set in spec, but: '/run/user/'"$UID" must be default path on most distributions.

Chocobo1 · 2017-06-17T07:33:22Z

@evsh @sledgehammer999
Shall we merge it? IMO it's ready in its current form.

Chocobo1 · 2017-07-01T09:02:24Z

Thank you!

zeule reviewed May 18, 2017

View reviewed changes

zeule previously approved these changes May 18, 2017

View reviewed changes

Chocobo1 reviewed May 19, 2017

View reviewed changes

Anton-Latukha dismissed zeule’s stale review via 55df5f2 June 6, 2017 21:18

Anton-Latukha force-pushed the master branch 3 times, most recently from bdb3b59 to fbac98a Compare June 7, 2017 16:17

Anton-Latukha force-pushed the master branch from fbac98a to eaf045a Compare June 7, 2017 17:03

Chocobo1 reviewed Jun 10, 2017

View reviewed changes

Comment thread dist/unix/systemd/qbittorrent-nox@.service.in Outdated

Copy link
Copy Markdown

Member

Chocobo1 Jun 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer moving this line on top of User=

Anton-Latukha mentioned this pull request Jun 12, 2017

If port for WebUI already used by instance from other user - qBittorrent-nox doesn't react to that #6955

Closed

systemd service with user switch.

e00c96d

rm previous systemd unit service cmake now placing new file 'qbittorrent-nox@.service.in' systemd service now "Type=simple"

Anton-Latukha force-pushed the master branch from eaf045a to e00c96d Compare June 12, 2017 19:58

Chocobo1 approved these changes Jun 13, 2017

View reviewed changes

zeule approved these changes Jun 22, 2017

View reviewed changes

LordNyriox approved these changes Jun 22, 2017

View reviewed changes

Chocobo1 merged commit 8150805 into qbittorrent:master Jul 1, 2017

nevivurn mentioned this pull request Feb 19, 2024

nixos/qbittorrent: init NixOS/nixpkgs#287923

Merged

19 tasks

Uh oh!

Conversation

Anton-Latukha commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Possibility to pick user:

On why old service removed

Packaging

Placing new service file

Uh oh!

Anton-Latukha commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeule commented May 18, 2017

Uh oh!

Anton-Latukha commented May 18, 2017

Uh oh!

zeule commented May 18, 2017

Uh oh!

Anton-Latukha commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anton-Latukha commented May 18, 2017

Uh oh!

zeule commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeule commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anton-Latukha commented May 18, 2017

Uh oh!

zeule commented May 18, 2017

Uh oh!

Anton-Latukha commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeule commented May 18, 2017

Uh oh!

zeule commented May 18, 2017

Uh oh!

zeule commented May 18, 2017

Uh oh!

Chocobo1 May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chocobo1 commented May 19, 2017

Uh oh!

KingLucius commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeule commented May 19, 2017

Uh oh!

zeule commented May 19, 2017

Uh oh!

Chocobo1 commented May 20, 2017

Uh oh!

zeule commented May 20, 2017

Uh oh!

Chocobo1 commented May 23, 2017

Uh oh!

Anton-Latukha commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anton-Latukha commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anton-Latukha commented May 18, 2017 •

edited

Loading

Anton-Latukha commented May 18, 2017 •

edited

Loading

Anton-Latukha commented May 18, 2017 •

edited

Loading

zeule commented May 18, 2017 •

edited

Loading

zeule commented May 18, 2017 •

edited

Loading

Anton-Latukha commented May 18, 2017 •

edited

Loading

Chocobo1 May 19, 2017 •

edited

Loading

KingLucius commented May 19, 2017 •

edited

Loading

Anton-Latukha commented Jun 7, 2017 •

edited

Loading

Chocobo1 Jun 12, 2017 •

edited

Loading

Anton-Latukha commented Jun 12, 2017 •

edited

Loading

Anton-Latukha commented Jun 15, 2017 •

edited

Loading