[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#44435] [PATCH v2 0/1] services: Add Transmission Daemon
From: |
Ludovic Courtès |
Subject: |
[bug#44435] [PATCH v2 0/1] services: Add Transmission Daemon |
Date: |
Wed, 18 Nov 2020 23:39:01 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Hi Simon,
Simon South <simon@simonsouth.net> skribis:
> Here's an updated version of the patch that
>
> - Fixes the "importing module from host" warning by removing an unnecessary
> import of (guix gexp) in transmission-daemon-computed-settings-file; and
Good. :-)
> - I've placed the code in a new "(gnu services file-sharing)" module and the
> documentation in a new "File-Sharing Services" section of the manual, only
> because these names seemed the most natural to me. ("Peer-to-peer" would be
> too broad a categorization, I think, while "BitTorrent" too narrow.)
Sounds good to me.
> - The module exports two procedures, "transmission-password-hash" and
> "transmission-random-salt", that together are my solution to the problem of
> assigning a value to the daemon's "rpc-password" configuration setting.
>
> Transmission clients seem to expect the user to supply a password in
> plaintext in their "settings.json" file. At startup, the client generates a
> random, eight-character salt value; hashes it and the password together; and
> writes the result back to the settings file, after which the password
> remains obscured. This obviously violates the functional nature of Guix, as
> we don't expect services to be rewriting their own configuration files and
> the use of a random salt value makes the process non-repeatable anyway.
>
> I've documented in the manual how a user can use these two procedures to
> create a suitable value for "rpc-password" that remains stable across system
> reconfigurations, but perhaps you know of a better (or more conventional)
> approach.
Looks like a good idea. At worst we’ll have to keep it in sync with
what future versions of Transmission do, but I guess it’s unlikely to
change often.
> - I've added a custom "stop" procedure to the Shepherd service that gives the
> daemon time to shut down before eventually killing its process. This is
> necessary since the daemon performs some housekeeping and sends a final
> update to BitTorrent trackers before it exits, which can take several
> seconds or more; without this code, restarting the service usually fails as
> the new daemon process finds the old one is still running and attached to
> the port used for peer connections.
OK.
> +@node File-Sharing Services
> +@subsection File-Sharing Services
> +
> +The @code{(gnu services file-sharing)} module provides services that
> +assist with transferring files over peer-to-peer file-sharing networks.
> +
> +@subsubheading Transmission Daemon Service
> +
> +@uref{https://transmissionbt.com/, Transmission} is a flexible
Great that you took the time to write good documentation with examples!
> +(define (transmission-password-hash password salt)
> + "Returns a string containing the result of hashing @var{password} together
> +with @var{salt}, in the format recognized by Transmission clients for their
> +@code{rpc-password} configuration setting.
> +
> +@var{salt} must be an eight-character string. The
> +@code{transmission-random-salt} procedure can be used to generate a suitable
> +salt value at random."
> + (if (not (eq? (string-length salt) %transmission-salt-length))
> + (throw 'out-of-range
> + (format #f
> + "salt value must be ~d characters in length"
> + %transmission-salt-length))
I’d recommend using (srfi srfi-34), (srfi srfi-35), (guix i18n), and
(guix diagnostics) and write it like so:
(raise (condition (formatted-message
(G_ "salt value …") …)))
Then you can also add this file to po/packages/POTFILE.in for
translation.
> + (let ((password-digest (call-with-input-string
> + (string-append password salt)
> + (lambda (port)
> + (port-hash (hash-algorithm sha1) port)))))
More concise:
(sha1 (string->utf8 (string-append password salt)))
> + (stop #~(lambda (pid)
> + (kill pid SIGTERM)
> +
> + ;; Transmission Daemon normally needs some time to shut down,
> + ;; as it will complete some housekeeping and send a final
> + ;; update to trackers before it exits.
> + ;;
> + ;; Wait a reasonable period for it to stop before continuing.
> + ;; If we don't do this, restarting the service can fail as
> the
> + ;; new daemon process finds the old one still running and
> + ;; attached to the port used for peer connections.
> + (let wait-before-killing ((period #$stop-wait-period))
> + (if (zero? (car (waitpid pid WNOHANG)))
> + (if (positive? period)
> + (begin
> + (sleep 1)
> + (wait-before-killing (- period 1)))
> + (begin
> + (format #t
> + "Wait period expired; killing \
> +transmission-daemon (pid ~a).~%"
> + pid)
> + (display "(If you see this message regularly,
> you \
> +may need to increase the value
> +of 'stop-wait-period' in the service configuration.)\n")
> + kill pid SIGKILL))))
^
Missing parens.
Ideally this SIGTERM-then-SIGKILL dance would be done by shepherd
itself. Future work!
I’m not familiar with Transmission so I can’t really comment on the
other things, but overall it LGTM apart from the details above.
Could you send an updated patch?
It would be nice to have a minimal system test to ensure at least
Transmission starts and is fine with the generated config file; we can
leave that to another patch though, if you prefer.
Thanks!
Ludo’.