[bug#44435] [PATCH v2 0/1] services: Add Transmission Daemon

["Ludovic Courtès" <ludo@gnu.org>]

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’.