bug#38172: fixing dangerous PulseAudio defaults and giving it a record type

bug#38172: fixing dangerous PulseAudio defaults and giving it a record type

[raingloom]

Discussion from IRC:
http://logs.guix.gnu.org/guix/2019-11-11.log#213424

Basically it makes volume settings behave in erratic and surprising
ways that are not obvious even to seasoned Linux users, including
sudden loud noises.

The fix is as simple as adding "flat-volumes = no"
to .config/pulse/daemon.conf, but this should really be a system
default.

However, rather than patching it, the consensus seems to be that we
should create a <pulseaudio-configuration> record type.
http://logs.guix.gnu.org/guix/2019-11-11.log#213953

bug#38172: fixing dangerous PulseAudio defaults and giving it a record type

[Leo Prikler]

[-- Attachment #1: Type: text/plain, Size: 2 bytes --]



[-- Attachment #2: 0001-services-Add-PulseAudio-service.patch --]
[-- Type: text/x-patch, Size: 3094 bytes --]

From d48594a3e7e02aef0c5ff9fff719c1d0fb45207e Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Tue, 12 Nov 2019 02:08:40 +0100
Subject: [PATCH] services: Add PulseAudio service

* gnu/services/sound.scm: (<pulseaudio-configuration>): New record type.
(pulseaudio-service-type): New service type.
---
 gnu/services/sound.scm | 58 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index f2dd24402f..2aedc03c75 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -30,7 +30,9 @@
   #:use-module (gnu packages pulseaudio)
   #:use-module (ice-9 match)
   #:export (alsa-configuration
-            alsa-service-type))
+            alsa-service-type
+            pulseaudio-configuration
+            pulseaudio-service-type))
 
 ;;; Commentary:
 ;;;
@@ -97,4 +99,58 @@ ctl.!default {
    (default-value (alsa-configuration))
    (description "Configure low-level Linux sound support, ALSA.")))
 
