GNU bug report logs - #61570
Backward incompatible changes in mpd-service-type

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61570 in the body.
You can then email your comments to 61570 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: bug-guix <bug-guix <at> gnu.org>
Cc: Liliana Prikler <liliana.prikler <at> student.tugraz>,
 Bruno Victal <mirai <at> makinata.eu>
Subject: Backward incompatible changes in mpd-service-type
Date: Fri, 17 Feb 2023 07:53:48 -0500

Hi!

I wanted to note some findings regarding the improved
mpd-configuration/mdp-service-type (thanks!)
(5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1, "services: mpd: Refactor MPD
service.") after having reconfigured my machine yesterday.  I've noted
these two backward incompatible changes:

1. the mixer type must now be a string instead of a symbol, and takes
"none" instead of 'null for the null mixer:

--8<---------------cut here---------------start------------->8---
@@ -239,7 +239,7 @@
                        (mpd-output
                         (name "streaming")
                         (type "httpd")
-                        (mixer-type 'null)
+                        (mixer-type "none")
                         (extra-options
                          `((encoder . "lame")
                            (port    . "6601")
--8<---------------cut here---------------end--------------->8---

It'd be nice if there was some deprecation warning and automatic healing
code for now.  The doc should also be updated, because it still mention
the @code{null} value:

--8<---------------cut here---------------start------------->8---
     ‘mixer-type’ (default: ‘"none"’) (type: string)
          This field accepts a string that specifies which mixer should
          be used for this audio output: the ‘hardware’ mixer, the
          ‘software’ mixer, the ‘null’ mixer (allows setting the volume,
          but with no effect; this can be used as a trick to implement
          an external mixer External Mixer) or no mixer (‘none’).
--8<---------------cut here---------------end--------------->8---

2.  The MPD user appears to be created instead of using an existing
one.  I was using my own account, like this:

--8<---------------cut here---------------start------------->8---
      (service mpd-service-type
               (mpd-configuration
                (user "maxim")
                (address "0.0.0.0")
                (outputs
                 (list (mpd-output)
                       (mpd-output
                        (name "streaming")
                        (type "httpd")
                        (mixer-type "none")
                        (extra-options
                         `((encoder . "lame")
                           (port    . "6601")
                           (bitrate . "320"))))))))
--8<---------------cut here---------------end--------------->8---

and 'guix system reconfigure' is now warning that a user is defined
twice, and my user description following reconfiguration is: "Music
Player Daemon (MPD) user" :-).

Perhaps it should check if a user already exists and not add it if it
does?  Else an error rather than a warning when multiple same-name users
are defined would be more appropriate, I think.

-- 
Thanks,
Maxim

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Liliana Prikler <liliana.prikler <at> student.tugraz>,
 bug-guix <bug-guix <at> gnu.org>
Subject: Re: Backward incompatible changes in mpd-service-type
Date: Fri, 17 Feb 2023 15:33:35 +0000

Hi Maxim,

