[bug#72398] [PATCH v5] services: Add readymedia-service-type.

[Ludovic Courtès]

Hello,

Fabio Natali <me@fabionatali.com> skribis:

> * doc/guix.texi: Add documentation.
> * gnu/local.mk: Add mention of new files.

This is really minor, but please mention the place where this is added,
like:

  * doc/guix.texi (Section Name): New node.

> On a Guix system tests can be run with this command:
>
> make check-system TESTS="readymedia-service"

I get two failures:

--8<---------------cut here---------------start------------->8---
PASS: ReadyMedia user exists
PASS: ReadyMedia group exists
PASS: cache directory exists
PASS: cache directory has correct ownership
PASS: cache directory has expected permissions
/gnu/store/3z061ii32vr6klh3y8p9b43zq6lwibja-readymedia-service-test-builder:1: 
FAIL cache file exists
/gnu/store/3z061ii32vr6klh3y8p9b43zq6lwibja-readymedia-service-test-builder:1: 
FAIL cache file has expected permissions
PASS: cache file is non-empty
PASS: log directory exists
PASS: log directory has correct ownership
PASS: log directory has expected permissions
PASS: log file exists
PASS: log file has expected permissions
PASS: log file is non-empty
PASS: ReadyMedia service is running
PASS: ReadyMedia service is listening for connections
# of expected passes      14
# of unexpected failures  2
--8<---------------cut here---------------end--------------->8---

This might have to do with activation, see below.

> +The @code{(gnu services upnp)} module offers services related to the
> +DLNA and UPnP-VA networking protocols.  For now, it provides the

I would add a few words about what DLNA and UPnP-VA allow users to do,
and perhaps what they mean.

> +@code{readymedia-service-type} is a Guix service that wraps around
> +ReadyMedia's @code{minidlnad}.  For increased security, the service
> +makes use of @code{least-authority-wrapper} which limits the resources
> +that the daemon has access to.  The daemon runs as the
> +@code{readymedia} unprivileged user, which is a member of the
> +@code{readymedia} group.

I would omit everything that follows “For increased security” since it’s
largely an implementation detail (a nice one though!) and could get out
of sync over time.

> +    (list (shepherd-service
> +           (documentation "Run the ReadyMedia/MiniDLNA daemon.")
> +           (provision '(readymedia))
> +           (requirement '(networking user-processes))
> +           (start
> +            #~(begin
> +                (use-modules (gnu build activation))
> +                (let* ((user (getpw #$%readymedia-user-account))
> +                       (dirs (list
> +                              #$cache-directory
> +                              #$log-directory
> +                              #$@(map (lambda (e)
> +                                        (readymedia-media-directory-path e))
> +                                      media-directories)))
> +                       (init-directory (lambda (d)
> +                                         (unless (file-exists? d)
> +                                           (mkdir-p/perms d user #o755)))))
> +                  (for-each init-directory dirs))
> +                (make-forkexec-constructor
> +                 ;; "-S" is to daemonise minidlnad.
> +                 (list #$readymedia "-f" #$minidlna-conf "-S")
> +                 #:log-file #$%readymedia-log-file
> +                 #:user #$%readymedia-user-account
> +                 #:group #$%readymedia-user-group)))

This is problematic because the code above ‘make-forkexec-constructor’
is effectively executed as soon as shepherd reads the config file, which
may be too early or undesirable.

If you intended it to run when the service is started, you’ll have to
structure it like this:

  (start #~(lambda ()
             ;; create directories etc.
             (fork+exec-command (list #$readymedia …) …)))

Also, use the ‘modules’ field instead of ‘use-modules’ right in the
middle.

But! While I agree in principle with what Bruno wrote about the
shortcomings of activation snippets, I would stick to an activation
snippet here to create directories etc.  The change Bruno proposes
should be treated separately and systematically across all the services,
not just one of them.

> +(define %test-readymedia-service

Just ‘%test-readymedia’…

> +  (system-test
> +   (name "readymedia-service")

… and “readymedia”, for consistency with other tests.

Thanks,
Ludo’.