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

[Arun Isaac <arunisaac@systemreboot.net>]

Hi Fabio,

Thank you for the patch. That's an excellent patch for a first Guix
service! I can only suggest a few minor improvements (mostly nitpicks
really).

Could you also suggest some quick way for me to test this service
without actually having to reconfigure my system? Can I, for example,
put it in a Guix system container or VM and test it that way?

> +(define %readymedia-cache-dir "/var/cache/readymedia")
> +(define %readymedia-log-dir "/var/log/readymedia")

Can we have these two in the <readymedia-configuration> record?

> +(define %readymedia-user-account "readymedia")
> +(define %readymedia-user-group "readymedia")

These are fine as they are.

> +  (readymedia readymedia-configuration-readymedia (default
> readymedia))

Nitpick: Just to be consistent with other services, I would indent this
(and the other fields) like so with the default on the next line:

>   (readymedia readymedia-configuration-readymedia
>               (default readymedia))

> +(define (readymedia-configuration->config-file config)
> +  "Return the ReadyMedia/MiniDLNA configuration file corresponding to CONFIG."
> +  (let ((friendly-name (readymedia-configuration-friendly-name config))
> +        (media-dirs (readymedia-configuration-media-dirs config))
> +        (port (readymedia-configuration-port config))
> +        (extra-config (readymedia-configuration-extra-config config)))
> +    (plain-file
> +     "minidlna.conf"
> +     (string-append
> +      "db_dir=" %readymedia-cache-dir "\n"
> +      "log_dir=" %readymedia-log-dir "\n"
> +      (if friendly-name (format #f "friendly_name=~a\n" friendly-name) "")
> +      (if port (format #f "port=~a\n" port) "")
> +      (string-join (map readymedia-media-dir->string media-dirs) "\n" 'suffix)
> +      (string-join extra-config "\n" 'suffix)))))

Could you use mixed-text-file here instead of plain-file? Or, you could
also try computed-file if that's more succinct.

> +(define (readymedia-shepherd-service config)
> +  "Return a least-authority ReadyMedia/MiniDLNA Shepherd service."
> +  (let* ((minidlna-conf (readymedia-configuration->config-file config))
> +         (media-dirs (readymedia-configuration-media-dirs config))
> +         (readymedia (least-authority-wrapper
> +                      (file-append
> +                       (readymedia-configuration-readymedia config)
> +                       "/sbin/minidlnad")
> +                      #:name "minidlna"
> +                      #:mappings (cons*
> +                                  (file-system-mapping
> +                                   (source %readymedia-cache-dir)
> +                                   (target source)
> +                                   (writable? #t))

Re-format by putting the first file-system-mapping on the same line as
the cons*. It's customary to format lisp function calls that way. It
makes it easier to see what the arguments are.

> +                                  (map
> +                                   (lambda (e)
> +                                     (file-system-mapping
> +                                      (source (readymedia-media-dir-path e))
> +                                      (target source)
> +                                      (writable? #f)))

Likwise with map. Put the lambda on the same line as the map.

Looking forward to a v2 patch!

Regards,
Arun