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

[Fabio Natali via Guix-patches via <guix-patches@gnu.org>]

Hi Bruno,

Thanks for providing feedback on this and thanks for the help provided
on IRC. I've gone through your comments and did my best to address
them. See my replies inline below.

On 2024-08-20, 03:14 +0100, Bruno Victal <mirai@makinata.eu> wrote:
>> +@item @code{media-dirs} (type: list)
>> +The list of media folders to serve content from.  Each item is a
>> +@code{readymedia-media-dir}.
>> +
>> +@item @code{cache-dir} (default: @code{"/var/cache/readymedia"}) (type: string)
>> +A folder for ReadyMedia's cache files. If not existing already, the
>> +folder will be created as part of the service activation and the
>> +ReadyMedia user will be assigned ownership.
>> +
>> +@item @code{log-dir} (default: @code{"/var/log/readymedia"}) (type: string)
>> +A folder for ReadyMedia's log files. If not existing already, the
>> +folder will be created as part of the service activation and the
>> +ReadyMedia user will be assigned ownership.
>
> Expand these to media-directories, cache-directory, etc.

Good point, now fixed.

>> +@item @code{extra-config} (default: @code{'()}) (type: list-of-strings)
>> +A list of further options, to be passed as key-value strings as
>> +accepted by ReadyMedia.
>
> Do you have an example on this?
> Given the description perhaps an alist would work better here.

True, great point. That's now an alist. Example added too.

>> +@deftp {Data Type} readymedia-media-dir
>> +A @code{media-dirs} entry includes a @code{path} and, optionally, a
>> +media type string.
>
> Likewise, expand to readymedia-media-directory.

Fixed.

>> +@item @code{type} (default: @code{""}) (type: string)
>> +Valid media types are @code{"A"} for audio, @code{"P"} for pictures,
>> +@code{"V"} for video, and a combination of those individual letters
>> +for mixed types.  An empty string means no type specified.
>
> I'd use a list of symbols (or enum) here.

Fixed, switched to symbols.

>> +(define %readymedia-user-account "readymedia")
>> +(define %readymedia-user-group "readymedia")
>
> I think it would be better to expose this in the
> readymedia-configuration record-type and have it be oriented around
> user-account and user-group record-types, i.e.
[...]
> This way you can allow for users to fine-tune the account permissions,
> groups & co. used by readymedia.

Fixed, although I'm not sure I'm 100% on board with this.

I'm not completely sure but I have the feeling that a configurable
ReadyMedia user might theoretically weaken the POLA, e.g. if the user
chose their own user for this service.

Following up on a related conversation we started on IRC, I suppose we
should either go all in with flexibility (i.e. allow the user to switch
off the least-authority-wrapper and set the service user) or adopt a
slightly more rigid approach (mandated POLA and fixed user).

I think I might have a slight preference for the latter, prioritising
compartmentalisation over flexibility - but I'm keen to know what you,
Arun, and all other Guixers may think about this.

I'm glad to send a new version in case, where I switch back to a
mandated, non-configurable 'readymedia' user.

>> +(define (readymedia-activation config)
>> +  "Set up directories for ReadyMedia/MiniDLNA."
[...]
> I'd avoid using activation-service-type since it doesn't account for
> shepherd dependencies (which implies file-system mounts), consequence
> being that this service will be broken if any of these directories
> happen to be located outside of the root filesystem.
> (My advice is to avoid using activation-service-type unless you're
> sure of how the chain of action in guix+shepherd goes)

Ha, ok, I'd have never thought of this! With a bit of a
don't-know-what-i'm-doing feeling, I might have fixed this too. :)

Thanks to you and Arun for all the helpful feedback!

I hope v3 is in a better shape now (to follow shortly).

Thanks, cheers, Fabio.