Package: guix-patches;
Reported by: Bruno Victal <mirai <at> makinata.eu>
Date: Fri, 24 Feb 2023 00:13:02 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61744 in the body.
You can then email your comments to 61744 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
ludo <at> gnu.org, guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Fri, 24 Feb 2023 00:13:02 GMT) Full text and rfc822 format available.Bruno Victal <mirai <at> makinata.eu>:ludo <at> gnu.org, guix-patches <at> gnu.org.
(Fri, 24 Feb 2023 00:13:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Bruno Victal <mirai <at> makinata.eu> To: guix-patches <at> gnu.org Cc: Bruno Victal <mirai <at> makinata.eu> Subject: [PATCH] services: base: Deprecate 'pam-limits-service' procedure. Date: Fri, 24 Feb 2023 00:12:10 +0000
* 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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Sat, 04 Mar 2023 21:18:02 GMT) Full text and rfc822 format available.Message #8 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Bruno Victal <mirai <at> makinata.eu> To: 61744 <at> debbugs.gnu.org Cc: Bruno Victal <mirai <at> makinata.eu> Subject: [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure. Date: Sat, 4 Mar 2023 21:17:38 +0000
* 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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Sat, 04 Mar 2023 21:18:02 GMT) Full text and rfc822 format available.Message #11 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Bruno Victal <mirai <at> makinata.eu> To: 61744 <at> debbugs.gnu.org Cc: Bruno Victal <mirai <at> makinata.eu> Subject: [PATCH v2 2/2] services: pam-limits-service-type: Deprecate file-like object support in favour for lists as service value. Date: Sat, 4 Mar 2023 21:17:39 +0000
* 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 <at> ajgrf.com>
# Copyright © 2022 ( <paren <at> disroot.org>
# Copyright © 2022 jgart <jgart <at> dismail.de>
+# Copyright © 2023 Bruno Victal <mirai <at> 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 <at> 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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Fri, 10 Mar 2023 18:16:01 GMT) Full text and rfc822 format available.Message #14 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <rekado <at> elephly.net> To: 61744 <at> debbugs.gnu.org Cc: mirai <at> makinata.eu Subject: [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure. Date: Fri, 10 Mar 2023 18:52:43 +0100
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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Sat, 11 Mar 2023 11:26:01 GMT) Full text and rfc822 format available.Message #17 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Bruno Victal <mirai <at> makinata.eu> To: Ricardo Wurmus <rekado <at> elephly.net> Cc: 61744 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [PATCH v2 1/2] services: base: Deprecate 'pam-limits-service' procedure. Date: Sat, 11 Mar 2023 11:25:13 +0000
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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Mon, 20 Mar 2023 17:50:01 GMT) Full text and rfc822 format available.Message #20 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Felix Lechner <felix.lechner <at> lease-up.com> To: 61744 <at> debbugs.gnu.org Cc: Ricardo Wurmus <rekado <at> elephly.net>, Bruno Victal <mirai <at> makinata.eu> Subject: Re: [PATCH] services: base: Deprecate 'pam-limits-service' procedure. Date: Mon, 20 Mar 2023 10:49:10 -0700
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
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Thu, 30 Mar 2023 20:55:01 GMT) Full text and rfc822 format available.Message #23 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Felix Lechner <felix.lechner <at> lease-up.com> Cc: Ricardo Wurmus <rekado <at> elephly.net>, 61744 <at> debbugs.gnu.org, Bruno Victal <mirai <at> makinata.eu> Subject: Re: bug#61744: [PATCH] services: base: Deprecate 'pam-limits-service' procedure. Date: Thu, 30 Mar 2023 22:53:54 +0200
Hi Felix, Felix Lechner <felix.lechner <at> 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’.
Ludovic Courtès <ludo <at> gnu.org>:Bruno Victal <mirai <at> makinata.eu>:Message #28 received at 61744-done <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Bruno Victal <mirai <at> makinata.eu> Cc: Ricardo Wurmus <rekado <at> elephly.net>, 61744-done <at> debbugs.gnu.org Subject: Re: bug#61744: [PATCH] services: base: Deprecate 'pam-limits-service' procedure. Date: Thu, 30 Mar 2023 23:08:06 +0200
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’.
guix-patches <at> gnu.org:bug#61744; Package guix-patches.
(Thu, 30 Mar 2023 21:20:02 GMT) Full text and rfc822 format available.Message #31 received at 61744 <at> debbugs.gnu.org (full text, mbox):
From: Felix Lechner <felix.lechner <at> lease-up.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Ricardo Wurmus <rekado <at> elephly.net>, 61744 <at> debbugs.gnu.org, Bruno Victal <mirai <at> makinata.eu> Subject: Re: bug#61744: [PATCH] services: base: Deprecate 'pam-limits-service' procedure. Date: Thu, 30 Mar 2023 14:19:06 -0700
Hi Ludovic, On Thu, Mar 30, 2023 at 1:54 PM Ludovic Courtès <ludo <at> 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
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Fri, 28 Apr 2023 11:24:12 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.