+\f
+;;;
+;;; PulseAudio
+;;;
+
+(define-record-type* <pulseaudio-configuration>
+  pulseaudio-configuration make-pulseaudio-configuration
+  pulseaudio-configuration?
+  (package pulseaudio-package (default pulseaudio))
+  (client-conf pulseaudio-client-conf (default '()))
+  (daemon-conf pulseaudio-daemon-conf (default '((flat-volumes no))))
+  (default-script pulseaudio-default-script (default #f))
+  (system-script pulseaudio-system-script (default #f)))
+
+(define (pulseaudio-conf-entry arg)
+  (match arg
+    ((key value)
+     (format #f "~a = ~a~%" key value))
+    ((? string? _)
+     (string-append arg "\n"))))
+
+(define pulseaudio-etc-service
+  (match-lambda
+    (($ <pulseaudio-configuration> package client-conf daemon-conf
+                                   default-script system-script)
+     (let ((default.pa (if default-script
+                           (apply mixed-text-file "default.pa"
+                                  default-script)
+                           (file-append package "/etc/pulse/default.pa"))))
+       `(("pulse"
+          ,(file-union
+            "pulse"
+            `(("client.conf"
+               ,(apply mixed-text-file "client.conf"
+                       (map pulseaudio-conf-entry client-conf)))
+              ("daemon.conf"
+               ,(apply mixed-text-file "daemon.conf"
+                       "default-script-file = " default.pa "\n"
+                       (map pulseaudio-conf-entry daemon-conf)))
+              ("default.pa" ,default.pa)
+              ("system.pa"
+               ,(if default-script
+                    (apply mixed-text-file "system.pa"
+                           system-script)
+                    (file-append package "/etc/pulse/system.pa")))))))))))
+
+(define pulseaudio-service-type
+  (service-type
+   (name 'pulseaudio)
+   (extensions
+    (list (service-extension etc-service-type pulseaudio-etc-service)))
+   (default-value (pulseaudio-configuration))
+   (description "Configure PulseAudio.")))
+
 ;;; sound.scm ends here
-- 
2.24.0

bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

[Leo Prikler]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

Hi Guix,

After looking at my older patch (which no longer cleanly applies), I've
noticed, that pulseaudio doesn't even read the files from /etc.  This
is troublesome in multiple ways.  For one, pulseaudio causes >500
rebuilds (with >900 dependent packages) and is therefore staging
material, for the other, hardcoding /etc in such a way breaks
pulseaudio without the service.

So far, I've only tested containers via `guix environment --container`, 
but from what I can gather with strace, the config file is indeed read
and hence flat-volumes are eliminated.  Other ways of making pulseaudio
accept /etc are very welcome.  Looking at Nix, they configure
pulseaudio with "--sysconfdir=/etc", but then override sysconfdir and
pulseconfdir during install.  I'm not quite sure which solution is
"better", but neither is going to read the config shipped with the
package.

Note: before this can be applied on staging,
a66ee82a05d8ff1ef7c5ff9ac7723cb32fc4e22a needs to be applied.

Regards,
Leo



[-- Attachment #2: 0001-services-Add-pulseaudio-configuration.patch --]
[-- Type: text/x-patch, Size: 3312 bytes --]

From bf4708923d14356c87daec69209b30aa0427d64f Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Wed, 8 Jan 2020 19:50:51 +0100
Subject: [PATCH 1/3] services: Add pulseaudio-configuration.

* gnu/services/sound (<pulseaudio-configuration>): New record.
(pulseaudio-etc): New procedure.
(pulseaudio-service-type): Update accordingly.
---
 gnu/services/sound.scm | 47 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index aaca733729..f01d958ce7 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -34,6 +34,7 @@
   #:export (alsa-configuration
             alsa-service-type
 
+            pulseaudio-configuration
             pulseaudio-service-type))
 
 ;;; Commentary:
@@ -106,19 +107,61 @@ ctl.!default {
 ;;; PulseAudio
 ;;;
 
+(define-record-type* <pulseaudio-configuration>
+  pulseaudio-configuration make-pulseaudio-configuration
+  pulseaudio-configuration?
+  (package pulseaudio-package (default pulseaudio))
+  (client-conf pulseaudio-client-conf (default '()))
+  (daemon-conf pulseaudio-daemon-conf (default '((flat-volumes no))))
+  (default-script pulseaudio-default-script (default #f))
+  (system-script pulseaudio-system-script (default #f)))
+
 (define (pulseaudio-environment config)
   ;; Define this variable in the global environment such that
   ;; pulseaudio swh-plugins works.
   `(("LADSPA_PATH"
      . ,(file-append swh-plugins "/lib/ladspa"))))
 
+(define (pulseaudio-conf-entry arg)
+  (match arg
+    ((key value)
+     (format #f "~a = ~s~%" key value))
+    ((? string? _)
+     (string-append arg "\n"))))
+
+(define pulseaudio-etc
+  (match-lambda
+    (($ <pulseaudio-configuration> package client-conf daemon-conf
+                                   default-script system-script)
+     (let ((default.pa (if default-script
+                           (apply mixed-text-file "default.pa"
+                                  default-script)
+                           (file-append package "/etc/pulse/default.pa"))))
+       `(("pulse"
+          ,(file-union
+            "pulse"
+            `(("client.conf"
+               ,(apply mixed-text-file "client.conf"
+                       (map pulseaudio-conf-entry client-conf)))
+              ("daemon.conf"
+               ,(apply mixed-text-file "daemon.conf"
+                       "default-script-file = " default.pa "\n"
+                       (map pulseaudio-conf-entry daemon-conf)))
+              ("default.pa" ,default.pa)
+              ("system.pa"
+               ,(if system-script
+                    (apply mixed-text-file "system.pa"
+                           system-script)
+                    (file-append package "/etc/pulse/system.pa")))))))))))
+
 (define pulseaudio-service-type
   (service-type
    (name 'pulseaudio)
    (extensions
     (list (service-extension session-environment-service-type
-                             pulseaudio-environment)))
-   (default-value #f)
+                             pulseaudio-environment)
+          (service-extension etc-service-type pulseaudio-etc)))
+   (default-value (pulseaudio-configuration))
    (description "Configure PulseAudio sound support.")))
 
 ;;; sound.scm ends here
-- 
2.24.1


[-- Attachment #3: 0002-gnu-pulseaudio-Honor-etc.patch --]
[-- Type: text/x-patch, Size: 1237 bytes --]

From 843d3968db990b5b7ff3f618db5847f83b999cb8 Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Thu, 9 Jan 2020 01:24:09 +0100
Subject: [PATCH 2/3] gnu: pulseaudio: Honor /etc.

* gnu/packages/pulseaudio.scm (pulseaudio) [phases]:
Set PA_DEFAULT_CONFIG_DIR to "/etc/pulse".
---
 gnu/packages/pulseaudio.scm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gnu/packages/pulseaudio.scm b/gnu/packages/pulseaudio.scm
index 671dcd1563..1fb5a2f578 100644
--- a/gnu/packages/pulseaudio.scm
+++ b/gnu/packages/pulseaudio.scm
@@ -161,6 +161,11 @@ rates.")
                                               (assoc-ref %outputs "out")
                                               "/lib/udev/rules.d"))
        #:phases (modify-phases %standard-phases
+                 (add-after 'configure 'hardcode-default-config-dir
+                   (lambda _
+                     (substitute* "config.h"
+                       (("(#define PA_DEFAULT_CONFIG_DIR).*$" all prefix)
+                        (string-append prefix " \"/etc/pulse\"")))))
                  (add-before 'check 'pre-check
                    (lambda _
                      ;; 'tests/lock-autospawn-test.c' wants to create a file
-- 
2.24.1


[-- Attachment #4: 0003-services-Add-pulseaudio-to-desktop-services.patch --]
[-- Type: text/x-patch, Size: 760 bytes --]

From e24016f9a44a113847dd937ac47ab4bdb960236d Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Thu, 9 Jan 2020 01:29:13 +0100
Subject: [PATCH 3/3] services: Add pulseaudio to %desktop-services.

* gnu/services/desktop.scm (%desktop-services): Add pulseaudio service.
---
 gnu/services/desktop.scm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gnu/services/desktop.scm b/gnu/services/desktop.scm
index b40622a637..1be05fda4e 100644
--- a/gnu/services/desktop.scm
+++ b/gnu/services/desktop.scm
@@ -1183,6 +1183,7 @@ or setting its password with passwd.")))
 
          x11-socket-directory-service
 
+         (service pulseaudio-service-type)
          (service alsa-service-type)
 
          %base-services))
-- 
2.24.1

bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 5866 bytes --]

Leo,

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Hi Guix,
>
> After looking at my older patch (which no longer cleanly applies), I've
> noticed, that pulseaudio doesn't even read the files from /etc.  This
> is troublesome in multiple ways.  For one, pulseaudio causes >500
> rebuilds (with >900 dependent packages) and is therefore staging
> material, for the other, hardcoding /etc in such a way breaks
> pulseaudio without the service.
>
> So far, I've only tested containers via `guix environment --container`, 
> but from what I can gather with strace, the config file is indeed read
> and hence flat-volumes are eliminated.  Other ways of making pulseaudio
> accept /etc are very welcome.  Looking at Nix, they configure
> pulseaudio with "--sysconfdir=/etc", but then override sysconfdir and
> pulseconfdir during install.  I'm not quite sure which solution is
> "better", but neither is going to read the config shipped with the
> package.
>
> Note: before this can be applied on staging,
> a66ee82a05d8ff1ef7c5ff9ac7723cb32fc4e22a needs to be applied.

Thank you for these patches.  Overall it LGTM.

[...]

> From bf4708923d14356c87daec69209b30aa0427d64f Mon Sep 17 00:00:00 2001
> From: Leo Prikler <leo.prikler@student.tugraz.at>
> Date: Wed, 8 Jan 2020 19:50:51 +0100
> Subject: [PATCH 1/3] services: Add pulseaudio-configuration.
>
> * gnu/services/sound (<pulseaudio-configuration>): New record.
> (pulseaudio-etc): New procedure.
> (pulseaudio-service-type): Update accordingly.

[...]

> +(define-record-type* <pulseaudio-configuration>
> +  pulseaudio-configuration make-pulseaudio-configuration
> +  pulseaudio-configuration?
> +  (package pulseaudio-package (default pulseaudio))
> +  (client-conf pulseaudio-client-conf (default '()))
> +  (daemon-conf pulseaudio-daemon-conf (default '((flat-volumes no))))

I have a preference for making this field empty initially to have a 1:1
compatibility with the current PA client and daemon configuration
(i.e. nothing).  Then a follow-up patch can add this new configuration,
perhaps with an explaining comment.

> +  (default-script pulseaudio-default-script (default #f))
> +  (system-script pulseaudio-system-script (default #f)))
> +
>  (define (pulseaudio-environment config)
>    ;; Define this variable in the global environment such that
>    ;; pulseaudio swh-plugins works.
>    `(("LADSPA_PATH"
>       . ,(file-append swh-plugins "/lib/ladspa"))))
>  
> +(define (pulseaudio-conf-entry arg)
> +  (match arg
> +    ((key value)
> +     (format #f "~a = ~s~%" key value))
> +    ((? string? _)
> +     (string-append arg "\n"))))
> +
> +(define pulseaudio-etc
> +  (match-lambda
> +    (($ <pulseaudio-configuration> package client-conf daemon-conf
> +                                   default-script system-script)
> +     (let ((default.pa (if default-script
> +                           (apply mixed-text-file "default.pa"
> +                                  default-script)
> +                           (file-append package "/etc/pulse/default.pa"))))
> +       `(("pulse"
> +          ,(file-union
> +            "pulse"
> +            `(("client.conf"
> +               ,(apply mixed-text-file "client.conf"
> +                       (map pulseaudio-conf-entry client-conf)))
> +              ("daemon.conf"
> +               ,(apply mixed-text-file "daemon.conf"
> +                       "default-script-file = " default.pa "\n"
> +                       (map pulseaudio-conf-entry daemon-conf)))
> +              ("default.pa" ,default.pa)
> +              ("system.pa"
> +               ,(if system-script
> +                    (apply mixed-text-file "system.pa"
> +                           system-script)
> +                    (file-append package "/etc/pulse/system.pa")))))))))))
> +

Does it make sense to have default-script and system-script default to
(file-append pulseaudio "...") and avoid the conditional altogether?

[...]

> From 843d3968db990b5b7ff3f618db5847f83b999cb8 Mon Sep 17 00:00:00 2001
> From: Leo Prikler <leo.prikler@student.tugraz.at>
> Date: Thu, 9 Jan 2020 01:24:09 +0100
> Subject: [PATCH 2/3] gnu: pulseaudio: Honor /etc.
>
> * gnu/packages/pulseaudio.scm (pulseaudio) [phases]:
> Set PA_DEFAULT_CONFIG_DIR to "/etc/pulse".

This means pulseaudio will start looking in /etc/pulse for configuration
files on foreign distributions too, right?

I wonder if there is better way to give it configuration files.  Perhaps
by patching the D-Bus service files?  Not a blocker for this series, but
something to consider in case /etc/pulse causes trouble.

[...]

> +                 (add-after 'configure 'hardcode-default-config-dir
> +                   (lambda _
> +                     (substitute* "config.h"
> +                       (("(#define PA_DEFAULT_CONFIG_DIR).*$" all prefix)
> +                        (string-append prefix " \"/etc/pulse\"")))))

End on #t.

[...]

> From e24016f9a44a113847dd937ac47ab4bdb960236d Mon Sep 17 00:00:00 2001
> From: Leo Prikler <leo.prikler@student.tugraz.at>
> Date: Thu, 9 Jan 2020 01:29:13 +0100
> Subject: [PATCH 3/3] services: Add pulseaudio to %desktop-services.
>
> * gnu/services/desktop.scm (%desktop-services): Add pulseaudio service.

This will pull in "swh-plugins" which was the original intention behind
pulseaudio-service before this patch series.  Before adding it to
%desktop-services, I would prefer if the pulseaudio environment
configuration could be made modular, so that there are no configuration
differences for end users, i.e. they'd have to actively enable the
LADSPA plugin.

I'm not sure what the best approach would be though.  Ideas, Oleg?

As a final note, can you also update doc/guix.texi accordingly?

TIA,
Marius

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

[Leo Prikler]

Am Donnerstag, den 09.01.2020, 21:48 +0100 schrieb Marius Bakke:
> 
> I have a preference for making this field empty initially to have a
> 1:1
> compatibility with the current PA client and daemon configuration
> (i.e. nothing).  Then a follow-up patch can add this new
> configuration,
> perhaps with an explaining comment.
Fair enough.  This would mean I'd have to split 0001 into two, but
okay.

> Does it make sense to have default-script and system-script default
> to
> (file-append pulseaudio "...") and avoid the conditional altogether?
The idea behind it was to have the script itself in the code rather
than asking users to construct a mixed-text-file, but I'm fine either
way.

> This means pulseaudio will start looking in /etc/pulse for
> configuration
> files on foreign distributions too, right?
> 
> I wonder if there is better way to give it configuration
> files.  Perhaps
> by patching the D-Bus service files?  Not a blocker for this series,
> but
> something to consider in case /etc/pulse causes trouble.
This is already addressed by the renewed series I sent to guix-patches. 
I know you already found that, but I'd like to repeat it for those who
thus far have only read this thread.

> End on #t.
As above, but thanks for the hint, I missed the warning it seems.

> [...]
> 
> > From e24016f9a44a113847dd937ac47ab4bdb960236d Mon Sep 17 00:00:00
> > 2001
> > From: Leo Prikler <leo.prikler@student.tugraz.at>
> > Date: Thu, 9 Jan 2020 01:29:13 +0100
> > Subject: [PATCH 3/3] services: Add pulseaudio to %desktop-services.
> > 
> > * gnu/services/desktop.scm (%desktop-services): Add pulseaudio
> > service.
> 
> This will pull in "swh-plugins" which was the original intention
> behind
> pulseaudio-service before this patch series.  Before adding it to
> %desktop-services, I would prefer if the pulseaudio environment
> configuration could be made modular, so that there are no
> configuration
> differences for end users, i.e. they'd have to actively enable the
> LADSPA plugin.
I think adding a field ladspa-plugins, which accepts a list of packages
and adds their "lib/ladspa" would be the right approach here, but I
also feel, that this perhaps deserves its own service unrelated to
pulseaudio.  WDYT?
Either way, I agree on the "having to actively enable" part. 

> As a final note, can you also update doc/guix.texi accordingly?
I will once I've figured out how to best handle these fields.

Regards,
Leo

bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Am Donnerstag, den 09.01.2020, 21:48 +0100 schrieb Marius Bakke:
>> 
>> I have a preference for making this field empty initially to have a
>> 1:1
>> compatibility with the current PA client and daemon configuration
>> (i.e. nothing).  Then a follow-up patch can add this new
>> configuration,
>> perhaps with an explaining comment.
> Fair enough.  This would mean I'd have to split 0001 into two, but
> okay.

Excellent.  :-)

>> Does it make sense to have default-script and system-script default
>> to
>> (file-append pulseaudio "...") and avoid the conditional altogether?
> The idea behind it was to have the script itself in the code rather
> than asking users to construct a mixed-text-file, but I'm fine either
> way.

Right.  I just have a preference for the default being "up-front",
instead of magic hiding behind an #f.  :-)

There's also LOCAL-FILE and PLAIN-FILE, which are more "obvious" than
MIXED-TEXT-FILE.

It could be useful to support plain strings for users who don't wish to
much about with G-expressions though, hopefully users will send a bug
report if they find it limiting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]