On 2023-02-17 12:53, Maxim Cournoyer wrote:
> Hi!
> 
> I wanted to note some findings regarding the improved
> mpd-configuration/mdp-service-type (thanks!)
> (5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1, "services: mpd: Refactor MPD
> service.") after having reconfigured my machine yesterday.  I've noted
> these two backward incompatible changes:
> 
> 1. the mixer type must now be a string instead of a symbol, and takes
> "none" instead of 'null for the null mixer:

Indeed, this was an unintentional API breakage. IMO taking symbols here
always stood out since most of the fields were happy taking strings as values.
A deprecated warning and a symbol->string can be added here for compatibility.

> --8<---------------cut here---------------start------------->8---
> @@ -239,7 +239,7 @@
>                         (mpd-output
>                          (name "streaming")
>                          (type "httpd")
> -                        (mixer-type 'null)
> +                        (mixer-type "none")
>                          (extra-options
>                           `((encoder . "lame")
>                             (port    . "6601")
> --8<---------------cut here---------------end--------------->8---
> 
> It'd be nice if there was some deprecation warning and automatic healing
> code for now.  The doc should also be updated, because it still mention
> the @code{null} value:
> 
> --8<---------------cut here---------------start------------->8---
>      ‘mixer-type’ (default: ‘"none"’) (type: string)
>           This field accepts a string that specifies which mixer should
>           be used for this audio output: the ‘hardware’ mixer, the
>           ‘software’ mixer, the ‘null’ mixer (allows setting the volume,
>           but with no effect; this can be used as a trick to implement
>           an external mixer External Mixer) or no mixer (‘none’).
> --8<---------------cut here---------------end--------------->8---

There's no mistake here, 'null' mixer is distinct from 'none' mixer.
<https://mpd.readthedocs.io/en/latest/user.html#configuring-audio-outputs>

> 2.  The MPD user appears to be created instead of using an existing
> one.  I was using my own account, like this:
> 
> --8<---------------cut here---------------start------------->8---
>       (service mpd-service-type
>                (mpd-configuration
>                 (user "maxim")
>                 (address "0.0.0.0")
>                 (outputs
>                  (list (mpd-output)
>                        (mpd-output
>                         (name "streaming")
>                         (type "httpd")
>                         (mixer-type "none")
>                         (extra-options
>                          `((encoder . "lame")
>                            (port    . "6601")
>                            (bitrate . "320"))))))))
> --8<---------------cut here---------------end--------------->8---
> 
> and 'guix system reconfigure' is now warning that a user is defined
> twice, [...]

This is an unfortunate situation arising from a bug before the service was refactored.
Before d7fd9ec209f72e9cfff04a48bf16e092f258d8ff (actually 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1)
mpd-service-type contained a service-extension for %mpd-accounts where the values for both group and user were hardcoded to "mpd"
but this was actually never used since shepherd would launch the service using root and mpd would downgrade its permissions and switch
to the user specified in the mpd-configuration record since this field is serialized to the configuration file.

No "warning for user defined twice" would show precisely because the service-extension %mpd-accounts for account-service-type was
hardcoded, unless your "user-account" happened to be named "mpd".

%mpd-accounts being hardcoded also meant that the mpd-service-activation procedure made little sense. (since it was
chown'ing and chmod'ing directories from the "mpd" user to the value in the 'user' field from mpd-configuration record type)

> [...] and my user description following reconfiguration is: "Music
> Player Daemon (MPD) user" :-).

