Re: [PATCH] Add pam-limits-service.

[Ludovic Courtès]

Guten Tag!

Ricardo Wurmus <address@hidden> skribis:

> We now have a constructor “pam-limits-entry”, which validates given
> settings (i.e. it throws an error when values are passed that don’t make
> sense) and returns a value of type “<pam-limits-entry>”.
>
> A list of these values can be passed to “pam-limits-service”, which
> generates a working “/etc/security/limits.conf”.  I’m using it right now
> with the exact same limits that are now documented in the manual.
>
> This snippet:
>
>   (pam-limits-service
>    (list
>     (pam-limits-entry "@realtime" 'both 'rtprio 99)
>     (pam-limits-entry "@realtime" 'both 'memlock 'unlimited)))
>
> generates a limits.conf file with the following contents:
>
>   @realtime   -  rtprio     99
>   @realtime   -  memlock    unlimited
>
> One advantage of using “pam-limits-entry” instead of a plain string is
> that values are validated according to the documentation in “man 5
> limits.conf”.

Nice!

Eventually, we should probably use a constructor in the spirit of (rnrs
enums) to provide expansion-time validation, as already done in (gnu
system nss) (info "(guile) rnrs enums").

> From 3f5d7b405ac7faadd753719fe4100d8f6605d191 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <address@hidden>
> Date: Mon, 12 Oct 2015 07:11:51 +0200
> Subject: [PATCH] services: Add pam-limits-service.
>
> * gnu/system/pam.scm (<pam-limits-entry>): New record type.
> (pam-limits-entry, pam-limits-entry->string): New procedures.
> * gnu/services/base.scm (pam-limits-service-type): New variable.
> (pam-limits-service): New procedure.
> * doc/guix.texi (Base Services): Document it.

[...]

> address@hidden {Scheme Procedure} pam-limits-service [#:limits @var{limits}]
> +
> +Return a service that installs a configuration file for the
> address@hidden module.  The procedure optionally takes a list of
         ^^^^^^^^^^^^^^^^^^
It would be nice to add an @uref to the on-line manual of pam_limits, if
it exists.

> +(define pam-limits-service-type
> +  (let ((security-limits
> +         ;; Create /etc/security containing the provided "limits.conf" file.
> +         (lambda (limits-file)
> +           `(("security"
> +              ,(computed-file
> +                "security"
> +                #~(begin (mkdir #$output)
> +                         (stat #$limits-file)
> +                         (symlink #$limits-file
> +                                  (string-append #$output 
> "/limits.conf"))))))))

Indentation, rather:

  (begin
    (mkdir #$output)
    …)

> +    (service-type
> +     (name 'limits)
> +     (extensions
> +      (list (service-extension etc-service-type security-limits)
> +            (service-extension pam-root-service-type
> +                               (lambda _ (list pam-extension))))))))

It may be useful to allow users to extend this service with additional
<pam-limits-entry> objects.  To do that we’d simply need something like:

  (service-type
    (name 'limits)
    ;; …
    (compose concatenate)   ;concatenate lists of <pam-limits-entry>
    (extend append))        ;append them

WDYT?

This shouldn’t block this patch, though.

> +(define-record-type <pam-limits-entry>
> +  (make-pam-limits-entry domain type item value)

Maybe just add a comment above with the URL of the reference manual.

Otherwise LGTM, thank you!

Ludo’.