Backward incompatible changes in mpd-service-type

  • Done
  • quality assurance status badge
Details
3 participants
  • Liliana Marie Prikler
  • Maxim Cournoyer
  • Bruno Victal
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
M
M
Maxim Cournoyer wrote on 17 Feb 2023 13:53
(name . bug-guix)(address . bug-guix@gnu.org)
87y1owsbab.fsf@gmail.com
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:

Toggle snippet (11 lines)
@@ -239,7 +239,7 @@
(mpd-output
(name "streaming")
(type "httpd")
- (mixer-type 'null)
+ (mixer-type "none")
(extra-options
`((encoder . "lame")
(port . "6601")

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:

Toggle snippet (8 lines)
‘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’).

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

Toggle snippet (16 lines)
(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"))))))))

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
B
B
Bruno Victal wrote on 17 Feb 2023 16:33
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
8c7394ba-b8fa-eac5-7d3e-3d8160b71894@makinata.eu
Hi Maxim,

On 2023-02-17 12:53, Maxim Cournoyer wrote:
Toggle quote (11 lines)
> 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.

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

Toggle quote (23 lines)
> 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)

Toggle quote (3 lines)
> [...] 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'.


Toggle quote (4 lines)
> 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
L
L
Liliana Marie Prikler wrote on 17 Feb 2023 19:06
(address . 61570@debbugs.gnu.org)
959a4528abf2fec979e3816ff8d175f65d13d1ab.camel@gmail.com
Hi Bruno and Maxim,

Am Freitag, dem 17.02.2023 um 15:33 +0000 schrieb Bruno Victal:
Toggle quote (33 lines)
> > 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:
Toggle quote (3 lines)
> 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:
Toggle quote (2 lines)
> 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

L
L
Liliana Marie Prikler wrote on 18 Feb 2023 18:42
[PATCH] services: mpd: Use proper records.
(address . 61570@debbugs.gnu.org)
08fff16dcb7b4e2da9f4e2e05d010b1ea339b776.camel@gmail.com
* 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(-)

Toggle diff (104 lines)
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
B
B
Bruno Victal wrote on 19 Feb 2023 14:54
(address . 61570@debbugs.gnu.org)
41356046-dc40-0f77-77d4-16a951cc0798@makinata.eu
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
Toggle quote (5 lines)
> 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
M
M
Maxim Cournoyer wrote on 25 Feb 2023 21:13
(name . Bruno Victal)(address . mirai@makinata.eu)
87o7phec6q.fsf@gmail.com
Hi,

Bruno Victal <mirai@makinata.eu> writes:

Toggle quote (12 lines)
> 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
M
M
Maxim Cournoyer wrote on 7 Mar 2023 02:13
Re: Backward incompatible changes in mpd-service-type
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)
87bkl5z7lt.fsf@gmail.com
Hi Liliana,

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

[...]

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

Toggle quote (7 lines)
> 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
L
L
Liliana Marie Prikler wrote on 7 Mar 2023 06:31
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
e8c36c4f16827c00b2634723591f1a64c508efc3.camel@gmail.com
Am Montag, dem 06.03.2023 um 20:13 -0500 schrieb Maxim Cournoyer:
Toggle quote (9 lines)
> > 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.

Toggle quote (4 lines)
> 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
B
B
Bruno Victal wrote on 2 Apr 2023 16:42
control-msg
(address . 61570-done@debbugs.gnu.org)
f17a6175-8fd6-0512-0570-e118de71d1fb@makinata.eu
Done with 7fdadeac11a997583305cb867b4a8828808ae953.
Closed
?
Your comment

This issue is archived.

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

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