AFAIK, the 'users' (resp. 'groups') fields from the 'operating-system' record type do not have precedence over account-service-type service extensions
(though this should be the case) so it's "whoever remains" after duplicates are filtered out, which in this case mpd-service-type happened to "win the
race".
I'd expect this behavior to happen with any service whose configuration record-type contains a user (resp. group) field that is relayed to a
account-service-type service-extension. (examples would be some of the git services if I'm not mistaken)

Another way to trigger this is with 'radicale', by manually creating the 'radicale' user in 'operating-system' (say that you want to change the
HOME directory since its where the data is saved to) and adding 'radicale-service-type' to 'services'.


> Perhaps it should check if a user already exists and not add it if it
> does?  Else an error rather than a warning when multiple same-name users
> are defined would be more appropriate, I think. 

I don't think service definitions have access to the operating-system field,
this was one of the hurdles when #60735 was being tackled.

IMO, this "issue" is avoided by not using the mpd-service-type as a "home service", which is what you're trying to do
when you pass it your own "interactive" user account.
Forcing mpd-service-type as a home service is fraught with subtle peculiarities, one of them was noted in commit message
637a9c3b264fe8eb76b6ed9f104b635d95632be6 and was discussed in #59866.

This service should be used "system-wide", which could be streaming oriented setups (via HTTP, icecast, PulseAudio + rtp, ...) or
system-wide PulseAudio. (not recommended)
"Home service" use cases should get its own home service definition, either by reusing many of the record-types and procedures here
or simply by inheriting and selectively deleting some of the service extensions. (such as account-service-type)


Cheers,
Bruno

Message #11 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Bruno Victal <mirai <at> makinata.eu>, Maxim Cournoyer
 <maxim.cournoyer <at> gmail.com>
Cc: 61570 <at> debbugs.gnu.org
Subject: Re: Backward incompatible changes in mpd-service-type
Date: Fri, 17 Feb 2023 19:06:50 +0100

Hi Bruno and Maxim,

Am Freitag, dem 17.02.2023 um 15:33 +0000 schrieb Bruno Victal:
> > 2.  The MPD user appears to be created instead of using an existing
> > one.  I was using my own account, like this:
> > 
> > --8<---------------cut here---------------start------------->8---
> >       (service mpd-service-type
> >                (mpd-configuration
> >                 (user "maxim")
> >                 (address "0.0.0.0")
> >                 (outputs
> >                  (list (mpd-output)
> >                        (mpd-output
> >                         (name "streaming")
> >                         (type "httpd")
> >                         (mixer-type "none")
> >                         (extra-options
> >                          `((encoder . "lame")
> >                            (port    . "6601")
> >                            (bitrate . "320"))))))))
> > --8<---------------cut here---------------end--------------->8---
> > 
> > and 'guix system reconfigure' is now warning that a user is defined
> > twice, [...]
> 
> This is an unfortunate situation arising from a bug before the
> service was refactored.
> Before d7fd9ec209f72e9cfff04a48bf16e092f258d8ff (actually
> 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1)
> mpd-service-type contained a service-extension for %mpd-accounts
> where the values for both group and user were hardcoded to "mpd"
> but this was actually never used since shepherd would launch the
> service using root and mpd would downgrade its permissions and switch
> to the user specified in the mpd-configuration record since this
> field is serialized to the configuration file.
It would be quite weird if someone had already pointed out how to
properly handle the accounts and groups only for that to be ignored
later in the review.

Am Samstag, dem 24.12.2022 um 18:20 +0100 schrieb eine leichtsinnige
Person, die ihre eigenen Anmerkungen vergisst:
> I think you should make it so that you can pass a user-account and
> user-group to the mpd service so that they can be reused (with a
> sanitizer that creates a user/group from string).
Never mind then.


Am Freitag, dem 17.02.2023 um 07:53 -0500 schrieb Maxim Cournoyer:
> Else an error rather than a warning when multiple same-name users are
> defined would be more appropriate, I think.
Guess what, it used to be a formatted message (i.e. an actual error). 
However, that broke some configs as reported in [1], so I demoted it to
a warning.

Cheers

[1] http://git.savannah.gnu.org/cgit/guix.git/commit/?id=8488f45b6e05d646224cc2b410497ddf9864c612

Message #14 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Bruno Victal
 <mirai <at> makinata.eu>
Cc: 61570 <at> debbugs.gnu.org
Subject: [PATCH] services: mpd: Use proper records.
Date: Sat, 18 Feb 2023 18:42:45 +0100

* gnu/services/audio.scm (mpd-user, mpd-group): New variables.
(mpd-serialize-user-account, mpd-serialize-user-group): New variables.
(mpd-configuration)[user]: Change type to user-account.
Change default accordingly.
[group]: Change type to user-group.  Change default accordingly.
(mpd-shepherd-service, mpd-accounts): Adjust accordingly.
* doc/guix.texi ("Music Player Daemon") Adjust accordingly.
---
This patch fixes the issue of not being able to insert actual users and
groups into MPD service.  Sadly, as define-configuration lacks proper
support for sanitizers, it's a backwards-incompatible change.
Perhaps it makes sense to extend define-configuration to support this
case?

Cheers

 doc/guix.texi          |  4 ++--
 gnu/services/audio.scm | 41 +++++++++++++++++++++++++++--------------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 44e2165a82..fa75eff62e 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -33181,10 +33181,10 @@ Data type representing the configuration of @command{mpd}.
 @item @code{package} (default: @code{mpd}) (type: file-like)
 The MPD package.
 
-@item @code{user} (default: @code{"mpd"}) (type: string)
+@item @code{user} (default: @code{(mpd-user "mpd")}) (type: user-account)
 The user to run mpd as.
 
-@item @code{group} (default: @code{"mpd"}) (type: string)
+@item @code{group} (default: @code{(mpd-group "mpd")}) (type: user-group)
 The group to run mpd as.
 
 @item @code{shepherd-requirement} (default: @code{()}) (type: list-of-symbol)
diff --git a/gnu/services/audio.scm b/gnu/services/audio.scm
index d55b804ba9..df9b0ac7f1 100644
--- a/gnu/services/audio.scm
+++ b/gnu/services/audio.scm
@@ -62,6 +62,9 @@ (define-module (gnu services audio)
             mpd-partition-name
             mpd-partition-extra-options
 
+            mpd-user
+            mpd-group
+
             mpd-configuration
             mpd-configuration?
             mpd-configuration-package
@@ -328,6 +331,26 @@ (define list-of-mpd-plugin-or-output?
   (list-of (lambda (x)
              (or (mpd-output? x) (mpd-plugin? x)))))
 
+(define* (mpd-user #:optional (name "mpd") (group "mpd"))
+  (user-account
+   (name name)
+   (group group)
+   (system? #t)
+   (comment "Music Player Daemon (MPD) user")
+   (home-directory "/var/lib/mpd")
+   (shell (file-append shadow "/sbin/nologin"))))
+
+(define* (mpd-group #:optional (name "mpd"))
+  (user-group
+   (name name)
+   (system? #t)))
+
+(define (mpd-serialize-user-account field value)
+  (mpd-serialize-string field (user-account-name value)))
+
+(define (mpd-serialize-user-group field value)
+  (mpd-serialize-string field (user-group-name value)))
+
 (define-configuration mpd-configuration
   (package
    (file-like mpd)
@@ -335,11 +358,11 @@ (define-configuration mpd-configuration
    empty-serializer)
 
   (user
-   (string "mpd")
+   (user-account (mpd-user "mpd"))
    "The user to run mpd as.")
 
   (group
-   (string "mpd")
+   (user-group (mpd-group "mpd"))
    "The group to run mpd as.")
 
   (shepherd-requirement
@@ -512,7 +535,7 @@ (define (mpd-shepherd-service config)
                   (and=> #$(maybe-value log-file)
                          (compose mkdir-p dirname))
 
-                  (let ((user (getpw #$user)))
+                  (let ((user (getpw #$(user-account-name user))))
                     (for-each
                      (lambda (x)
                        (when (and x (not (file-exists? x)))
@@ -546,17 +569,7 @@ (define (mpd-shepherd-service config)
 
 (define (mpd-accounts config)
   (match-record config <mpd-configuration> (user group)
-    (list (user-group
-           (name group)
-           (system? #t))
-          (user-account
-           (name user)
-           (group group)
-           (system? #t)
-           (comment "Music Player Daemon (MPD) user")
-           ;; MPD can use $HOME (or $XDG_CONFIG_HOME) to place its data
-           (home-directory "/var/lib/mpd")
-           (shell (file-append shadow "/sbin/nologin"))))))
+    (list user group)))
 
 (define mpd-service-type
   (service-type

base-commit: 312f1f41d3f3f3e5d2c36ff46920c6dce1c21a17
-- 
2.39.1

Message #17 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 61570 <at> debbugs.gnu.org
Subject: Re: [PATCH] services: mpd: Use proper records.
Date: Sun, 19 Feb 2023 13:54:41 +0000

Hi Maxim, Liliana

On 2023-02-18 17:42, Liliana Marie Prikler wrote:> This patch fixes the issue of not being able to insert actual users and
> groups into MPD service.  Sadly, as define-configuration lacks proper
> support for sanitizers, it's a backwards-incompatible change.
> Perhaps it makes sense to extend define-configuration to support this
> case?

I'd like to ask to hold merging this patch yet. I've got a few additional patches that addresses
some usability issues and IMO we can get a nicer patch by fixing define-configuration directly (whilst preserving API compatibility).
My intention is to patch define-configuration to accept a custom-sanitizer if specified.


Cheers,
Bruno

Message #20 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: Liliana Marie Prikler <liliana.prikler <at> gmail.com>, 61570 <at> debbugs.gnu.org
Subject: Re: [PATCH] services: mpd: Use proper records.
Date: Sat, 25 Feb 2023 15:13:01 -0500

Hi,

Bruno Victal <mirai <at> makinata.eu> writes:

> Hi Maxim, Liliana
>
> On 2023-02-18 17:42, Liliana Marie Prikler wrote:> This patch fixes the issue of not being able to insert actual users and
>> groups into MPD service.  Sadly, as define-configuration lacks proper
>> support for sanitizers, it's a backwards-incompatible change.
>> Perhaps it makes sense to extend define-configuration to support this
>> case?
>
> I'd like to ask to hold merging this patch yet. I've got a few additional patches that addresses
> some usability issues and IMO we can get a nicer patch by fixing define-configuration directly (whilst preserving API compatibility).
> My intention is to patch define-configuration to accept a custom-sanitizer if specified.

Sounds good!  Thanks for working toward that.

-- 
Thanks,
Maxim

Message #23 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: Bruno Victal <mirai <at> makinata.eu>, 61570 <at> debbugs.gnu.org
Subject: Re: Backward incompatible changes in mpd-service-type
Date: Mon, 06 Mar 2023 20:13:34 -0500

Hi Liliana,

Liliana Marie Prikler <liliana.prikler <at> gmail.com> writes:

[...]

>> This is an unfortunate situation arising from a bug before the
>> service was refactored.
>> Before d7fd9ec209f72e9cfff04a48bf16e092f258d8ff (actually
>> 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1)
>> mpd-service-type contained a service-extension for %mpd-accounts
>> where the values for both group and user were hardcoded to "mpd"
>> but this was actually never used since shepherd would launch the
>> service using root and mpd would downgrade its permissions and switch
>> to the user specified in the mpd-configuration record since this
>> field is serialized to the configuration file.
> It would be quite weird if someone had already pointed out how to
> properly handle the accounts and groups only for that to be ignored
> later in the review.
>
> Am Samstag, dem 24.12.2022 um 18:20 +0100 schrieb eine leichtsinnige
> Person, die ihre eigenen Anmerkungen vergisst:
>> I think you should make it so that you can pass a user-account and
>> user-group to the mpd service so that they can be reused (with a
>> sanitizer that creates a user/group from string).
> Never mind then.

I think Bruno has been reworking that, I think they must be about ready.

> Am Freitag, dem 17.02.2023 um 07:53 -0500 schrieb Maxim Cournoyer:
>> Else an error rather than a warning when multiple same-name users are
>> defined would be more appropriate, I think.
> Guess what, it used to be a formatted message (i.e. an actual error). 
> However, that broke some configs as reported in [1], so I demoted it to
> a warning.

Interesting.  I didn't know we were usefully (?) abusing duplicate users
and group.  Perhaps we should try to isolate the most common offenders
(services?), fix them up, and then re-introduce the check, perhaps
gradually (e.g. "in 6 months time, duplicated users or groups will
become a configuration error").

-- 
Thanks,
Maxim

Message #26 received at 61570 <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Bruno Victal <mirai <at> makinata.eu>, 61570 <at> debbugs.gnu.org
Subject: Re: Backward incompatible changes in mpd-service-type
Date: Tue, 07 Mar 2023 06:31:43 +0100

Am Montag, dem 06.03.2023 um 20:13 -0500 schrieb Maxim Cournoyer:
> > Am Freitag, dem 17.02.2023 um 07:53 -0500 schrieb Maxim Cournoyer:
> > > Else an error rather than a warning when multiple same-name users
> > > are defined would be more appropriate, I think.
> > Guess what, it used to be a formatted message (i.e. an actual
> > error).  However, that broke some configs as reported in [1], so I
> > demoted it to a warning.
> 
> Interesting.  I didn't know we were usefully (?) abusing duplicate
> users and group.
As far as I'm aware, we aren't.  Even if such uses exist, they raise
said warning and probably cause more issues down the line, like with
your bug report.

> Perhaps we should try to isolate the most common offenders
> (services?), fix them up, and then re-introduce the check, perhaps
> gradually (e.g. "in 6 months time, duplicated users or groups will
> become a configuration error").
The only instance known to me (cups creating a duplicate lp group) was
fixed back in 2021.

Cheers

Message #31 received at 61570-done <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: 61570-done <at> debbugs.gnu.org
Subject: control-msg
Date: Sun, 2 Apr 2023 15:42:24 +0100

Done with 7fdadeac11a997583305cb867b4a8828808ae953.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.