bug#61570: Backward incompatible changes in mpd-service-type

bug#61570: Backward incompatible changes in mpd-service-type

[Maxim Cournoyer]

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

bug#61570: Backward incompatible changes in mpd-service-type

[Bruno Victal]

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

bug#61570: Backward incompatible changes in mpd-service-type

[Liliana Marie Prikler]

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

bug#61570: Backward incompatible changes in mpd-service-type

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@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

bug#61570: Backward incompatible changes in mpd-service-type

[Liliana Marie Prikler]

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

bug#61570: [PATCH] services: mpd: Use proper records.

[Liliana Marie Prikler]

* 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

bug#61570: [PATCH] services: mpd: Use proper records.

[Bruno Victal]

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

bug#61570: [PATCH] services: mpd: Use proper records.

[Maxim Cournoyer]

Hi,

Bruno Victal <mirai@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

bug#61570: control-msg

[Bruno Victal]

Done with 7fdadeac11a997583305cb867b4a8828808ae953.