spk/rutorrent: fix startup issues #4234
Conversation
| "gzip" => '', // Something like /usr/bin/gzip. If empty, will be found in PATH. | ||
| "id" => '', // Something like /usr/bin/id. If empty, will be found in PATH. | ||
| "stat" => '', // Something like /usr/bin/stat. If empty, will be found in PATH. | ||
| + "python"=> '', // Something like /usr/bin/python3. If empty, will be found in PATH |
There was a problem hiding this comment.
This is to be able to define explicitely the location where to look for python binary.
| "id" => '', // Something like /usr/bin/id. If empty, will be found in PATH. | ||
| "stat" => '', // Something like /usr/bin/stat. If empty, will be found in PATH. | ||
| + "python"=> '', // Something like /usr/bin/python3. If empty, will be found in PATH | ||
| + "pgrep" => '', // Something like /usr/bin/pgrep. If empty, will be found in PATH |
There was a problem hiding this comment.
This is to be able to define explicitely the location where to look for pgrep binary.
spk/rutorrent/Makefile
Outdated
| @@ -34,7 +41,7 @@ INSTALL_PREFIX = /usr/local/$(SPK_NAME) | |||
|
|
|||
| POST_STRIP_TARGET = rutorrent_extra_install | |||
|
|
|||
| BUSYBOX_CONFIG = usrmng daemon nice | |||
| BUSYBOX_CONFIG = usrmng daemon nice procutils | |||
There was a problem hiding this comment.
This enables to ship pgrep binary.
| @@ -5,6 +5,6 @@ schedule = watch_directory,5,5,load_start=@watch_dir@/*.torrent | |||
| max_memory_usage = @max_memory@ | |||
| log.open_file = "rtorrent.log", "/usr/local/rutorrent/var/rtorrent.log" | |||
| log.add_output = "warn", "rtorrent.log" | |||
| http_cacert = /etc/ssl/certs/ca-certificates.crt | |||
| network.http.cacert = /etc/ssl/certs/ca-certificates.crt | |||
There was a problem hiding this comment.
Since rtorrent upgrade this is the new parameter name.
Note: this means that current rutorrent ui won't be able to change that value (it may even break the configuration to be honest but I didn't take the time to check this)
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "php56": { | |||
There was a problem hiding this comment.
Officially rutorrent seems to be PHP 5.6 only... Didn't bother adding other PHP versions in case it wouldn't work. I'll leave that to some other user who would be interested in testing it ;)
spk/rutorrent/src/installer.sh
Outdated
| @@ -147,6 +183,22 @@ postinst () | |||
| chown -R ${USER}:${APACHE_USER} ${INSTALL_DIR}/tmp | |||
| chown -R ${APACHE_USER}:${APACHE_USER} ${WEB_DIR}/${PACKAGE} | |||
|
|
|||
| # Fix rights using ACL (so that test.sh remains executable) | |||
| if [ "${BUILDNUMBER}" -ge "7321" ]; then | |||
| set_acl_full_permissions "${WEB_DIR}/${PACKAGE}" "user:${USER}" | |||
There was a problem hiding this comment.
Ok, so basically the issue here was that rutorrent (running with user http) relies on rtorrent (running with user sc-rutorrent) to execute a file under php/test.sh to detect / valid external programs.
So here we basically need to ensure that (following rutorrent official documentation) web installation directory is accessible to sc-rutorrent with full rights.
And, unfortunately, it seems that for Synology it's either you're going all in on ACLs or you're using standard Linux rights. So I needed to give full acl permissions to sc-rutorrent and web user / groups.
spk/rutorrent/src/installer.sh
Outdated
| @@ -207,24 +259,67 @@ preupgrade () | |||
| rm -fr ${TMP_DIR}/${PACKAGE} | |||
| mkdir -p ${TMP_DIR}/${PACKAGE} | |||
| mv ${WEB_DIR}/${PACKAGE}/conf/config.php ${TMP_DIR}/${PACKAGE}/ | |||
| if [ -f "${WEB_DIR}/${PACKAGE}/.htaccess" ]; then | |||
There was a problem hiding this comment.
If someone defines a .htaccess to configure authentication you'd better not lose it when upgrading :)
|
Flagging this PR as a draft one since installation from scratch seems to be facing issues with default user configuration (e.g. it prompts for an "Unavailable" message and in the logs I find |
Same error occurring when trying to update. At this stage I guess I'd better migrate this package to a standard package layout in order to minimize the burden... |
spk/rutorrent/Makefile
Outdated
| CHANGELOG ="1. Update rutorrent to 3.10<br/>2. Update to rtorrent 0.9.8<br/>3. Update to libtorrent 0.13.8<br/>4. Update to xmlrpc-c 1.51.06<br/>5. Update openssl to 1.1" | ||
| CHANGELOG = "1. Update rutorrent to 3.10<br/> | ||
| CHANGELOG += 2. Update to rtorrent 0.9.8<br/> | ||
| CHANGELOG += 3. Update to libtorrent 0.13.8<br/> | ||
| CHANGELOG += 4. Update to xmlrpc-c 1.51.06<br/> | ||
| CHANGELOG += 5. Update openssl to 1.1<br/> | ||
| CHANGELOG += 6. Embed cloudflare requirements (requires python 3)<br/> | ||
| CHANGELOG += 7. Embed pgrep (required by tasks)<br/> | ||
| CHANGELOG += 8. Fixed stat<br />" |
There was a problem hiding this comment.
Sorry but as this changelog targets end-user audience, I would go into so much details - whereas I am pleased to find in it commit message.
There was a problem hiding this comment.
I changed the changelog but I must confess I didn't find anything really satisfactory to put in there (fixing ? :/ )
spk/rutorrent/src/installer.sh
Outdated
| set_syno_permissions () | ||
| set_traverse_permissions () |
There was a problem hiding this comment.
This old beta code has been replaced by generic installer to avoid copy-paste pattern: https://github.com/SynoCommunity/spksrc/blob/master/mk/spksrc.service.installer#L145
Really it is worth move to generic installer:
There was a problem hiding this comment.
I moved to the generic installer.
By doing so I found a tiny bug though in the (very much used) set_syno_permissions function and I find this a little bit unsettling and weird... :/
There was a problem hiding this comment.
My bad about the tiny bug I had misunderstood the way it worked. A classic.
smaarn
force-pushed
the
fix/rutorrent/add-webstation-support
branch
from
f21c3a1
to
e135c17
Compare
|
@smaarn before I hard disk died I had barely looked into converting |
Not necessarily but the real concern here was about fixing the overall permission logic issue (I may have identified the issue but it seemed to be a little bit more complicated than this and migrating to using service-setup seemed like a good way to simplify the thing). |
smaarn
force-pushed
the
fix/rutorrent/add-webstation-support
branch
from
e135c17
to
2a79ce9
Compare
spk/rutorrent/Makefile
Outdated
| CHANGELOG += 3. Update to libtorrent 0.13.8<br/> | ||
| CHANGELOG += 4. Update to xmlrpc-c 1.51.06<br/> | ||
| CHANGELOG += 5. Update openssl to 1.1<br/> | ||
| CHANGELOG += 6. Embed missing binary requirements<br/>" |
There was a problem hiding this comment.
Couldn't find a better changelog entry I must confess :|
There was a problem hiding this comment.
Such details have a right place in a commit message but means nothing for end-users.
Please mind target audience and simplify to only "Update to version 3.10" (for instance)
spk/rutorrent/src/service-setup.sh
Outdated
| # Sets recursive read / execute permissions for ${KEY} on specified directory | ||
| # Usage: grant_basic_permissions "${SHARE_FOLDER}" "user:<user>" | ||
| # Usage: grant_basic_permissions "${SHARE_FOLDER}" "group:<group>" | ||
| grant_basic_permissions () |
There was a problem hiding this comment.
I needed this method because I'm in case where I need specific locations to be available for the web user to execute code... And I didn't want the apache user to be allowed to do anything in those directories.
|
At this stage:
The only thing I would which was weird was that when adding a torrent it would always tell me there was an error when it was actually successfully added - though not started - and visible in the web ui, this may totally be due to misconfiguration on my side to be honest. |
|
I'll give it a shot and see if the same behavior hapens on my side. |
|
Compiled and installed like a charm. Really awesome work @smaarn Issues found so far (haven't yet investigated them):
But it actually put the torrent to STOP with a success message: There is some info on this on archlinux wiki:
Hints are that this may be caused by a dysfunctional plugin... |
|
Made a wild attempt by enforcing explicit binary paths... not really sure it will solve anything to be honest but it seemed to be me that curl misconfiguration could actually explain the root cause for failures in fetching files. |
|
Having a look it seems that there are lots of plugins which seem to be using shell scripts which are supposed to be invoked by rtorrent remotely... and obviously the ACL rights are not appropriately configured... I may have an option here. |
|
From what I see the "downloading of the torrent from an URL" part is actually not handled by rtorrent but by some internal code of the ruTorrent wrapper. At the same time there are authorizations which need being fixed so that rutorrent binary has at least appropriate rights on plugin shell executables when relevant. |
|
Great work! Event failures looks like solved. |
|
Suggesting to add the following into the configuration file: |
|
I have new hints related to the failure to download torrent from URL. |
smaarn
force-pushed
the
fix/rutorrent/add-webstation-support
branch
from
198fbcf
to
424b4b1
Compare
|
Ok I did a rebase with several squashing to avoid the "back and forth". @th0ma7 I'm afraid the test case isn't exactly that. It's more about the following command (this is a mock code I need to test it to be honest !): That's basically what's causing the issue (code can be read from EDIT (1 and 2): corrected the command to execute |
Just re-read the code and it seems to be using curl under the hood. Need to check this. |
Ok just found the issue... the temp directory path wasn't ending with a slash... :'( |
smaarn
force-pushed
the
fix/rutorrent/add-webstation-support
branch
from
2e9b1c5
to
cf35fd5
Compare
|
More into the "make it clean" phase: use normalized service command workflow + removed custom service privilege thing (which may actually be messing up several thing to be honest). It seems to still work on my end, any better luck on your end ? |
|
Working like a charm. I haven't tested all functionalities but at first glance it rocks! Note that directory permissions are still the same (e.g. 0777) like my previous comment. I've tried updating, uninstall/reinstall, all working great but directory permissions stays odd on my side. |
|
Ok, finally have my PC back... And the directory permissions are being set as followed: Reading the For comparison purpose I gave it a shot at installing How can I run the installer script manually? I could set -x and try to confirm where the issue is. |
|
This is really odd (and its getting late)... In trying to figure out where the issue is I commented out the following lines in hope to un-comment and confirm the change of behavior: The directory end-result is the following (note the no ACL And in |
Just confirmed: it works very well without the lines you removed. The only bits which were actually required were the share and tmp. Come to think of it I actually even wonder if the share directory needs also to be fixed... |
Ok so it turns out that |
Directory permissions where first set using Linux type and after superseeded with ACL. Permissions on http user was too wide leading the ability to the application to change ownership to default 0777. Following changes only uses ACL and reduce permissions to http user and group to only what it needs to work.
|
I ended playing a bit with
After a first torrent download the I've now restrained the ACL of both Now all looks good, and stays good. |
|
@smaarn I now believe the package is good for a round of review so I've also asked @hgy59 and @ymartin59 to have a look at it. |
spk/rutorrent/Makefile
Outdated
| CHANGELOG += 3. Update to libtorrent 0.13.8<br/> | ||
| CHANGELOG += 4. Update to xmlrpc-c 1.51.06<br/> | ||
| CHANGELOG += 5. Update openssl to 1.1<br/> | ||
| CHANGELOG += 6. Embed missing binary requirements<br/>" |
There was a problem hiding this comment.
Such details have a right place in a commit message but means nothing for end-users.
Please mind target audience and simplify to only "Update to version 3.10" (for instance)
|
I've reviewed your latest changes and besides the changelog numbering that started at @ymartin59 and/or @smaarn is this PR good for a merge (presuming testing is good)? If so I'll take care of merging + pushing up updated packages + closing all relevant pending issues. |
|
Nothing to add / change or my side. Thanks a lot for the help ! |
Pre-allocating the download file reduces the overhead when validating the hash post-download resulting in a better behavior of the NAS (reduces halt of other processes du to IO wait on large files). Functionality is compatible with xfs, ext4, btrfs and ocfs2 as such compatible with Synology filesystems.
Thnx, will squash & merge shortly then after I finish testing (added pre-allocation configuration option to reduce NAS overhead when checking large file hash). |
|
Thank you for your work and dedication. I installed the package on DS918+. The package runs, but I'm getting errors in rutorrent's log tab. I used rutorrent.v9.f15047[apollolake-avoton-braswell-broadwell-broadwellnk-bromolow-cedarview-denverton-dockerx64-geminilake-grantley-purley-kvmx64-v1000-x86-x86_64].spk [07.12.2020 20:25:02] WebUI started. |
|
@defaultsecurity Thnx for your feedback. Can you please create an new issue so we can track that independently from this closed PR? |
rtorrent upgrade followup: fixed invalid configuration clause for certificate path
Added: missing pgrep commandline tool (used by _tasks plugin)
Added: missing sox commandline tool
Added: php workers configuration (for webstation integration)
Fixed: automatic detection of binaries was broken because of rights on php/test.sh script
Added: if python 3 installation is present also install cloudscraper wheel (required for _cloudflare plugin)
Fixed: former configuration file was always lost upon upgrade
Fixed: .htaccess file defined at root level wasn't backed up
Fixed: permissions were lost on the tmp directory (which needed to be accessible to both web and rutorrent user)
Motivation: Having rutorrent back operational
Linked issues: Should tackle #4227
Checklist
all-supportedcompleted successfullyPending issues
Download event action failed: Bad return code.Event 'event.download.erased' failed: Bad return code.httpsURL:Failed to add torrent. Can't retrieve URL.Scheduled command failed: watch_directory: Command "load_start" does not exist.Optional
Add built-in support for- issue [REQUEST] autodl-irssi for rutorrent #2221autodl-rutorrent