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

  • Done
  • quality assurance status badge
Details
4 participants
  • Felix Lechner
  • Ludovic Courtès
  • Bruno Victal
  • Ricardo Wurmus
Owner
unassigned
Submitted by
Bruno Victal
Severity
normal
B
B
Bruno Victal wrote on 24 Feb 2023 01:12
(address . guix-patches@gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
d319522cc9ff3ca759a5d1ebd8d57d7165e4bdc5.1677197399.git.mirai@makinata.eu
* 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(-)

Toggle diff (159 lines)
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
B
B
Bruno Victal wrote on 4 Mar 2023 22:17
[PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.
(address . 61744@debbugs.gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
47849c839cb8acb6909eccd1f050b0316373b377.1677964609.git.mirai@makinata.eu
* 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(-)

Toggle diff (117 lines)
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
B
B
Bruno Victal wrote on 4 Mar 2023 22:17
[PATCH v2 2/2] services: pam-limits-service-type: Deprecate file-like object support in favour for lists as service value.
(address . 61744@debbugs.gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
29b2df64b1a9a857227d573e7d0a1aa1f9ef52d2.1677964609.git.mirai@makinata.eu
* 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

Toggle diff (247 lines)
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))
+
+
+;;;
+;;; 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
R
R
Ricardo Wurmus wrote on 10 Mar 2023 18:52
[PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure.
(address . 61744@debbugs.gnu.org)(address . mirai@makinata.eu)
871qlwo4m8.fsf@elephly.net
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
B
B
Bruno Victal wrote on 11 Mar 2023 12:25
(name . Ricardo Wurmus)(address . rekado@elephly.net)
271039c5-c316-7a12-53a2-152b0b186538@makinata.eu
Hi Ricardo,

On 2023-03-10 17:52, Ricardo Wurmus wrote:
Toggle quote (8 lines)
> 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:

[...]

Toggle quote (13 lines)
>
> +(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".

Toggle quote (16 lines)
>
> +(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.

Toggle quote (19 lines)
>
> 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
F
F
Felix Lechner wrote on 20 Mar 2023 18:49
Re: [PATCH] services: base: Deprecate 'pam-limits-service' procedure.
(address . 61744@debbugs.gnu.org)
CAFHYt57dM1xAHnaZC3vmhcAp0fZ+hER-bqgNjjoG-id5UV8DLQ@mail.gmail.com
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

L
L
Ludovic Courtès wrote on 30 Mar 2023 22:53
Re: bug#61744: [PATCH] services: base: Deprecate 'pam-limits-service' procedure.
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
87jzyyhsf1.fsf_-_@gnu.org
Hi Felix,

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

Toggle quote (4 lines)
> 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’.
L
L
Ludovic Courtès wrote on 30 Mar 2023 23:08
(name . Bruno Victal)(address . mirai@makinata.eu)
875yaihrrd.fsf_-_@gnu.org
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’.
Closed
F
F
Felix Lechner wrote on 30 Mar 2023 23:19
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAFHYt57GWj7P3XHEfXKhqaP-FnAdhWfVb9Yz-geBc3u43oCFkA@mail.gmail.com
Hi Ludovic,

On Thu, Mar 30, 2023 at 1:54?PM Ludovic Courtès <ludo@gnu.org> wrote:
Toggle quote (3 lines)
>
> 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


* * *

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
?
Your comment

This issue is archived.

To comment on this conversation send an email to 61744@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 61744
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch