[PATCH] services: transmission: Create downloads directory.

  • Open
  • quality assurance status badge
Details
3 participants
  • Morgan.J.Smith
  • Efraim Flashner
  • Simon South
Owner
unassigned
Submitted by
Morgan.J.Smith
Severity
normal
M
M
Morgan.J.Smith wrote on 17 Jul 2021 02:28
(address . guix-patches@gnu.org)(name . Morgan Smith)(address . Morgan.J.Smith@outlook.com)
BYAPR05MB40231D4388D70966EC26C79CC5109@BYAPR05MB4023.namprd05.prod.outlook.com
From: Morgan Smith <Morgan.J.Smith@outlook.com>

* gnu/services/file-sharing.scm (transmission-daemon-activation): Create
downloads directory.

This fixes a bug where transmission doesn't have permission to create it's
download directory. This bug occurs when download-dir is configured to a
location where the transmission user doesn't have permission to create a directory.
---
gnu/services/file-sharing.scm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

Toggle diff (24 lines)
diff --git a/gnu/services/file-sharing.scm b/gnu/services/file-sharing.scm
index 72cd6478d6..143fd3ed75 100644
--- a/gnu/services/file-sharing.scm
+++ b/gnu/services/file-sharing.scm
@@ -746,6 +746,7 @@ produces a Transmission settings file (@file{settings.json}) matching CONFIG."
(define (transmission-daemon-activation config)
"Return the Transmission Daemon activation GEXP for CONFIG."
(let ((config-dir %transmission-daemon-configuration-directory)
+ (download-dir (transmission-daemon-configuration-download-dir config))
(incomplete-dir-enabled
(transmission-daemon-configuration-incomplete-dir-enabled? config))
(incomplete-dir
@@ -769,7 +770,8 @@ produces a Transmission settings file (@file{settings.json}) matching CONFIG."
(for-each (lambda (directory-specification)
(apply mkdir-p/perms directory-specification))
'(#$@(append
- `((,config-dir #o750))
+ `((,config-dir #o750)
+ (,download-dir #o755))
(if incomplete-dir-enabled
`((,incomplete-dir #o750))
'())
--
2.32.0
S
S
Simon South wrote on 17 Jul 2021 13:52
(address . Morgan.J.Smith@outlook.com)(address . 49601@debbugs.gnu.org)
87y2a5gnt6.fsf@simonsouth.net
Morgan.J.Smith@outlook.com writes:
Toggle quote (5 lines)
> This fixes a bug where transmission doesn't have permission to create
> it's download directory. This bug occurs when download-dir is
> configured to a location where the transmission user doesn't have
> permission to create a directory.

Morgan,

I recall originally having the service activation create the downloads
directory, as your change makes it do, but then removing this as there
was some common case where it made service activation fail.
Unfortunately, I've forgotten what that was.

Let me see if I can duplicate that and if so, perhaps we can find a
solution that will handle both cases. At any rate:

Toggle quote (2 lines)
> + (,download-dir #o755))

The folder permissions should be #o750, not #o755. The reasoning here is
that it ought to be possible to place limits on who can see and access
files being shared by other users.

--
Simon South
simon@simonsouth.net
S
S
Simon South wrote on 17 Jul 2021 19:47
(address . Morgan.J.Smith@outlook.com)(address . 49601@debbugs.gnu.org)
87v958g7dw.fsf@simonsouth.net
Simon South <simon@simonsouth.net> writes:
Toggle quote (2 lines)
> Let me see if I can duplicate that...

I haven't been able to duplicate whatever issue it was I saw originally.
Likely it had to do with the download directory being placed on a
separate volume and thus matching a mount point already declared in the
operating-system declaration. Probably the system test should be
expanded to check this kind of configuration works but a quick test on
my end shows that at least for existing setups, it is not a problem.

Morgan, just a few pieces of feedback then:

Toggle quote (3 lines)
> This fixes a bug where transmission doesn't have permission to create
> it's download directory. This bug occurs when download-dir is

"it's" should be "its" (no apostrophe) here, the possessive form.

Toggle quote (2 lines)
> + (,download-dir #o755))

Again, the permissions here should be "#o750".

Finally, I recommend updating the system test (in
gnu/tests/file-sharing.scm) to verify the download directory is created
correctly, now that this functionality has been added. I've included a
patch below that does this for you. To verify this works, apply the
patch and run the test suite with

make check-system TESTS="transmission-daemon"

The Guix manual has more information[0].

Otherwise, looks good to me. Thanks for submitting this patch.


--
Simon South
simon@simonsouth.net


Toggle diff (52 lines)
diff --git a/gnu/tests/file-sharing.scm b/gnu/tests/file-sharing.scm
index 9a8ee6a593..d27a206c4f 100644
--- a/gnu/tests/file-sharing.scm
+++ b/gnu/tests/file-sharing.scm
@@ -34,6 +34,8 @@
(define %transmission-daemon-group "transmission")
(define %transmission-daemon-config-dir "/var/lib/transmission-daemon")
+(define %transmission-daemon-download-dir
+ (string-append %transmission-daemon-config-dir "/downloads"))
(define %transmission-daemon-watch-dir
(string-append %transmission-daemon-config-dir "/watch"))
(define %transmission-daemon-incomplete-dir
@@ -110,8 +112,9 @@
#t)
marionette))
- ;; Make sure Transmission Daemon's configuration directory has been
- ;; created with the correct ownership and permissions.
+ ;; Make sure Transmission Daemon's configuration and download
+ ;; directories have been created with the correct ownership and
+ ;; permissions.
(test-assert "configuration directory exists"
(marionette-eval
'(eq? (stat:type (stat #$%transmission-daemon-config-dir))
@@ -132,6 +135,26 @@
#o750)
marionette))
+ (test-assert "download directory exists"
+ (marionette-eval
+ '(eq? (stat:type (stat #$%transmission-daemon-download-dir))
+ 'directory)
+ marionette))
+ (test-assert "download directory has correct ownership"
+ (marionette-eval
+ '(let ((download-dir (stat #$%transmission-daemon-download-dir))
+ (transmission-user (getpwnam #$%transmission-daemon-user)))
+ (and (eqv? (stat:uid download-dir)
+ (passwd:uid transmission-user))
+ (eqv? (stat:gid download-dir)
+ (passwd:gid transmission-user))))
+ marionette))
+ (test-assert "download directory has expected permissions"
+ (marionette-eval
+ '(eqv? (stat:perms (stat #$%transmission-daemon-download-dir))
+ #o750)
+ marionette))
+
;; Make sure the incomplete-downloads and watch directories have been
;; created with the correct ownership and permissions.
(test-assert "incomplete-downloads directory exists"
E
E
Efraim Flashner wrote on 19 Jul 2021 14:13
(name . Simon South)(address . simon@simonsouth.net)
YPVsUAU50+SKodCt@3900XT
On Sat, Jul 17, 2021 at 07:52:21AM -0400, Simon South wrote:
Toggle quote (23 lines)
> Morgan.J.Smith@outlook.com writes:
> > This fixes a bug where transmission doesn't have permission to create
> > it's download directory. This bug occurs when download-dir is
> > configured to a location where the transmission user doesn't have
> > permission to create a directory.
>
> Morgan,
>
> I recall originally having the service activation create the downloads
> directory, as your change makes it do, but then removing this as there
> was some common case where it made service activation fail.
> Unfortunately, I've forgotten what that was.
>
> Let me see if I can duplicate that and if so, perhaps we can find a
> solution that will handle both cases. At any rate:
>
> > + (,download-dir #o755))
>
> The folder permissions should be #o750, not #o755. The reasoning here is
> that it ought to be possible to place limits on who can see and access
> files being shared by other users.
>

It's possible it was mkdir vs mkdir-p

(ins)scheme@(guile-user)> (mkdir "tmp")
(ins)scheme@(guile-user)> (mkdir "tmp")
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure mkdir: File exists

Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue.
(ins)scheme@(guile-user) [1]> ,q
(ins)scheme@(guile-user)> (use-modules (guix build utils))
(ins)scheme@(guile-user)> (mkdir-p "tmp")
$1 = #t


--
Efraim Flashner <efraim@flashner.co.il> ????? ?????
GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted
-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEoov0DD5VE3JmLRT3Qarn3Mo9g1EFAmD1bFAACgkQQarn3Mo9
g1ESIxAAvwXa1H0F+0oxG1qsXm7kCX462HoHCg1g83WGn0K86wPgtUxZ4XK6f7GN
hxSYOdepLf40sIv7Q4CoeJa70B5BeNAnEqb+0IPgcnK1qwbYWXmUHSiRfkFdOEU/
1XG9Wjx7a5n8syas7kyaq6R/3AEA6buXCqdiGttH10nMsgb/V/Bl1r3tMTBbcxvD
X/M3lRm4EdvUTH3y7rh7TE1SJLNQ9g1Fh5LvnFCpBRh5ZZKtKYC+DBV0kMvZ9sBA
Ye2oDzSvAQp0AjfgikiLUlKeaFcqBYU4JL/FZLa756YU6qHIDgGdiWMpTzD1ez7o
MwYyIV8I52gVyicHKNq6z9Zg0mUpe2cOqPMA/KnUD+Um1/uThiTPuFOK6xElxpee
LHcPlrmZCe3bN5FeY6NOJfrKqL5/I/1Qj0yAeXR4VpHkxS4KdpM3t90WZmU65NNk
qTaqNpHbTK0Xexg0dWoQL3N4h6iCU29unmGBd6VqwOw5qrgAXa5AK/UYbhw7zEMG
XMUhkX13/a+obCeJGGF1g4+ySiNrfP4zXPoYAy3u4sDMOpTBj8eboosc8rM6WLHE
oiG0Rek3yI0I3/B3kViXdLos28ureyXmqovZ146sYnc/og4gOnhOYG+GkbQSV7s8
ZnvqviC9hjYPd/a7Y45fPBlfIicXmIdDWYFkrsAyjNYEEZvAAuU=
=SBKO
-----END PGP SIGNATURE-----


S
S
Simon South wrote on 21 Jul 2021 15:14
(name . Efraim Flashner)(address . efraim@flashner.co.il)
87r1frajwt.fsf@simonsouth.net
Efraim Flashner <efraim@flashner.co.il> writes:
Toggle quote (2 lines)
> It's possible it was mkdir vs mkdir-p

Yes, or perhaps I hadn't yet added the code that sets the ownership and
permissions on the directory.

Morgan, I hope you'll submit a revised patch so others can review.

--
Simon South
simon@simonsouth.net
?