[bug#61744] [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[bug#61744] [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[Bruno Victal]

* doc/guix.texi (Base Services): Replace pam-limits-service with pam-limits-service-type.
* gnu/packages/benchmark.scm (python-locust)[description]: Update index anchor to manual.
* gnu/services/base.scm (pam-limits-service-type): Accept both lists and
file-like objects for compatibility.
(pam-limits-service): Deprecate procedure.
---

Sending this one for review now since this service is a bit unusual compared to the other ones.

 doc/guix.texi              | 18 ++++++++---------
 gnu/packages/benchmark.scm |  2 +-
 gnu/services/base.scm      | 41 +++++++++++++++++++++++++++-----------
 3 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index a7ef00f421..9127090d44 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -18926,7 +18926,6 @@ Base Services
 @var{device} does not exist.
 @end deffn
 
-@anchor{pam-limits-service}
 @cindex session limits
 @cindex ulimit
 @cindex priority
@@ -18934,19 +18933,20 @@ Base Services
 @cindex jackd
 @cindex nofile
 @cindex open file descriptors
-@deffn {Scheme Procedure} pam-limits-service [#:limits @code{'()}]
-
-Return a service that installs a configuration file for the
+@anchor{pam-limits-service-type}
+@defvar pam-limits-service-type
+Type of the service that installs a configuration file for the
 @uref{http://linux-pam.org/Linux-PAM-html/sag-pam_limits.html,
-@code{pam_limits} module}.  The procedure optionally takes a list of
-@code{pam-limits-entry} values, which can be used to specify
+@code{pam_limits} module}.  The value for this service type is
+a list of @code{pam-limits-entry} values, which can be used to specify
 @code{ulimit} limits and @code{nice} priority limits to user sessions.
+By default, the value is the empty list.
 
 The following limits definition sets two hard and soft limits for all
 login sessions of users in the @code{realtime} group:
 
 @lisp
-(pam-limits-service
+(service pam-limits-service-type
  (list
   (pam-limits-entry "@@realtime" 'both 'rtprio 99)
   (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
@@ -18961,7 +18961,7 @@ Base Services
 descriptors that can be used:
 
 @lisp
-(pam-limits-service
+(service pam-limits-service-type
  (list
   (pam-limits-entry "*" 'both 'nofile 100000)))
 @end lisp
@@ -18972,7 +18972,7 @@ Base Services
 else the users would be prevented from login in.  For more information
 about the Pluggable Authentication Module (PAM) limits, refer to the
 @samp{pam_limits} man page from the @code{linux-pam} package.
-@end deffn
+@end defvar
 
 @defvar greetd-service-type
 @uref{https://git.sr.ht/~kennylevinsen/greetd, @code{greetd}} is a minimal and
diff --git a/gnu/packages/benchmark.scm b/gnu/packages/benchmark.scm
index 33e2466da9..fd8513f41d 100644
--- a/gnu/packages/benchmark.scm
+++ b/gnu/packages/benchmark.scm
@@ -458,7 +458,7 @@ (define-public python-locust
 
 Note: Locust will complain if the available open file descriptors limit for
 the user is too low.  To raise such limit on a Guix System, refer to
-@samp{info guix --index-search=pam-limits-service}.")
+@samp{info guix --index-search=pam-limits-service-type}.")
     (license license:expat)))
 
 (define-public interbench
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 35b03a877b..5a2e0263e4 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -40,7 +40,7 @@
 (define-module (gnu services base)
   #:use-module (guix store)
   #:use-module (guix deprecation)
-  #:autoload   (guix diagnostics) (warning &fix-hint)
+  #:autoload   (guix diagnostics) (warning report-error &fix-hint)
   #:autoload   (guix i18n) (G_)
   #:use-module (guix combinators)
   #:use-module (gnu services)
@@ -245,7 +245,7 @@ (define-module (gnu services base)
             kmscon-service-type
 
             pam-limits-service-type
-            pam-limits-service
+            pam-limits-service  ; deprecated
 
             greetd-service-type
             greetd-configuration
@@ -1570,17 +1570,13 @@ (define* (syslog-service #:optional (config (syslog-configuration)))
 
 
 (define pam-limits-service-type
-  (let ((security-limits
-         ;; Create /etc/security containing the provided "limits.conf" file.
-         (lambda (limits-file)
-           `(("security/limits.conf"
-              ,limits-file))))
-        (pam-extension
+  (let ((pam-extension
          (lambda (pam)
            (let ((pam-limits (pam-entry
                               (control "required")
                               (module "pam_limits.so")
-                              (arguments '("conf=/etc/security/limits.conf")))))
+                              (arguments
+                               '("conf=/etc/security/limits.conf")))))
              (if (member (pam-service-name pam)
                          '("login" "greetd" "su" "slim" "gdm-password" "sddm"
                            "sudo" "sshd"))
@@ -1588,7 +1584,26 @@ (define pam-limits-service-type
                   (inherit pam)
                   (session (cons pam-limits
                                  (pam-service-session pam))))
-                 pam)))))
+                 pam))))
+
+        ;; XXX: Using file-like objects is deprecated, use lists instead.
+        ;;      This is to be reduced into the list? case when the deprecated
+        ;;      code gets removed.
+        ;; Create /etc/security containing the provided "limits.conf" file.
+        (security-limits
+         (match-lambda
+           ((? file-like? obj)
+            (warning (G_ "Using file-like value for 'pam-limits-service-type'
+is deprecated~%"))
+            obj)
+           ((? list? lst)
+            `(("security/limits.conf"
+               ,(plain-file "limits.conf"
+                            (string-join (map pam-limits-entry->string lst)
+                                         "\n" 'suffix)))))
+           (_ (report-error
+               (G_ "invalid input for 'pam-limits-service-type'~%"))))))
+
     (service-type
      (name 'limits)
      (extensions
@@ -1598,9 +1613,11 @@ (define pam-limits-service-type
      (description
       "Install the specified resource usage limits by populating
 @file{/etc/security/limits.conf} and using the @code{pam_limits}
-authentication module."))))
+authentication module.")
+     (default-value '()))))
 
-(define* (pam-limits-service #:optional (limits '()))
+(define-deprecated (pam-limits-service #:optional (limits '()))
+  pam-limits-service-type
   "Return a service that makes selected programs respect the list of
 pam-limits-entry specified in LIMITS via pam_limits.so."
   (service pam-limits-service-type

base-commit: 5d10644371abd54d0edcd638691113f0a92de743
-- 
2.39.1

[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.

[Bruno Victal]

* doc/guix.texi (Base Services): Replace pam-limits-service with pam-limits-service-type.
* gnu/packages/benchmark.scm (python-locust)[description]: Update index anchor to manual.
* gnu/services/base.scm (pam-limits-service-type): Set default value.
(pam-limits-service): Deprecate procedure.
---
 doc/guix.texi              | 37 ++++++++++++++++++++++---------------
 gnu/packages/benchmark.scm |  2 +-
 gnu/services/base.scm      |  8 +++++---
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 74658dbc86..3aa9c0cdf4 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -18938,7 +18938,6 @@ Base Services
 @end table
 @end deftp
 
-@anchor{pam-limits-service}
 @cindex session limits
 @cindex ulimit
 @cindex priority
@@ -18946,22 +18945,28 @@ Base Services
 @cindex jackd
 @cindex nofile
 @cindex open file descriptors
-@deffn {Scheme Procedure} pam-limits-service [#:limits @code{'()}]
-
-Return a service that installs a configuration file for the
+@anchor{pam-limits-service-type}
+@defvar pam-limits-service-type
+Type of the service that installs a configuration file for the
 @uref{http://linux-pam.org/Linux-PAM-html/sag-pam_limits.html,
-@code{pam_limits} module}.  The procedure optionally takes a list of
-@code{pam-limits-entry} values, which can be used to specify
-@code{ulimit} limits and @code{nice} priority limits to user sessions.
+@code{pam_limits} module}.  The value for this service type is
+a file-like object containing a list of @code{pam-limits-entry} values
+which can be used to specify @code{ulimit} limits and @code{nice}
+priority limits to user sessions.
 
 The following limits definition sets two hard and soft limits for all
 login sessions of users in the @code{realtime} group:
 
 @lisp
-(pam-limits-service
- (list
-  (pam-limits-entry "@@realtime" 'both 'rtprio 99)
-  (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
+(service
+  pam-limits-service-type
+  (plain-file
+    "limits.conf"
+    (string-join
+      (map pam-limits-entry->string
+        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
+      "\n")))
 @end lisp
 
 The first entry increases the maximum realtime priority for
@@ -18973,9 +18978,11 @@ Base Services
 descriptors that can be used:
 
 @lisp
-(pam-limits-service
- (list
-  (pam-limits-entry "*" 'both 'nofile 100000)))
+(service
+  pam-limits-service-type
+  (plain-file
+    "limits.conf"
+    (pam-limits-entry->string (pam-limits-entry "*" 'both 'nofile 100000))))
 @end lisp
 
 In the above example, the asterisk means the limit should apply to any
@@ -18984,7 +18991,7 @@ Base Services
 else the users would be prevented from login in.  For more information
 about the Pluggable Authentication Module (PAM) limits, refer to the
 @samp{pam_limits} man page from the @code{linux-pam} package.
-@end deffn
+@end defvar
 
 @defvar greetd-service-type
 @uref{https://git.sr.ht/~kennylevinsen/greetd, @code{greetd}} is a minimal and
diff --git a/gnu/packages/benchmark.scm b/gnu/packages/benchmark.scm
index 33e2466da9..fd8513f41d 100644
--- a/gnu/packages/benchmark.scm
+++ b/gnu/packages/benchmark.scm
@@ -458,7 +458,7 @@ (define-public python-locust
 
 Note: Locust will complain if the available open file descriptors limit for
 the user is too low.  To raise such limit on a Guix System, refer to
-@samp{info guix --index-search=pam-limits-service}.")
+@samp{info guix --index-search=pam-limits-service-type}.")
     (license license:expat)))
 
 (define-public interbench
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 1423ab6767..e5023b8175 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -246,7 +246,7 @@ (define-module (gnu services base)
             kmscon-service-type
 
             pam-limits-service-type
-            pam-limits-service
+            pam-limits-service  ; deprecated
 
             greetd-service-type
             greetd-configuration
@@ -1612,9 +1612,11 @@ (define pam-limits-service-type
      (description
       "Install the specified resource usage limits by populating
 @file{/etc/security/limits.conf} and using the @code{pam_limits}
-authentication module."))))
+authentication module.")
+     (default-value (plain-file "limits.conf" "")))))
 
-(define* (pam-limits-service #:optional (limits '()))
+(define-deprecated (pam-limits-service #:optional (limits '()))
+  pam-limits-service-type
   "Return a service that makes selected programs respect the list of
 pam-limits-entry specified in LIMITS via pam_limits.so."
   (service pam-limits-service-type
-- 
2.39.1

[bug#61744] [PATCH v2 2/2] services: pam-limits-service-type: Deprecate file-like object support in favour for lists as service value.

[Bruno Victal]

* doc/guix.texi (Base Services): Document it.
* gnu/local.mk: Register test.
* gnu/services/base.scm (pam-limits-service-type): Accept both lists and
file-like objects. Deprecate file-like object support.
* gnu/tests/pam.scm: New file.
---
 doc/guix.texi         | 27 +++++-------
 gnu/local.mk          |  2 +
 gnu/services/base.scm | 36 +++++++++++-----
 gnu/tests/pam.scm     | 97 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 135 insertions(+), 27 deletions(-)
 create mode 100644 gnu/tests/pam.scm

diff --git a/doc/guix.texi b/doc/guix.texi
index 3aa9c0cdf4..5c9a9333b9 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -18950,23 +18950,18 @@ Base Services
 Type of the service that installs a configuration file for the
 @uref{http://linux-pam.org/Linux-PAM-html/sag-pam_limits.html,
 @code{pam_limits} module}.  The value for this service type is
-a file-like object containing a list of @code{pam-limits-entry} values
-which can be used to specify @code{ulimit} limits and @code{nice}
-priority limits to user sessions.
+a list of @code{pam-limits-entry} values, which can be used to specify
+@code{ulimit} limits and @code{nice} priority limits to user sessions.
+By default, the value is the empty list.
 
 The following limits definition sets two hard and soft limits for all
 login sessions of users in the @code{realtime} group:
 
 @lisp
-(service
-  pam-limits-service-type
-  (plain-file
-    "limits.conf"
-    (string-join
-      (map pam-limits-entry->string
-        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
-              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
-      "\n")))
+(service pam-limits-service-type
+         (list
+          (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+          (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
 @end lisp
 
 The first entry increases the maximum realtime priority for
@@ -18978,11 +18973,9 @@ Base Services
 descriptors that can be used:
 
 @lisp
-(service
-  pam-limits-service-type
-  (plain-file
-    "limits.conf"
-    (pam-limits-entry->string (pam-limits-entry "*" 'both 'nofile 100000))))
+(service pam-limits-service-type
+         (list
+          (pam-limits-entry "*" 'both 'nofile 100000)))
 @end lisp
 
 In the above example, the asterisk means the limit should apply to any
diff --git a/gnu/local.mk b/gnu/local.mk
index 415955bd3f..6291d8a558 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -56,6 +56,7 @@
 # Copyright © 2022 Alex Griffin <a@ajgrf.com>
 # Copyright © 2022 ( <paren@disroot.org>
 # Copyright © 2022 jgart <jgart@dismail.de>
+# Copyright © 2023 Bruno Victal <mirai@makinata.eu>
 #
 # This file is part of GNU Guix.
 #
@@ -778,6 +779,7 @@ GNU_SYSTEM_MODULES =				\
   %D%/tests/messaging.scm			\
   %D%/tests/networking.scm			\
   %D%/tests/package-management.scm		\
+  %D%/tests/pam.scm				\
   %D%/tests/reconfigure.scm			\
   %D%/tests/rsync.scm				\
   %D%/tests/samba.scm				\
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index e5023b8175..80f9607d44 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -40,7 +40,7 @@
 (define-module (gnu services base)
   #:use-module (guix store)
   #:use-module (guix deprecation)
-  #:autoload   (guix diagnostics) (warning &fix-hint)
+  #:autoload   (guix diagnostics) (warning formatted-message &fix-hint)
   #:autoload   (guix i18n) (G_)
   #:use-module (guix combinators)
   #:use-module (gnu services)
@@ -1584,17 +1584,13 @@ (define-deprecated (syslog-service #:optional (config (syslog-configuration)))
 
 
 (define pam-limits-service-type
-  (let ((security-limits
-         ;; Create /etc/security containing the provided "limits.conf" file.
-         (lambda (limits-file)
-           `(("security/limits.conf"
-              ,limits-file))))
-        (pam-extension
+  (let ((pam-extension
          (lambda (pam)
            (let ((pam-limits (pam-entry
                               (control "required")
                               (module "pam_limits.so")
-                              (arguments '("conf=/etc/security/limits.conf")))))
+                              (arguments
+                               '("conf=/etc/security/limits.conf")))))
              (if (member (pam-service-name pam)
                          '("login" "greetd" "su" "slim" "gdm-password" "sddm"
                            "sudo" "sshd"))
@@ -1602,7 +1598,27 @@ (define pam-limits-service-type
                   (inherit pam)
                   (session (cons pam-limits
                                  (pam-service-session pam))))
-                 pam)))))
+                 pam))))
+
+        ;; XXX: Using file-like objects is deprecated, use lists instead.
+        ;;      This is to be reduced into the list? case when the deprecated
+        ;;      code gets removed.
+        ;; Create /etc/security containing the provided "limits.conf" file.
+        (security-limits
+         (match-lambda
+           ((? file-like? obj)
+            (warning (G_ "Using file-like value for \
+'pam-limits-service-type' is deprecated~%"))
+            `(("security/limits.conf" ,obj)))
+           ((? list? lst)
+            `(("security/limits.conf"
+               ,(plain-file "limits.conf"
+                            (string-join (map pam-limits-entry->string lst)
+                                         "\n" 'suffix)))))
+           (_ (raise
+               (formatted-message
+                (G_ "invalid input for 'pam-limits-service-type'~%")))))))
+
     (service-type
      (name 'limits)
      (extensions
@@ -1613,7 +1629,7 @@ (define pam-limits-service-type
       "Install the specified resource usage limits by populating
 @file{/etc/security/limits.conf} and using the @code{pam_limits}
 authentication module.")
-     (default-value (plain-file "limits.conf" "")))))
+     (default-value '()))))
 
 (define-deprecated (pam-limits-service #:optional (limits '()))
   pam-limits-service-type
diff --git a/gnu/tests/pam.scm b/gnu/tests/pam.scm
new file mode 100644
index 0000000000..5cf13d97d7
--- /dev/null
+++ b/gnu/tests/pam.scm
@@ -0,0 +1,97 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (gnu tests pam)
+  #:use-module (gnu tests)
+  #:use-module (gnu services)
+  #:use-module (gnu services base)
+  #:use-module (gnu system)
+  #:use-module (gnu system pam)
+  #:use-module (gnu system vm)
+  #:use-module (guix gexp)
+  #:use-module (ice-9 format)
+  #:export (%test-pam-limits
+            %test-pam-limits-deprecated))
+
+\f
+;;;
+;;; pam-limits-service-type
+;;;
+
+(define pam-limit-entries
+  (list
+   (pam-limits-entry "@realtime" 'both 'rtprio 99)
+   (pam-limits-entry "@realtime" 'both 'memlock 'unlimited)))
+
+(define (run-test-pam-limits config)
+  "Run tests in a os with pam-limits-service-type configured."
+  (define os
+    (marionette-operating-system
+     (simple-operating-system
+      (service pam-limits-service-type config))))
+
+  (define vm
+    (virtual-machine os))
+
+  (define name (format #f "pam-limit-service~:[~;-deprecated~]"
+                       (file-like? config)))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (gnu build marionette)
+                       (srfi srfi-64))
+
+          (let ((marionette (make-marionette (list #$vm))))
+
+            (test-runner-current (system-test-runner #$output))
+
+            (test-begin #$name)
+
+            (test-assert "/etc/security/limits.conf ready"
+              (wait-for-file "/etc/security/limits.conf" marionette))
+
+            (test-equal "/etc/security/limits.conf content matches"
+              #$(string-join (map pam-limits-entry->string pam-limit-entries)
+                             "\n" 'suffix)
+              (marionette-eval
+               '(call-with-input-file "/etc/security/limits.conf"
+                  get-string-all)
+               marionette))
+
+            (test-end)))))
+
+  (gexp->derivation (string-append name "-test") test))
+
+(define %test-pam-limits
+  (system-test
+   (name "pam-limits-service")
+   (description "Test that pam-limits-service can serialize its config
+(as a list) to @file{limits.conf}.")
+   (value (run-test-pam-limits pam-limit-entries))))
+
+(define %test-pam-limits-deprecated
+  (system-test
+   (name "pam-limits-service-deprecated")
+   (description "Test that pam-limits-service can serialize its config
+(as a file-like object) to @file{limits.conf}.")
+   (value (run-test-pam-limits
+           (plain-file "limits.conf"
+                       (string-join (map pam-limits-entry->string
+                                         pam-limit-entries)
+                                    "\n" 'suffix))))))
-- 
2.39.1

[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.

[Ricardo Wurmus]

Hi,

thank you for the patches!

The effective change looks fine to me, but I’m confused about why these
are two patches.  The first one introduces this as an example in the
docs:

+(service
+  pam-limits-service-type
+  (plain-file
+    "limits.conf"
+    (string-join
+      (map pam-limits-entry->string
+        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
+      "\n")))

But the second removes this again in favour of this prettier form:

+(service pam-limits-service-type
+         (list
+          (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+          (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))

Which is really close to the original form:

-(pam-limits-service
- (list
-  (pam-limits-entry "@@realtime" 'both 'rtprio 99)
-  (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))

Could you merge these two patches to reduce the number of unnecessary
changes?  I don’t think we should change to file-likes as the argument
value for the pam-limits-service-type.

Another thing that confused me:

+            (test-equal "/etc/security/limits.conf content matches"
+              #$(string-join (map pam-limits-entry->string pam-limit-entries)
+                             "\n" 'suffix)
+              (marionette-eval
+               '(call-with-input-file "/etc/security/limits.conf"
+                  get-string-all)
+               marionette))

Why use the gexp with a computed value here instead of using just the
plain text of the expected contents of that file?  Computing
the expected value in a test where the compared value is computed in the
same way feels like begging the question.

Or perhaps I’m misunderstanding something here?

-- 
Ricardo

[bug#61744] [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.

[Bruno Victal]

Hi Ricardo,

On 2023-03-10 17:52, Ricardo Wurmus wrote:
> Hi,
> 
> thank you for the patches!
> 
> The effective change looks fine to me, but I’m confused about why these
> are two patches.  The first one introduces this as an example in the
> docs:

[...]

> 
> +(service
> +  pam-limits-service-type
> +  (plain-file
> +    "limits.conf"
> +    (string-join
> +      (map pam-limits-entry->string
> +        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> +              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> +      "\n")))
> 
> But the second removes this again in favour of this prettier form:

This was to ensure that each commit is "atomic".

> 
> +(service pam-limits-service-type
> +         (list
> +          (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> +          (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> 
> Which is really close to the original form:
> 
> -(pam-limits-service
> - (list
> -  (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> -  (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> 
> Could you merge these two patches to reduce the number of unnecessary
> changes?  I don’t think we should change to file-likes as the argument
> value for the pam-limits-service-type.
The v2 patch-series are a dis-aggregation of the v1 series (save for a bug fix
in the match clauses, test suite and using raise instead of report-error)
as indicated in the 10/27 patch-series review from #61789.

> 
> Another thing that confused me:
> 
> +            (test-equal "/etc/security/limits.conf content matches"
> +              #$(string-join (map pam-limits-entry->string pam-limit-entries)
> +                             "\n" 'suffix)
> +              (marionette-eval
> +               '(call-with-input-file "/etc/security/limits.conf"
> +                  get-string-all)
> +               marionette))
> 
> Why use the gexp with a computed value here instead of using just the
> plain text of the expected contents of that file?  Computing
> the expected value in a test where the compared value is computed in the
> same way feels like begging the question.
> 
> Or perhaps I’m misunderstanding something here?
> 

I wrote this test suite to simply check that both deprecated and "new" service-type forms
work correctly, i.e. the files are present in their locations.
(this actually caught a bug within the match clauses in the v1 patch)


Cheers,
Bruno

bug#61744: [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[Ludovic Courtès]

Hi Bruno,

Thanks for explaining.  It seems to me that none of the issues raised is
a blocker, so I went ahead and applied these two patches.

Thank you, and apologies for the delay!

Ludo’.

[bug#61744] [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[Felix Lechner via Guix-patches via]

Hi Bruno,

Thanks for this great and important work!

Can we refer to the limits.conf file in the store, please? I do not
believe we need a copy in /etc/security, and should not keep one
there.

The "conf=" argument to pam_limits(8) accepts an absolute path. [1] We
use that mechanism already (for the default path). Thanks!

Kind regards,
Felix Lechner

[1] https://linux.die.net/man/8/pam_limits

[bug#61744] [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[Ludovic Courtès]

Hi Felix,

Felix Lechner <felix.lechner@lease-up.com> skribis:

> Can we refer to the limits.conf file in the store, please? I do not
> believe we need a copy in /etc/security, and should not keep one
> there.

I’m generally in favor of not populating /etc and instead referring to
store file names.

In some cases (maybe this one), this can be a problem though, in
particular for upgrades (the module keeps referring to the old config
file in the store).  So I don’t know, but this needs to be taken into
account.

Ludo’.

[bug#61744] [PATCH] services: base: Deprecate 'pam-limits-service' procedure.

[Felix Lechner via Guix-patches via]

Hi Ludovic,

On Thu, Mar 30, 2023 at 1:54 PM Ludovic Courtès <ludo@gnu.org> wrote:
>
> In some cases (maybe this one), this can be a problem

Thanks for pointing that out! I would like to learn more about that.

My next suggestion would have been to refer to the core PAM modules,
which ship with Linux-PAM, by absolute paths as well.

You can see the current inconsistencies in my PAM 'login' service,
which I included below. Which breakage do you expect?

On a side note, I am also working with the pam_mount maintainer on a
store path for /etc/security/pam_mount_conf.xml. [1] (Jan previously
accepted another suggestion of mine, and it became popular with
users.) Then we can drop the definition of 'greet-pam-mount' [2] which
is very nearly a duplicate of the regular 'pam-mount'. [3]

Kind regards
Felix

[1] https://codeberg.org/jengelh/pam_mount/issues/1
[2] https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/admin.scm#n5314
[3] https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/admin.scm#n4709

* * *

account required pam_unix.so
auth required pam_unix.so nullok
auth optional /gnu/store/zb9ns323p7yv8m1m155yfgrxlxaadx3d-greetd-pam-mount-2.18/lib/security/pam_mount.so
disable_interactive
password required pam_unix.so sha512 shadow
session required
/gnu/store/7sq4qp09fl1pn72jw828ndm13nbpknhv-elogind-246.10/lib/security/pam_elogind.so
session required pam_limits.so conf=/etc/security/limits.conf
session optional pam_motd.so
motd=/gnu/store/mrk0km6gqw4zn20az2bqidvajps7yy93-motd
session required pam_loginuid.so
session required pam_env.so
session required pam_unix.so
session optional
/gnu/store/zb9ns323p7yv8m1m155yfgrxlxaadx3d-greetd-pam-mount-2.18/lib/security/pam_mount.so
disable_interactive