| Message ID | 364a2fe961ddce2c4668c0c8b78f46bffe2c2096.1679583701.git.mirai@makinata.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#62298,v2,1/8] services: configuration: Add user-defined sanitizer support. | expand |
Am Donnerstag, dem 23.03.2023 um 15:02 +0000 schrieb Bruno Victal: > +(define %mympd-user > + (user-account > + (name "mympd") > + (group "mympd") > + (system? #t) > + (comment "myMPD user") > + (home-directory "/var/empty") > + (shell (file-append shadow "/sbin/nologin")))) > + > +(define %mympd-group > + (user-group > + (name "mympd") > + (system? #t))) > + > +;;; TODO: procedures for unsupported value types, to be removed. > +(define (mympd-user-sanitizer value) > + (cond ((user-account? value) value) > + ((string? value) > + (warning (G_ "string value for 'user' is not supported, use > \ > +user-account instead~%")) > + (user-account > + (inherit %mympd-user) > + (name value) > + ;; XXX: this is to be lazily substituted in (…-accounts) > + ;; with the value from 'group'. > + (group %lazy-group))) > + (else > + (configuration-field-error #f 'user value)))) I think an in-place creation of the user and group might make more sense than defining a dummy value for inheritance purposes. Same probably also applies to the MPD service patch. Cheers
Hello, Bruno Victal <mirai@makinata.eu> writes: > services: mympd: Use user-account (resp. user-group) for user (resp. group) fields. > > * gnu/services/audio.scm (%mympd-user, %mympd-group): New variable. > (mympd-user-sanitizer, mympd-group-sanitizer): New procedure. > (mympd-configuration)[user, group]: Set value type to user-account (resp. user-group). > (mympd-serialize-configuration): Adapt for user-account values in user field. > (mympd-accounts): Adapt for user-account (resp. user-group) in user (resp. group) field. Please configure your editor for the 80 characters mark. > --- > doc/guix.texi | 4 +-- > gnu/services/audio.scm | 74 ++++++++++++++++++++++++++++++++++-------- > 2 files changed, 63 insertions(+), 15 deletions(-) > > diff --git a/doc/guix.texi b/doc/guix.texi > index 520a65b0b1..ee1e66b3ff 100644 > --- a/doc/guix.texi > +++ b/doc/guix.texi > @@ -33732,10 +33732,10 @@ Audio Services > This is a list of symbols naming Shepherd services that this service > will depend on. > > -@item @code{user} (default: @code{"mympd"}) (type: string) > +@item @code{user} (type: maybe-user-account) > Owner of the @command{mympd} process. > > -@item @code{group} (default: @code{"nogroup"}) (type: string) > +@item @code{group} (type: maybe-user-group) > Owner group of the @command{mympd} process. > > @item @code{work-directory} (default: @code{"/var/lib/mympd"}) (type: string) > diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm > index c168d1481c..f7f430039e 100644 > --- a/gnu/services/audio.scm > +++ b/gnu/services/audio.scm > @@ -659,6 +659,54 @@ (define-configuration/no-serialization mympd-ip-acl > (define-maybe/no-serialization integer) > (define-maybe/no-serialization mympd-ip-acl) > > +;; XXX: These will shadow the previous definition used by mpd > +;; and cause warnings to be shown. Maybe split the file > +;; into audio/mpd.scm and audio/mympd.scm ? > +#;(define-maybe/no-serialization user-account) > +#;(define-maybe/no-serialization user-group) I'd rather keeping them together if possible; could the prefix trick be used with them? No need for a hanging indent for continued text, here and for the other occurrences. The expressions commented; should they be? On another note, '#;' appears undocumented, I'd avoid it until it is (and it's not necessary here). > +(define %mympd-user > + (user-account > + (name "mympd") > + (group "mympd") > + (system? #t) > + (comment "myMPD user") > + (home-directory "/var/empty") > + (shell (file-append shadow "/sbin/nologin")))) > + > +(define %mympd-group > + (user-group > + (name "mympd") > + (system? #t))) > + > +;;; TODO: procedures for unsupported value types, to be removed. ^ Procedures > +(define (mympd-user-sanitizer value) > + (cond ((user-account? value) value) > + ((string? value) > + (warning (G_ "string value for 'user' is not supported, use \ > +user-account instead~%")) > + (user-account > + (inherit %mympd-user) > + (name value) > + ;; XXX: this is to be lazily substituted in (…-accounts) > + ;; with the value from 'group'. Extraneous hanging indent :-). > + (group %lazy-group))) > + (else > + (configuration-field-error #f 'user value)))) > + > +(define (mympd-group-sanitizer value) > + (cond ((user-group? value) value) > + ((string? value) > + (warning (G_ "string value for 'group' is not supported, use \ > +user-group instead~%")) > + (user-group > + (inherit %mympd-group) > + (name value))) > + (else > + (configuration-field-error #f 'group value)))) > +;;; Was this ';;;' added by mistake? > ;; XXX: The serialization procedures are insufficient since we require > ;; access to multiple fields at once. > ;; Fields marked with empty-serializer are never serialized and are > @@ -676,13 +724,15 @@ (define-configuration/no-serialization mympd-configuration > empty-serializer) > > (user > - (string "mympd") > + (maybe-user-account %mympd-user) > "Owner of the @command{mympd} process." > + (sanitizer mympd-user-sanitizer) > empty-serializer) > > (group > - (string "nogroup") > + (maybe-user-group %mympd-group) > "Owner group of the @command{mympd} process." > + (sanitizer mympd-group-sanitizer) > empty-serializer) > > (work-directory > @@ -817,7 +867,8 @@ (define (mympd-shepherd-service config) > (match-record config <mympd-configuration> (package shepherd-requirement > user work-directory > cache-directory log-level log-to) > - (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level))) > + (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level)) > + (username (user-account-name user))) > (shepherd-service > (documentation "Run the myMPD daemon.") > (requirement `(loopback user-processes > @@ -825,7 +876,7 @@ (define (mympd-shepherd-service config) > ,@shepherd-requirement)) > (provision '(mympd)) > (start #~(begin > - (let* ((pw (getpwnam #$user)) > + (let* ((pw (getpwnam #$username)) > (uid (passwd:uid pw)) > (gid (passwd:gid pw))) > (for-each (lambda (dir) > @@ -835,7 +886,7 @@ (define (mympd-shepherd-service config) > > (make-forkexec-constructor > `(#$(file-append package "/bin/mympd") > - "--user" #$user > + "--user" #$username > #$@(if (eqv? log-to 'syslog) '("--syslog") '()) > "--workdir" #$work-directory > "--cachedir" #$cache-directory) > @@ -845,14 +896,11 @@ (define (mympd-shepherd-service config) > > (define (mympd-accounts config) > (match-record config <mympd-configuration> (user group) > - (list (user-group (name group) > - (system? #t)) > - (user-account (name user) > - (group group) > - (system? #t) > - (comment "myMPD user") > - (home-directory "/var/empty") > - (shell (file-append shadow "/sbin/nologin")))))) > + ;; TODO: deprecation code, to be removed Please use a full sentence. > + (let ((user (if (eq? (user-account-group user) %lazy-group) > + (inject-group-into-user user group) > + user))) > + (list user group)))) > > (define (mympd-log-rotation config) > (match-record config <mympd-configuration> (log-to) LGTM, with the comments from Liliana taken into account.
On 2023-03-24 16:03, Maxim Cournoyer wrote:> Bruno Victal <mirai@makinata.eu> writes: >> >> +;; XXX: These will shadow the previous definition used by mpd >> +;; and cause warnings to be shown. Maybe split the file >> +;; into audio/mpd.scm and audio/mympd.scm ? >> +#;(define-maybe/no-serialization user-account) >> +#;(define-maybe/no-serialization user-group) > > I'd rather keeping them together if possible; could the prefix trick be > used with them? No need for a hanging indent for continued text, here > and for the other occurrences. The prefix trick is unlikely to help since the previous ones already use it. --8<---------------cut here---------------start------------->8--- (define-maybe user-account (prefix mpd-)) (define-maybe user-group (prefix mpd-)) --8<---------------cut here---------------end--------------->8--- I'm using define-maybe as a “cheat” here for prettier documentation generation. Without define-maybe, the documentation is rendered as: --8<---------------cut here---------------start------------->8--- @item @code{user} (type: user-account) The user to run mpd as. --8<---------------cut here---------------end--------------->8--- … which is the documentation format for a field that requires an explicit value, even though we are setting a default one using %mpd-account. On the other hand, using define-maybe yields: --8<---------------cut here---------------start------------->8--- @item @code{user} (type: maybe-user-account) The user to run mpd as. --8<---------------cut here---------------end--------------->8--- … which is the format for fields that do not require any manual intervention. > The expressions commented; should they be? On another note, '#;' > appears undocumented, I'd avoid it until it is (and it's not necessary > here). They're commented because the “right way of things” would have that the first maybe-user-account is mpd-configuration specific due to (prefix mpd-) and that we should have another maybe-user-account that is unrelated. Here, we're actually reusing the one from mpd and it happens to be “ok” since we do no serialization due to define-configuration/no-serialization. Regarding #;, it is documented, though perhaps in a indirect manner. It's SRFI-62, which is listed as supported in Guile manual. >> + (inherit %mympd-group) >> + (name value))) >> + (else >> + (configuration-field-error #f 'group value)))) >> +;;; > > Was this ';;;' added by mistake? It serves as an aid to demarcate the deprecation helpers to be erased when the time arrives. The helpers for MPD are also similarly demarcated.
On 2023-03-23 19:19, Liliana Marie Prikler wrote: > Am Donnerstag, dem 23.03.2023 um 15:02 +0000 schrieb Bruno Victal: >> +(define %mympd-user >> + (user-account >> + (name "mympd") >> + (group "mympd") >> + (system? #t) >> + (comment "myMPD user") >> + (home-directory "/var/empty") >> + (shell (file-append shadow "/sbin/nologin")))) >> + >> +(define %mympd-group >> + (user-group >> + (name "mympd") >> + (system? #t))) >> + >> +;;; TODO: procedures for unsupported value types, to be removed. >> +(define (mympd-user-sanitizer value) >> + (cond ((user-account? value) value) >> + ((string? value) >> + (warning (G_ "string value for 'user' is not supported, use >> \ >> +user-account instead~%")) >> + (user-account >> + (inherit %mympd-user) >> + (name value) >> + ;; XXX: this is to be lazily substituted in (…-accounts) >> + ;; with the value from 'group'. >> + (group %lazy-group))) >> + (else >> + (configuration-field-error #f 'user value)))) > I think an in-place creation of the user and group might make more > sense than defining a dummy value for inheritance purposes. Same > probably also applies to the MPD service patch. Though already replied to in private via IRC, to leave this clarified for other reviewers, these are not dummy values. They're the default values that the service will use if none are specified. The inheritance happened to be a bonus here. Cheers, Bruno
Am Samstag, dem 25.03.2023 um 00:33 +0000 schrieb Bruno Victal: > On 2023-03-24 16:03, Maxim Cournoyer wrote:> Bruno Victal > <mirai@makinata.eu> writes: > > > > > > +;; XXX: These will shadow the previous definition used by mpd > > > +;; and cause warnings to be shown. Maybe split the file > > > +;; into audio/mpd.scm and audio/mympd.scm ? > > > +#;(define-maybe/no-serialization user-account) > > > +#;(define-maybe/no-serialization user-group) > > > > I'd rather keeping them together if possible; could the prefix > > trick be > > used with them? No need for a hanging indent for continued text, > > here > > and for the other occurrences. > > The prefix trick is unlikely to help since the previous ones already > use it. > --8<---------------cut here---------------start------------->8--- > (define-maybe user-account (prefix mpd-)) > (define-maybe user-group (prefix mpd-)) > --8<---------------cut here---------------end--------------->8--- > > I'm using define-maybe as a “cheat” here for prettier documentation > generation. > Without define-maybe, the documentation is rendered as: > > --8<---------------cut here---------------start------------->8--- > @item @code{user} (type: user-account) > The user to run mpd as. > --8<---------------cut here---------------end--------------->8--- > > … which is the documentation format for a field that requires an > explicit value, even though we are setting a default one using %mpd- > account. > > On the other hand, using define-maybe yields: > > --8<---------------cut here---------------start------------->8--- > @item @code{user} (type: maybe-user-account) > The user to run mpd as. > --8<---------------cut here---------------end--------------->8--- > > … which is the format for fields that do not require any manual > intervention. This is a slight abuse of define-maybe, though. define-maybe communicates, that the field having no value, i.e. not even a default value, is acceptable. Since we always need a user to run MPD with, this makes no sense semantically. > > The expressions commented; should they be? On another note, '#;' > > appears undocumented, I'd avoid it until it is (and it's not > > necessary > > here). > > They're commented because the “right way of things” would have that > the first maybe-user-account is mpd-configuration specific due to > (prefix mpd-) and that we should have another maybe-user-account that > is unrelated. Since we require "explicit" users in both cases, I think we can work around this by dropping the maybe, no? Cheers
diff --git a/doc/guix.texi b/doc/guix.texi index 520a65b0b1..ee1e66b3ff 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -33732,10 +33732,10 @@ Audio Services This is a list of symbols naming Shepherd services that this service will depend on. -@item @code{user} (default: @code{"mympd"}) (type: string) +@item @code{user} (type: maybe-user-account) Owner of the @command{mympd} process. -@item @code{group} (default: @code{"nogroup"}) (type: string) +@item @code{group} (type: maybe-user-group) Owner group of the @command{mympd} process. @item @code{work-directory} (default: @code{"/var/lib/mympd"}) (type: string) diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm index c168d1481c..f7f430039e 100644 --- a/gnu/services/audio.scm +++ b/gnu/services/audio.scm @@ -659,6 +659,54 @@ (define-configuration/no-serialization mympd-ip-acl (define-maybe/no-serialization integer) (define-maybe/no-serialization mympd-ip-acl) +;; XXX: These will shadow the previous definition used by mpd +;; and cause warnings to be shown. Maybe split the file +;; into audio/mpd.scm and audio/mympd.scm ? +#;(define-maybe/no-serialization user-account) +#;(define-maybe/no-serialization user-group) + +(define %mympd-user + (user-account + (name "mympd") + (group "mympd") + (system? #t) + (comment "myMPD user") + (home-directory "/var/empty") + (shell (file-append shadow "/sbin/nologin")))) + +(define %mympd-group + (user-group + (name "mympd") + (system? #t))) + +;;; TODO: procedures for unsupported value types, to be removed. +(define (mympd-user-sanitizer value) + (cond ((user-account? value) value) + ((string? value) + (warning (G_ "string value for 'user' is not supported, use \ +user-account instead~%")) + (user-account + (inherit %mympd-user) + (name value) + ;; XXX: this is to be lazily substituted in (…-accounts) + ;; with the value from 'group'. + (group %lazy-group))) + (else + (configuration-field-error #f 'user value)))) + +(define (mympd-group-sanitizer value) + (cond ((user-group? value) value) + ((string? value) + (warning (G_ "string value for 'group' is not supported, use \ +user-group instead~%")) + (user-group + (inherit %mympd-group) + (name value))) + (else + (configuration-field-error #f 'group value)))) +;;; + + ;; XXX: The serialization procedures are insufficient since we require ;; access to multiple fields at once. ;; Fields marked with empty-serializer are never serialized and are @@ -676,13 +724,15 @@ (define-configuration/no-serialization mympd-configuration empty-serializer) (user - (string "mympd") + (maybe-user-account %mympd-user) "Owner of the @command{mympd} process." + (sanitizer mympd-user-sanitizer) empty-serializer) (group - (string "nogroup") + (maybe-user-group %mympd-group) "Owner group of the @command{mympd} process." + (sanitizer mympd-group-sanitizer) empty-serializer) (work-directory @@ -817,7 +867,8 @@ (define (mympd-shepherd-service config) (match-record config <mympd-configuration> (package shepherd-requirement user work-directory cache-directory log-level log-to) - (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level))) + (let ((log-level* (format #f "MYMPD_LOGLEVEL=~a" log-level)) + (username (user-account-name user))) (shepherd-service (documentation "Run the myMPD daemon.") (requirement `(loopback user-processes @@ -825,7 +876,7 @@ (define (mympd-shepherd-service config) ,@shepherd-requirement)) (provision '(mympd)) (start #~(begin - (let* ((pw (getpwnam #$user)) + (let* ((pw (getpwnam #$username)) (uid (passwd:uid pw)) (gid (passwd:gid pw))) (for-each (lambda (dir) @@ -835,7 +886,7 @@ (define (mympd-shepherd-service config) (make-forkexec-constructor `(#$(file-append package "/bin/mympd") - "--user" #$user + "--user" #$username #$@(if (eqv? log-to 'syslog) '("--syslog") '()) "--workdir" #$work-directory "--cachedir" #$cache-directory) @@ -845,14 +896,11 @@ (define (mympd-shepherd-service config) (define (mympd-accounts config) (match-record config <mympd-configuration> (user group) - (list (user-group (name group) - (system? #t)) - (user-account (name user) - (group group) - (system? #t) - (comment "myMPD user") - (home-directory "/var/empty") - (shell (file-append shadow "/sbin/nologin")))))) + ;; TODO: deprecation code, to be removed + (let ((user (if (eq? (user-account-group user) %lazy-group) + (inject-group-into-user user group) + user))) + (list user group)))) (define (mympd-log-rotation config) (match-record config <mympd-configuration> (log-to)