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

[Ricardo Wurmus]

Ludovic Courtès <address@hidden> writes:

> Ricardo Wurmus <address@hidden> skribis:

[…]

>> 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").

Oh, that’s new to me.  I was a little disappointed that records don’t
support what Haskell calls “smart constructors”.  I’ll check out enums
and see if I can improve this later.  (Added to my growing org-mode
list of Guix things.)

Thanks for the pointer!

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

Added a link.

>> +(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)
>     …)

Okay.

>> +    (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.

Currently the default limits are an empty list, so there doesn’t seem to
be any advantage over simply passing a list of <pam-limits-entry>
values.  Or is composition/extension here about e.g. enabling some
specialised sub-service to inherit from pam-limits-service and modify
the list of entries?

>> +(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.

Done.

> Otherwise LGTM, thank you!

Yay, pushing this to master now.  Thank you for the patient review!

~~ Ricardo